Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge engine branch into master #382

Merged
merged 93 commits into from Dec 9, 2019

Conversation

@thrasher-
Copy link
Collaborator

thrasher- commented Nov 24, 2019

Description

This is the work of several months implementing a wide arrange of features, performance improvements and bug fixes.

Notable changes

  • gRPC client/server (see client and server functionality to manage the running bot instance
  • Database support (Postgres and SQLite3). See database
  • Several websocket improvements (performance, feature and currency pair handling)
  • New asset package
  • Restructuring of project/code to make it more modular and structured
  • Currency pair syncer which syncs all enabled currency pairs and chooses REST or websocket depending on exchange support and whether websocket is enabled/disabled
  • Several cmd utilities for checking exchange wrapper coverage, issues, OTP generation, database related tools (dbmigrate and gen_sqlboiler_config) and TLS cert generation for gRPC
  • Exchange wrapper template updates
  • Dispatcher service
  • Docker updates (support Go 1.13 plus the ability to build gctcli)
  • Bash/ZSH auto completion
  • Improved logger which supports optional levels (INFO/WARN/DEBUG/ERROR), output (console/file), file rotation and preserves your original advanced settings
  • Hundreds of bug fixes, QA improvements across the entire codebase
  • Increased test coverage for core packages (e.g config/exchange handling)
  • Orderbook simulation code and the ability to target specific pricing (aka whalebomb). However, this currently isn't linked up to the order manager.

WARNING

Your config will be upgraded to the new format automatically. However, in case of any unforeseen issues, please make a backup of your previous config

Fixes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

It is extremely hard to review this PR given the size of it but the people who have been using it frequently will tACK/nACK accordingly.

Several contributors who work on GCT consistently
go test ./... -race and CI

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules
thrasher- and others added 30 commits Oct 30, 2018
New GetExchangeOTPs API
CLI validation
Standardised pairs for GCTCLI
Expand test coverage
Trim SMS global from name is len > 11
Linter fixes
Link up various subsystems to be managed atomically with the ability to start/stop them
New subsystem APIs
Comms changes
Add back events tests
Fill out SMTP comms handler
Add getcommunicationrelayers gRPC command
* Initial update

* update

* Fix linter issues

* Add new documentation template and fix
Add addr helpers (will be split off into own package)
Engine status updates (log and data dir display)
Use GetPairFormat for various exchanges instead of calling the config
QA fixes
Implement GCTRPC exchange deposit address handling
* First pass at adding new logging system

* NewLogger

* NewLogger

* WIP

* silly bug fix

* :D removed files

* removed old logging interface

* added tests

* added tests

* Started to add new lines to all f calls

* Added subsystem log types

* Logger improvements

* Further performance improvements

* changes to logger and sublogger creation

* Renamed Logging types

* removed old print statement

* changes based on feedback

* moved sublogger types to own file

* :)

* added console as output type

* added get level command

* added get/set log level via grpc command

* added check for output being empty for migration support

* first pass at log rotation

* added log rotation

* :D derp fixed

* added tests

* changes based on feedback

* changed log type

* comments

* renamed file -> fileSettings

* typo fix

* changes based on feedback

* gofmt ran on additional files

* gofmt ran on additional files
lozdog245 and others added 2 commits Nov 19, 2019
* Change exchanges usage of GetName to Name

* Changed GetName to Name
* 1) Update Dockerfile/docker-compose.yml
2) Remove inline strings for buy/sell/test pairs
3) Remove dangerous order submission values
4) Fix consistency with audit_events (all other spec files use
CamelCase)
5) Update web websocket endpoint
6) Fix main param set (and induce dryrun mode on specific command line
params)

* Engine QA

Link up exchange syncer to cmd params, disarm market selling bombs and fix OKEX endpoints

* Fix linter issue after merge

* Engine QA changes

Template updates
Wrapper code cleanup
Disarmed order bombs
Documentation updates

* Daily engine QA

Bitstamp improvements
Spelling mistakes
Add Coinbene exchange to support list
Protect API authenticated calls for Coinbene/LBank

* Engine QA changes

Fix exchange_wrapper_coverage tool
Add SupportsAsset to exchange interface
Fix inline string usage and add BCH withdrawal support

* Engine QA

Fix Bitstamp types
Inform user of errors when parsing time accross the codebase
Change time parsing warnings to errors (as they are)
Update markdown docs [with linter fixes]

* Engine QA changes

1) Add test for dryrunParamInteraction
2) Disarm OKCoin/OKEX bombs if someone accidently sets canManipulateRealOrders to true and runs all package tests
3) Actually check exchange setup errors for BTSE and Coinbene, plus address this in the wrapper template
4) Hardcode missing/non-retrievable contributors and bump the contributors
5) Convert numbers/strings to meaningful types in Bitstamp and OKEX
6) If WS is supported for the exchange wrapper template, preset authWebsocketSupport var

