Skip to content

Conversation

@samuael
Copy link
Collaborator

@samuael samuael commented Jan 29, 2023

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 x in [] as the item is complete.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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.

  • go test ./... -race
  • golangci-lint run
  • Test X

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented on 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 Github Actions/AppVeyor with my changes
  • Any dependent changes have been merged and published in downstream modules

@shazbert shazbert requested a review from a team January 29, 2023 21:00
@shazbert shazbert added the review me This pull request is ready for review label Jan 29, 2023
Copy link
Collaborator

@shazbert shazbert left a 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.

Comment on lines 52 to 54
publicGetValuations = "public/get-valuations" // skipped
publicGetExpiredSettlementPrice = "public/get-expired-settlement-price" // skipped
publicGetInsurance = "public/get-insurance" // skipped
Copy link
Collaborator

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 {
Copy link
Collaborator

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 shazbert added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged review me This pull request is ready for review and removed review me This pull request is ready for review reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Jan 30, 2023
Copy link
Collaborator

@shazbert shazbert left a 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) {
Copy link
Collaborator

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) { }

@gloriousCode gloriousCode added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Feb 6, 2023
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Attention: Patch coverage is 33.89021% with 1662 lines in your changes missing coverage. Please review.

Project coverage is 36.85%. Comparing base (609011a) to head (6bc8907).

Files with missing lines Patch % Lines
exchanges/cryptodotcom/cryptodotcom_wrapper.go 30.91% 631 Missing and 55 partials ⚠️
exchanges/cryptodotcom/cryptodotcom.go 26.82% 461 Missing and 11 partials ⚠️
exchanges/cryptodotcom/cryptodotcom_websocket.go 47.57% 231 Missing and 39 partials ⚠️
...s/cryptodotcom/cryptodotcom_websocket_endpoints.go 0.00% 210 Missing ⚠️
exchanges/cryptodotcom/cryptodotcom_types.go 82.85% 8 Missing and 4 partials ⚠️
exchanges/cryptodotcom/cryptodotcom_convert.go 57.89% 8 Missing ⚠️
exchanges/order/orders.go 50.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1122      +/-   ##
==========================================
- Coverage   36.90%   36.85%   -0.05%     
==========================================
  Files         414      421       +7     
  Lines      180492   183006    +2514     
==========================================
+ Hits        66607    67449     +842     
- Misses     106046   107620    +1574     
- Partials     7839     7937      +98     
Files with missing lines Coverage Δ
engine/helpers.go 80.15% <100.00%> (+0.05%) ⬆️
exchanges/asset/asset.go 93.89% <100.00%> (+0.19%) ⬆️
exchanges/cryptodotcom/ratelimiter.go 100.00% <100.00%> (ø)
exchanges/support.go 100.00% <ø> (ø)
exchanges/order/orders.go 90.67% <50.00%> (-0.33%) ⬇️
exchanges/cryptodotcom/cryptodotcom_convert.go 57.89% <57.89%> (ø)
exchanges/cryptodotcom/cryptodotcom_types.go 82.85% <82.85%> (ø)
...s/cryptodotcom/cryptodotcom_websocket_endpoints.go 0.00% <0.00%> (ø)
exchanges/cryptodotcom/cryptodotcom_websocket.go 47.57% <47.57%> (ø)
exchanges/cryptodotcom/cryptodotcom.go 26.82% <26.82%> (ø)
... and 1 more

... and 12 files with indirect coverage changes

@shazbert shazbert added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Feb 7, 2023
Copy link
Collaborator

@shazbert shazbert left a 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Websocket isn't working.

Copy link
Collaborator

@shazbert shazbert left a 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.

@shazbert shazbert added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Feb 13, 2023
Copy link
Collaborator

@gloriousCode gloriousCode left a 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 @@
{
Copy link
Collaborator

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 {
Copy link
Collaborator

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{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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)
Copy link
Collaborator

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

Copy link
Collaborator

@shazbert shazbert left a 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asset.Spot -> assetType

Copy link
Collaborator Author

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

Copy link
Collaborator

@shazbert shazbert left a 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"}

Comment on lines 49 to 50
// instrumentOrderbookCnl,
// tickerCnl,
Copy link
Collaborator

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))
Copy link
Collaborator

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

Copy link
Collaborator

@shazbert shazbert left a 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 {
Copy link
Collaborator

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,
Copy link
Collaborator

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)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@samuael
Copy link
Collaborator Author

samuael commented Mar 28, 2023

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

dependabot bot and others added 12 commits September 2, 2025 17:26
…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
@thrasher-
Copy link
Collaborator

@codex please review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

Comment on lines 246 to 256
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)

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 👍 / 👎.

Comment on lines +221 to +224
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()

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 👍 / 👎.

Comment on lines +680 to +685
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()

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 👍 / 👎.

Comment on lines +1082 to +1086
var fee float64
switch feeBuilder.FeeType {
case exchange.CryptocurrencyTradeFee:
fee = calculateTradingFee(feeBuilder) * feeBuilder.Amount * feeBuilder.PurchasePrice
case exchange.CryptocurrencyWithdrawalFee:

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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review me This pull request is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants