-
Notifications
You must be signed in to change notification settings - Fork 897
exchanges: Add V2 Crypto.com exchange support #1122
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
base: master
Are you sure you want to change the base?
Conversation
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a basic look over. Well done.
| publicGetValuations = "public/get-valuations" // skipped | ||
| publicGetExpiredSettlementPrice = "public/get-expired-settlement-price" // skipped | ||
| publicGetInsurance = "public/get-insurance" // skipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why skipped?
| return nil, currency.ErrCurrencyPairEmpty | ||
| } | ||
| var notional float64 | ||
| switch s.Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Test for when both s.Amount and notional are sent.
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that, just a quick review of last changes. Once completed I will start doing end point tests.
| TriggerPrice float64 `json:"trigger_price"` | ||
| } | ||
|
|
||
| func (arg *CreateOrderParam) getCreateParamMap() (map[string]interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs full test coverage and utilizing if !errors.Is(err, errGlobalError) { }
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing the requested will go over authenticated endpoints once websocket is up and running.
| var responseStream chan SubscriptionRawData | ||
|
|
||
| // WsConnect creates a new websocket to public and private endpoints. | ||
| func (cr *Cryptodotcom) WsConnect() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Websocket isn't working.
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some pending things on recent changes before I do function testing.
gloriousCode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good! I'm in the process of getting API access, so just a quick overall code quality check first
| @@ -1,5 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only be adding the new exchange here, not the other changes you've made
| var responseStream chan SubscriptionRawData | ||
|
|
||
| // WsConnect creates a new websocket to public and private endpoints. | ||
| func (cr *Cryptodotcom) WsConnect() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I have highlighted in other reviews, just running the application with the exchange enabled is an excellent way of highlighting issues within your implementation and definitely should be part of how you test a given exchange PR.
- [ERROR] | WEBSOCKET | 20/02/2023 10:49:42 | exchange Cryptodotcom websocket error - can not pass push data message with signature 106116263 with method subscribe
|
|
||
| // GenerateDefaultSubscriptions Adds default subscriptions to websocket to be handled by ManageSubscriptions() | ||
| func (cr *Cryptodotcom) GenerateDefaultSubscriptions() ([]stream.ChannelSubscription, error) { | ||
| subscriptions := []stream.ChannelSubscription{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| subscriptions := []stream.ChannelSubscription{} | |
| var subscriptions []stream.ChannelSubscription |
The reason is because if there are any errors, we're not allocating memory space. Just declaring it as a var is an empty declaration. Its minor, but its a good practice to continue doing in all PRs
|
|
||
| // GetFeeByType returns an estimate of fee based on the type of transaction | ||
| func (cr *Cryptodotcom) GetFeeByType(ctx context.Context, feeBuilder *exchange.FeeBuilder) (float64, error) { | ||
| return 0, common.ErrNotYetImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement
| params.Set("depth", strconv.FormatInt(depth, 10)) | ||
| } | ||
| var resp *OrderbookDetail | ||
| return resp, cr.SendHTTPRequest(ctx, exchange.RestSpot, common.EncodeURLValues(publicOrderbook, params), request.Unset, &resp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not setting the ratelimits. I understand that this exchange has different style of ratelimits, but it seems you have defined them all but don't use them
d93f24a to
a149dd9
Compare
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes, this has a data race in websocket.
| return nil, fmt.Errorf("asset type of %s is not supported by %s", assetType, cr.Name) | ||
| return nil, fmt.Errorf("%w asset type: %v", asset.ErrNotSupported, assetType) | ||
| } | ||
| p, err := cr.FormatExchangeCurrency(p, asset.Spot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asset.Spot -> assetType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since crypto.com only use Spot, there maybe some places where I used the terms interchangeably. But, I will fix that
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of error messages that can be seen when running the application:
[ERROR] | WEBSOCKET | 17/03/2023 15:00:06 | exchange Cryptodotcom websocket error - could not match incoming message with signature: 146884893, and data: {"id":146884893,"code":0,"method":"subscribe","channel":"book.RSR_USDT"}
| // instrumentOrderbookCnl, | ||
| // tickerCnl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncomment
| if err != nil { | ||
| return err | ||
| } | ||
| tickersDatas := make([]ticker.Price, len(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do anything because websocketroutine_manager.go has no way of handling []ticker.Price so the data goes nowhere, forcing a REST switchover.
You'll need to update websocketroutine_manager.go to support it in the function websocketDataHandler
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks some issues I have found. I will be opening up a PR to circumvent an edge case with respect to the GetHistoricCandles function shortly.
| } | ||
|
|
||
| // ValidateCredentials validates current credentials used for wrapper | ||
| func (cr *Cryptodotcom) ValidateCredentials(ctx context.Context, assetType asset.Item) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being used
| kline.TwoWeek, | ||
| kline.OneMonth, | ||
| ), | ||
| ResultLimit: 200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
300
| if err != nil { | ||
| return err | ||
| } | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You pause everything up >= 20 seconds. Rate limiting is done within SendJSONMesage function. This was hard to find, running the application would have shown there was an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that because it is advised to have a 1s interval and was not sure of the presence of a Rate limiter before sending the next JSON message.
I run the application but I didn't see an issue related to that. But, for now, I removed it
|
Thanks for your PR. And, I was also wondering whether to have such a variable to distinguish whether asset Items( instruments of asset items) have candlestick data necessarily. Since some instruments may not have candlestick data even with in an extended time range |
…hrasher-corp#2013) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.10.0 to 1.11.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.10.0...v1.11.0) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-version: 1.11.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-corp#2017) Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.27.1 to 2.27.2. - [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases) - [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/main/.goreleaser.yml) - [Commits](grpc-ecosystem/grpc-gateway@v2.27.1...v2.27.2) --- updated-dependencies: - dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2 dependency-version: 2.27.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…thrasher-corp#2015) Bumps [github.com/mattn/go-sqlite3](https://github.com/mattn/go-sqlite3) from 1.14.30 to 1.14.32. - [Release notes](https://github.com/mattn/go-sqlite3/releases) - [Commits](mattn/go-sqlite3@v1.14.30...v1.14.32) --- updated-dependencies: - dependency-name: github.com/mattn/go-sqlite3 dependency-version: 1.14.32 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…hrasher-corp#2014) Bumps google.golang.org/protobuf from 1.36.7 to 1.36.8. --- updated-dependencies: - dependency-name: google.golang.org/protobuf dependency-version: 1.36.8 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ps (thrasher-corp#1968) * set limiter to first level mock data list and updated unit tests * address nested slices length limit * minor fix recording file and update unit tests * minor updates on unit tests * re-record mock files and minor fix on the unit tests ti adapt the mock data change * improve http recording limit value and fix issues with mock data in binance * added MockDataSliceLimit in request items and resolve minor unit test issues * resolve missed conflict * rename mock variables, resolve unit test issues, and other updates * minor fix to CheckJSON and update unit tests * minor unit test fix * further optimization on mock CheckJSON method, unit tests, and re-record poloniex * common and recording unit tests fix * minor linter issues fix * unit tests format fix * fix miscellaneous error * unit tests fix and minor docs update * re-record and reduce mock file size * indentation fix * minor assertion test fix * reverted log.Printf line in live testing * rename variables * update NewVCRServer unit test * replace string comparison with *net.OpError check * restructur net error test * exchanges/mock: Remove redundant error assertion message in TestNewVCRServer --------- Co-authored-by: Adrian Gallagher <[email protected]>
…anges (thrasher-corp#1860) * move limits, transition to key gen * rollout NewExchangePairAssetKey everywhere * test improvements * self-review fixes * ok, lets go * fix merge issue * slower value func,assertify,drop IsValidPairString * remove binance reference for backtesting test * Redundant nil checks removed due to redundancy * Update order_test.go * Move limits back into /exchanges/ * puts limits in a different box again * SHAZBERT SPECIAL SUGGESTIONS * Update gateio_wrapper.go * fixes all build issues * Many niteroos! * something has gone awry * bugfix * gk's everywhere nits * lint * extra lint * re-remove IsValidPairString * lint fix * standardise test * revert some bads * dupe rm * another revert 360 mcgee * un-in-revertify * Update exchange/order/limits/levels_test.go Co-authored-by: Adrian Gallagher <[email protected]> * fix * Update exchanges/binance/binance_test.go HERE'S HOPING GITHUB FORMATS THIS CORRECTLY! Co-authored-by: Gareth Kirwan <[email protected]> * update text * rn func, same line err gk4202000 --------- Co-authored-by: Adrian Gallagher <[email protected]> Co-authored-by: Gareth Kirwan <[email protected]>
* linters: Add modernise tool check and fix issues * engine: Simplify exch.SetDefaults call and remove localWG * CI: Revert config versions lint workflow
…p#2020) * devcontainer: Update Dockerfile and configuration for GoCryptoTrader setup * devcontainer: Update comment for clarity on format and config details * devcontainer: Refactor post-create command to use dedicated script for tool installation * devcontainer: Update comment for Go modernise tool for clarity
Co-authored-by: mathieu cesbron <[email protected]>
|
@codex please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| func (e *Exchange) UpdateTradablePairs(ctx context.Context, forceUpdate bool) error { | ||
| assetTypes := e.GetAssetTypes(true) | ||
| for _, assetType := range assetTypes { | ||
| pairs, err := e.FetchTradablePairs(ctx, assetType) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if assetType == asset.OTC && !e.IsRESTAuthenticationSupported() { | ||
| continue | ||
| } | ||
| return e.UpdatePairs(pairs, assetType, false, forceUpdate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Update tradable pairs for all assets
The loop in UpdateTradablePairs returns on the first iteration, so only the first asset type retrieved from GetAssetTypes(true) ever has its pairs updated. Subsequent asset types (e.g. PerpetualSwap) are skipped entirely, leaving their pairs stale and preventing features that rely on those assets from working. The function should continue iterating and only return after all assets are processed.
Useful? React with 👍 / 👎.
| func (e *Exchange) GetDepositHistory(ctx context.Context, ccy currency.Code, startTimestamp, endTimestamp time.Time, pageSize, page, status int64) (*DepositResponse, error) { | ||
| params := make(map[string]any) | ||
| if ccy.IsEmpty() { | ||
| params["currency"] = ccy.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Currency filter inverted in deposit history request
GetDepositHistory only attaches the currency parameter when ccy.IsEmpty() is true. This means a caller requesting a specific currency never includes that currency in the API request, while an empty currency sends an empty string. As a result, filtered deposit history cannot be retrieved. The condition should be if !ccy.IsEmpty() before setting the parameter.
Useful? React with 👍 / 👎.
| params := make(map[string]any) | ||
| if !currencyPair.Base.IsEmpty() { | ||
| params["base_currency"] = currencyPair.Base.String() | ||
| } | ||
| if !currencyPair.Base.IsEmpty() { | ||
| params["quote_currency"] = currencyPair.Quote.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Quote currency omitted when only quote provided
In GetOTCTradeHistory, the code checks !currencyPair.Base.IsEmpty() for both the base and quote currency parameters. If the caller supplies only a quote currency, the quote parameter is never sent, causing the request to ignore the intended filter. The second condition should reference currencyPair.Quote instead of the base again.
Useful? React with 👍 / 👎.
| var fee float64 | ||
| switch feeBuilder.FeeType { | ||
| case exchange.CryptocurrencyTradeFee: | ||
| fee = calculateTradingFee(feeBuilder) * feeBuilder.Amount * feeBuilder.PurchasePrice | ||
| case exchange.CryptocurrencyWithdrawalFee: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Trade fee multiplied twice
For CryptocurrencyTradeFee, calculateTradingFee already returns price * amount * rate, but the caller multiplies the result by feeBuilder.Amount * feeBuilder.PurchasePrice again. This squares the trade notional, drastically inflating reported fees. Assign the result directly instead of multiplying again to return the actual fee amount.
Useful? React with 👍 / 👎.
PR Description
This pull request adds a code implementation for version 2 of the Crypto.com exchange. The implementation supports REST and web-socket functionalities. All the REST, WebSocket endpoints, and wrapper functions are tested. My code adheres to the GCT style standards used by other exchanges.
Fixed some errors with the help of linter(golangci-lint), and unit tests. REST and WebSocket endpoint methods that have no support from the GCT wrapper are also implemented for future use.
Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
xin[]as the item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also, consider improving test coverage whilst working on a certain feature or package.
Checklist