* Fix the shadow people

* Link the SyncContinuously paramerino

* Also show SyncContinuously in engine.PrintSettings

* Address nitterinos and use correct filepath for logs

* Bitstamp: Extract ALL THE APM

* Fix additional nitterinos

* Fix time parsing error for Bittrex
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 24, 2019

Codecov Report

Merging #382 into master will decrease coverage by 2.09%.
The diff coverage is 83.31%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #382     +/-   ##
========================================
- Coverage   43.49%   41.4%   -2.1%     
========================================
  Files         129     163     +34     
  Lines       30628   38341   +7713     
========================================
+ Hits        13323   15874   +2551     
- Misses      16284   21352   +5068     
- Partials     1021    1115     +94
Impacted Files Coverage Δ
engine/timekeeper.go 0% <ø> (ø)
...rexprovider/openexchangerates/openexchangerates.go 57.6% <ø> (-0.47%) ⬇️
currency/conversion.go 70.05% <ø> (+11.98%) ⬆️
dispatch/mux.go 93.75% <ø> (ø)
...vider/currencyconverterapi/currencyconverterapi.go 0% <ø> (ø) ⬆️
currency/manager.go 100% <ø> (ø)
...rency/forexprovider/currencylayer/currencylayer.go 64.4% <ø> (-0.56%) ⬇️
database/models/sqlite3/boil_types.go 100% <ø> (ø)
engine/exchange.go 32.51% <ø> (ø)
currency/translation.go 100% <ø> (ø) ⬆️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fb4fdf...8c30505. Read the comment docs.

@thrasher- thrasher- mentioned this pull request Nov 25, 2019
Copy link
Collaborator

gloriousCode left a comment

🎉 I have been running and testing this branch for some time. For this PR, I'll test against the main bullet points of your description. There are a couple of issues regarding docker and the documentation tool. Here is a breakdown of test result

Feature to test Results Notes
gRPC client/server (see client and server functionality to manage the running bot instance Success Run both the client and server, recompiled grpc proto via gen_win.bat
Database support (Postgres and SQLite3). See database Success Created SQLite database, run migrations. Functions of dbmigrate tested
Several websocket improvements (performance, feature and currency pair handling) Success Orderbook works via GRPC stream + successful reconnection after disconnect. Orderbook handling does not fall behind
New asset package Success A config_example upscaling successfully splits and retrieves new asset types
Restructuring of project/code to make it more modular and structured Success Another difficult to test, however I approve of the restructuring
Currency pair syncer which syncs all enabled currency pairs and chooses REST or websocket depending on exchange support and whether websocket is enabled/disabled Success Tested this with unpopular currencies enabled. When updates are rare, syncer will call REST endpoint, reevaluate that websocket is still connected and switch back if there is an update: CoinbasePro LOOM-USDC: No orderbook update after 15 seconds, switching from websocket to rest
Several cmd utilities for checking exchange wrapper coverage, issues OTP generation database related tools (dbmigrate and gen_sqlboiler_config) and TLS cert generation for gRPC Mostly successful All cmd applications work without crashing with various configurations applied. Only issue was the comment for documentation and web installation
Exchange wrapper template updates Success Much improved and tested over time
Dispatcher service Success Streamed orderbook updates
Docker updates (support Go 1.13 plus the ability to build gctcli) Success Unsuccessful when using Windows Docker environment, but Linux environment works. However, there is an issue regarding 32 bit usage of nonces. See here: https://pastebin.com/8Yfy2nWu
Bash/ZSH auto completion N/A Not sure how to test this
Improved logger which supports optional levels (INFO/WARN/DEBUG/ERROR), output (console/file), file rotation and preserves your original advanced settings Success Logging to console/files works, rotation works, filtering works
Hundreds of bug fixes, QA improvements across the entire codebase Success go test ./… -race is all I can do to verify this one. We have all done many QA passes now
Increased test coverage for core packages (e.g config/exchange handling) N/A Relying on our online code coverage app for this
Orderbook simulation code and the ability to target specific pricing (aka whalebomb). However, this currently isn't linked up to the order manager. Success Sadly reveals that I do not have the capital to send be a whale
cmd/documentation/documentation.go Outdated Show resolved Hide resolved
cmd/config/config.go Show resolved Hide resolved
@thrasher- thrasher- mentioned this pull request Nov 27, 2019
9 of 9 tasks complete
Copy link
Member

xtda left a comment

The much anticipated engine merge into master!
image

As @gloriousCode stated have been using this regularly for the last few months

There are a few issues that already have PR's open to fix them (btcmarkets timestamp broken, logger disable not having any impact etc) so wont comment on those

Just one change for the zsh/bash autocomplete files to keep us linux users happy

contrib/zsh_autocomplete Show resolved Hide resolved
* Adds new file.Move func to address a bug with Golang/Docker volumes when using os.Rename

Also uses TempDir for tests instead of live directories and increases test coverage for file.Write

* Goimport the imports

* Make usage of file package name consistent so it no longer clashes with vars

* Remove outputFile if io.Copy fails
thrasher- and others added 2 commits Nov 28, 2019
Change CLRF -> LF
* Add directories to exclusion list && change default repo string

* Add in .idea folder to directory exclusion list

* cleaned

* Added support for calling the tool outside of its file

* fix formatting

* changed strings
Copy link
Collaborator

shazbert left a comment

Just some things I have found (out of scope). 💯
LGTM and tested.

SatoshisPerBTC = 100000000
SatoshisPerLTC = 100000000
WeiPerEther = 1000000000000000000
)

