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

exchanges: Add Coinbase International support #1381

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

samuael
Copy link
Contributor

@samuael samuael commented Oct 28, 2023

ሰላም ወንድሞች፣

PR Description

This pull request Included a code implementation for the Coinbase International exchange version 1; with REST and web socket support. All the Websocket and Wrapper functions are tested. My code follows the style guidelines of other exchanges of GCT. Fixed all linter issues errors with the help of golangci-lint. Endpoint methods that have no support from the GCT wrapper are also implemented for future use.

Fixes # (issue)

Type of change

  • 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

  • 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

@gloriousCode gloriousCode requested a review from a team October 29, 2023 21:38
@gloriousCode gloriousCode added the review me This pull request is ready for review label Oct 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.

Great work, just had a quick look over code. 👍

Comment on lines 40 to 43
errAddressIsRequired = errors.New("err: missing address")
errAssetIdentifierRequired = errors.New("err: asset identified is required")
errEmptyArgument = errors.New("err: empty argument")
errTimeInForceRequired = errors.New("err: time_in_force is required")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably RM err: string

if orderID == "" {
return nil, order.ErrOrderIDNotSet
}
if arg == (&ModifyOrderParam{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs a nil check

if arg == nil || *arg == (ModifyOrderParam{}) {

params.Set("status", status)
}
if transferType != "" {
params.Set("type", status)
Copy link
Collaborator

Choose a reason for hiding this comment

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

status -> transferType

Comment on lines 453 to 462
value := reflect.ValueOf(data)
var payload []byte
if value != (reflect.Value{}) && !value.IsNil() && value.Kind() != reflect.Ptr {
return errArgumentMustBeInterface
} else if value != (reflect.Value{}) && !value.IsNil() {
payload, err = json.Marshal(data)
if err != nil {
return err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a lot of stuff going on here. I think we could just go about doing a if data != nil {} check.

Or if the pointer value is really needed:

	var payload []byte
	if data != nil {
		if reflectlite.ValueOf(data).Kind() != reflectlite.Ptr {
			return errArgumentMustBePointer
		}
		payload, err = json.Marshal(data)
		if err != nil {
			return err
		}
	}

QuoteAssetUUID string `json:"quote_asset_uuid"`
QuoteAssetName string `json:"quote_asset_name"`
BaseIncrement string `json:"base_increment"`
QuoteIncrement string `json:"quote_increment"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you go over these string values across this file and convert them to float64 values? I will highlight a few if they come up.


// GetRecentTrades returns the most recent trades for a currency and asset
func (co *CoinbaseInternational) GetRecentTrades(_ context.Context, _ currency.Pair, _ asset.Item) ([]trade.Data, error) {
return nil, 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.

Not supported? and below?

if err != nil {
return nil, err
}
return &order.SubmitResponse{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: can use method on S to get default order.SubmitResponse if you want.

Comment on lines 574 to 577
if err != nil {
return err
}
return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

return err


// CancelBatchOrders cancels orders by their corresponding ID numbers
func (co *CoinbaseInternational) CancelBatchOrders(context.Context, []order.Cancel) (*order.CancelBatchResponse, error) {
return nil, 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.

👀

// WithdrawFiatFunds returns a withdrawal ID when a withdrawal is
// submitted
func (co *CoinbaseInternational) WithdrawFiatFunds(context.Context, *withdraw.Request) (*withdraw.ExchangeResponse, error) {
return nil, 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.

I will stop mentioning this but common.ErrNotSupported

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 26.20408% with 904 lines in your changes are missing coverage. Please review.

Project coverage is 46.32%. Comparing base (d679a76) to head (b62bb26).
Report is 4 commits behind head on master.

❗ Current head b62bb26 differs from pull request most recent head b198487. Consider uploading reports for the commit b198487 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1381       +/-   ##
===========================================
+ Coverage   35.89%   46.32%   +10.42%     
===========================================
  Files         411      366       -45     
  Lines      177595   120470    -57125     
===========================================
- Hits        63752    55806     -7946     
+ Misses     106058    56996    -49062     
+ Partials     7785     7668      -117     
Files Coverage Δ
engine/helpers.go 82.29% <100.00%> (+2.18%) ⬆️
exchanges/support.go 100.00% <ø> (ø)
...seinternational/coinbaseinternational_websocket.go 23.82% <23.82%> (ø)
...ges/coinbaseinternational/coinbaseinternational.go 18.47% <18.47%> (ø)
...baseinternational/coinbaseinternational_wrapper.go 32.31% <32.31%> (ø)

... and 393 files with indirect coverage changes

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.

None yet

3 participants