// GetV4UUID returns a RFC 4122 UUID based on random numbers

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator

Is there a reason to not call this package directly?

common/common.go Show resolved Hide resolved
@@ -423,7 +243,7 @@ func JSONEncode(v interface{}) ([]byte, error) {

// JSONDecode decodes JSON data into a structure
func JSONDecode(data []byte, to interface{}) error {

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator

Unnecessary call to the reflect package, a non memory address passed in will return an error. Would be good just to drop this and call the json package directly across the codebase to keep things consistant.

len(communications.IComm))
_, err := NewComm(&cfg)
if err == nil {
t.Error("NewComm should failed on no enabled communication mediums")

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator

NewComm should have failed on no enabled communication mediums

}
// No needs to check err here as the only err condition is an empty
// string which is already checked above
e.Requester.SetProxy(proxy)

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator

I think this might get flagged when we add in the linter option.
_ = e.Requester.SetProxy(proxy) might be better

engine/connection.go Show resolved Hide resolved
engine/connection.go Show resolved Hide resolved
engine/exchange_test.go Show resolved Hide resolved
var (
logger = &Logger{}
// FileLoggingConfiguredCorrectly flag set during config check if file logging meets requirements
FileLoggingConfiguredCorrectly bool
// GlobalLogConfig holds global configuration options for logger
GlobalLogConfig = &Config{}
// GlobalLogFile hold global configuration options for file logger
GlobalLogFile = &Rotate{}

eventPool = &sync.Pool{
New: func() interface{} {
return &LogEvent{
data: make([]byte, 0, 80),
}
},
}

// LogPath system path to store log files in
LogPath string
Comment on lines 76 to 94

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator

const -> vars -> then types


// Supported returns a list of supported asset types
func Supported() Items {
var a Items

This comment has been minimized.

Copy link
@shazbert

shazbert Nov 29, 2019

Collaborator
return Items{Spot,
		Margin,
		Index,
		Binary,
		PerpetualContract,
		PerpetualSwap,
		Futures,
		UpsideProfitContract,
		DownsideProfitContract,
	}
@shazbert

This comment has been minimized.

Copy link
Collaborator

shazbert commented Dec 2, 2019

refer to #390 for changes

thrasher- and others added 2 commits Dec 2, 2019
* Add 32bit build matrix test

* Adjust travis/Makefile

* Set dist back to xenial

* export CGO_ENABLED=1

* Add gcc-multilib

* Sudo installerino

* make check -> make test
* drop common uuid v4 func and imported package as needed

* removed common functions regarding json marshal and unmarshal and used the json package directly. WRT unmarshal it was calling reflect and converted to string which is also checked in the JSON package so it was doing a double up, this will be a tiny gain as it was directly used in the requester package for all our outbound requests.

* add in string

* explicitly throw away return error value

* atleast return the error that websocket initialise returns

* return error when not connected

* fix comment

* Adds comments

* move package declarations

* drop append whenever we call supported

* remove unused import

* Change incorrect spelling

* fix tests

* fix go import issue
Copy link
Collaborator

shazbert left a comment

tACK 🥇

Copy link
Collaborator

gloriousCode left a comment

With all of our testing and Shazbert's latest PR addressing his own nits, I'm happy to give this a tACK. I'm confident that this PR contains no showstopping issues and any minor issues can easily be addressed in future PRs. Thanks for making the other changes
... I mean... LGTM 😎

@xtda
xtda approved these changes Dec 3, 2019
Copy link
Member

xtda left a comment

tested latest changes from a clean master to engine config
zsh/bash autocompletes now also work as expected

All working great

tACK approved from me

thrasher- and others added 5 commits Dec 3, 2019
Saves having Windows users experience pain and suffering as documented
here:

golang/go#31870
* Broken WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

* Errors Fixed

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* PR Changes

* Offline Fees Fixed

* MarketCandles fixed and constants added

* t.log deleted

* Broken WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

WIP

* Errors Fixed

* PR Changes

* PR Changes

* PR Changes

* MarketCandles fixed and constants added

* t.log deleted

* Added BSB and Order Status

* WIP

* Websocket and BatchPlaceCancelOrder and other minor nits fixed

* Linter Issues Fixed

* Function Name Change

* Replacing b.GetMarkets with b.GetEnabledPairs

* Pagination param changes and PlaceCancelBatch changes

* Merge Conflicts WIP

* Linter Issue Fixed

* optional params fixed
* Basic concept commit

* Initial changes to support bitfinex v2. Reverts linter changes as they suck. Exports bitfinex ws types

* Adds ticker, trade and orderbook support

* Candles sub that returns no data COMPLETE

* Adds authenticated ws support

* Adds the barebones endpoints to support

* Adds more endpoints

* Even more endpoints

* minicommit to switch and test

* All the interactive types

* Adds support for simultaneous connections. Updates tests. Nothing is working

* Successfully adds place order. Moves all authenticated endpoints to new switch case

* Cancel order and modify order

* Cancel all orders, cancel multi orders

* Finalises implementation. Uses testMain

* Adds WS wrapper support for some funcs

* Fixing rebasing issues

* Replaces use of currency as a variable. Updates a lot of coinut websocket auth endpoint stuff

* Fixes some fun for loops with GetEnabledPairs

* Fixes tests impacted by currency var change

* Adds coinut support for WS functions. Replaces `order` vars with `ord`. Fixes some for loops too. Removes verbose from bitfinex

* So many panics

* I'm fixing a hole, where the panics get in, and stops my mind from wandering, where it will go

* Moves func `CanUseAuthenticatedWebsocketEndpoint` to Websocket package as it fits better. Adds test coverage of `CanUseAuthenticatedWebsocketEndpoint`

* Finishes up all of coinuts ws implementations.

* GateIO implementation

* Adds some helper funcs for types, sides and status. Adds support for huobi. Removes unnecessary type

* Adds forgotten huobi endpoint

* Fixes cancel order endpoint

* go hates my formatting and so do I

* The process to get authenticated kraken websocket to work. Uses testmain. Adds new auth channel, auth subscriptions, auth data handling. Not working yet

* Finishes open orders handling

* Mini update for status only updates

* Fixes some kraken points

* Finishes WS kraken since it doesn't work

* Unfinished commit, cleaning up types

* Finishes the const replacing

* Fixes extra GetNAmes after rebase

* An end to the cleanup. testmain for gateio

* Adds ZB support

* Bitfinex cleanup. Renamed func

* Testmain-47s for everyone!!! yayaaaaaaa

* Adds kraken websocket wrapper support

* Fixes rebase issues

* Fixes tests from rebase

* Adds test for conversion. Fixes for loop. Updates test order pricing. Fixes some poor made tests. Adds proper error handling for ws responses instead of logging them. Fixed issue where commented code ruined kraken ws.

* Fixes secret linting issues. Prioritises bitfinex channelID responses over authorised

* Fixes sloppy error/var declarations

* Fixes crazy bad logic where submit order errors weren't really considered. Parralols alphapoint/alphapoint_test.go. Removes buffer for multi-websocket comms channel.

* Removal of inline string and removal of redundant nil checkerinos

* Fixes err checks. Checks whether float has decimal. Fixes append. Drops omitempties. Parallel to some tests. Moves var declarations

* Replaces my lazy sprintfs with strconv.FormatInt(time.Now().Unix(), 10)

* Adds shiny new FullyMatched bool. Fixes coinbene buy sell consts

* Fixes oopsie with coinbene const replacement

* Fixes currency issue

* Cleans up new places that use JSONDecode

* Fixes huge panic bug from string int conversion. Adds large testtable for strings to order types

* Fixes some more strconversion issues. Fixes table test var usage. Changes mapperino name

* Added some new scenarios for number splitting

* Fixes lint issues

* negative num fix

* Typo fix

* Accuracy warning comment
* Engine pre merge changes

* Remove redundant "seconds"
* Magic strings

* Comment, format and builder fixes
@shazbert

This comment has been minimized.

Copy link
Collaborator

shazbert commented Dec 9, 2019

tACK again

@thrasher- thrasher- merged commit 473092a into master Dec 9, 2019
6 of 7 checks passed
6 of 7 checks passed
Review Action 0 issues found before hitting an error.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 83.31% of diff hit (target 43.49%)
Details
codecov/project Absolute coverage decreased by -2.09% but relative coverage increased by +39.81% compared to 0fb4fdf
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.