-
Notifications
You must be signed in to change notification settings - Fork 822
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: Okx Update #1420
base: master
Are you sure you want to change the base?
exchanges: Okx Update #1420
Conversation
… into okx_update
… into okx_update
… into okx_update
… into okx_update
… into okx_update
… into okx_update
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.
Second batch of feedback from me.
I'm stopping here for today on this review, so giving you what I have so far.
Great work. Action or Ignore anything marked Nits, Up-To-You 😄
exchanges/futures/contract_test.go
Outdated
assert.ErrorIs(t, err, contractSettlementTypesMap[x].Error) | ||
for x, v := range contractSettlementTypesMap { | ||
val, err := StringToContractSettlementType(x) | ||
assert.Equal(t, val, v.CT, "got %v, expected %v", val, v.CT) |
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.
Should definitely be remove the string.
The purpose of assert is to do the comparrison and display it.
The purpose of any message is to explain the test.
In this case, no explanation is required.
exchanges/futures/contract_test.go
Outdated
assert.ErrorIs(t, err, contractSettlementTypesMap[x].Error) | ||
for x, v := range contractSettlementTypesMap { | ||
val, err := StringToContractSettlementType(x) | ||
assert.Equal(t, val, v.CT, "got %v, expected %v", val, v.CT) |
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.
Also, strings are the wrong way around:
assert.Equal(t, val, v.CT, "got %v, expected %v", val, v.CT) | |
assert.Equal(t, v,CT, val) |
exchanges/futures/contract_test.go
Outdated
ContractType(200): "unset", | ||
} | ||
for k := range contractTypeToStringMap { | ||
assert.Equal(t, k.String(), contractTypeToStringMap[k]) |
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.
Also switch params:
assert.Equal(t, k.String(), contractTypeToStringMap[k]) | |
assert.Equal(t, v, k.String()) |
exchanges/margin/margin_test.go
Outdated
t.Errorf("received '%v' expected 'isolated'", alien.M) | ||
} | ||
assert.NoError(t, err) | ||
assert.Equalf(t, Isolated, alien.M, "received '%v' expected '%v'", alien.M, Isolated) |
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.
Use .String()
on the values you're comparing in this case 😄
Definitely don't want to Equalf like that, IMO.
Applies to all the examples below.
exchanges/okx/okx.go
Outdated
errNoInstrumentFound = errors.New("no instrument found") | ||
baseURL = "https://www.okx.com/" | ||
apiURL = baseURL + apiPath | ||
apiVersion = "/v5/" |
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.
Nit:
apiVersion = "/v5/" | |
versionPath = "/v5/" |
exchanges/okx/okx.go
Outdated
tradeSpot = "trade-spot/" | ||
tradeMargin = "trade-margin/" | ||
tradeFutures = "trade-futures/" | ||
tradePerps = "trade-swap/" | ||
tradeOptions = "trade-option/" |
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.
Nit: tradeSpotPath etc
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.
Less Nit, but still optional:
Though actually, having these here made them feel important and used across multiplle exchanges.
But they're actually only used in GetCurrencyTradeURL
.
Compare trade/order*
and other const literals which are hardcoded throughout this file in all the Get*
func calls and others.
So I think these ☝️ should go as hardcoded literals in GetCurrencyTradeURL
since they're literally just used once
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.
On recent changes, thank you kindly sir 🕊️ 👑
exchanges/margin/margin_types.go
Outdated
@@ -74,20 +74,20 @@ const ( | |||
// Unset is the default value | |||
Unset = Type(0) | |||
// Isolated means a margin trade is isolated from other margin trades | |||
Isolated Type = 1 << (iota - 1) | |||
Isolated Type = 1 << iota |
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.
Better to revert this, as this skips the first bit 00000010
and you only have 7 entries if you want to add more.
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.
Nit:
Would read slightly saner if we move comments to line ends and use the same type declaration.
Then values and comments would line up nicely.
const (
Unset Type = 0
Isolated = 1 << (iota - 1) // Margin is isolated from other trades
Multi // Margin is shared amongst trades and assets; Cross-Margin
NoMargin // No Margin; Trade is unleveraged
SpotIsolated // Margin shared amongst all spot trades but isolated from other assets
)
exchanges/margin/margin.go
Outdated
@@ -8,7 +8,7 @@ import ( | |||
|
|||
// Valid returns whether the margin type is valid | |||
func (t Type) Valid() bool { | |||
return t != Unset && supported&t == t | |||
return t != Unset && (t&(t-1) == 0) && supported&t == t |
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.
(t&(t-1) == 0)
is fine but it blocks any future combinations and will need to remove if we introduce that for any reason.
exchanges/okx/helpers.go
Outdated
case "C", "P", "c", "p": | ||
enabled, err := ok.IsPairEnabled(pair, asset.Options) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if enabled { | ||
return []asset.Item{asset.Options}, nil | ||
} | ||
default: | ||
enabled, err := ok.IsPairEnabled(pair, asset.Futures) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if enabled { | ||
return []asset.Item{asset.Futures}, nil | ||
} | ||
} |
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.
Missing coverage
exchanges/okx/okx_wrapper.go
Outdated
@@ -1126,7 +1126,7 @@ func priceTypeString(pt order.PriceType) string { | |||
|
|||
func (ok *Okx) marginTypeToString(m margin.Type) string { | |||
switch m { | |||
case margin.Isolated, margin.Cash, margin.SpotIsolated: | |||
case margin.Isolated, margin.NoMargin, margin.SpotIsolated: |
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.
suggestion: bitwise op can be done here, if ya want. Leave that up to you. 🤷
I'm seeing this error on running with spread enabled: Cause: If assetEnabled is true but enabled and available is empty then the |
exchanges/margin/margin_types.go
Outdated
@@ -74,20 +74,20 @@ const ( | |||
// Unset is the default value | |||
Unset = Type(0) | |||
// Isolated means a margin trade is isolated from other margin trades | |||
Isolated Type = 1 << (iota - 1) | |||
Isolated Type = 1 << iota |
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.
Nit:
Would read slightly saner if we move comments to line ends and use the same type declaration.
Then values and comments would line up nicely.
const (
Unset Type = 0
Isolated = 1 << (iota - 1) // Margin is isolated from other trades
Multi // Margin is shared amongst trades and assets; Cross-Margin
NoMargin // No Margin; Trade is unleveraged
SpotIsolated // Margin shared amongst all spot trades but isolated from other assets
)
exchanges/okx/helpers.go
Outdated
} | ||
arg.OrderType = strings.ToLower(arg.OrderType) | ||
if !slices.Contains([]string{orderMarket, orderLimit, orderPostOnly, orderFOK, orderIOC, orderOptimalLimitIOC, "mmp", "mmp_and_post_only"}, arg.OrderType) { | ||
return fmt.Errorf("%w %v", order.ErrTypeIsInvalid, arg.OrderType) |
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.
return fmt.Errorf("%w %v", order.ErrTypeIsInvalid, arg.OrderType) | |
return fmt.Errorf("%w: %v", order.ErrTypeIsInvalid, arg.OrderType) |
Or
return fmt.Errorf("%w %v", order.ErrTypeIsInvalid, arg.OrderType) | |
return fmt.Errorf("%w: `%v`", order.ErrTypeIsInvalid, arg.OrderType) |
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.
Nit: Inconsistency on what you've added here, mixing backticks and apostrophes 🙂
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, just on recent changes.
exchanges/gateio/gateio_types.go
Outdated
@@ -1301,7 +1301,7 @@ type CrossMarginBalance struct { | |||
BorrowedNet string `json:"borrowed_net"` | |||
TotalNetAssetInUSDT string `json:"net"` | |||
PositionLeverage string `json:"leverage"` | |||
Risk string `json:"risk"` // Risk rate. When it falls below 110%, liquidation will be triggered. Calculation formula: total / (borrowed+interest) | |||
Risk string `json:"risk"` // Risk percentage; Liquidation is triggered when this below required margin Calculation: total / (borrowed+interest) |
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.
Risk string `json:"risk"` // Risk percentage; Liquidation is triggered when this below required margin Calculation: total / (borrowed+interest) | |
Risk string `json:"risk"` // Risk percentage; Liquidation is triggered when this falls below required margin. Calculation: total / (borrowed+interest) |
exchanges/okx/okx_test.go
Outdated
OrderType: "limit", | ||
Amount: 2.6, | ||
Price: 2.1, | ||
Price: 2000000.1, |
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.
revert please
exchanges/okx/okx_wrapper.go
Outdated
@@ -1125,14 +1125,12 @@ func priceTypeString(pt order.PriceType) string { | |||
} | |||
|
|||
func (ok *Okx) marginTypeToString(m margin.Type) string { | |||
switch m { | |||
case margin.Isolated, margin.NoMargin, margin.SpotIsolated: | |||
if (margin.Isolated|margin.NoMargin|margin.SpotIsolated)&m == m { |
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.
(margin.Isolated|margin.NoMargin|margin.SpotIsolated) can be a global
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 this one pedantic change and nothing more from me.
exchanges/okx/okx_wrapper.go
Outdated
func (ok *Okx) marginTypeToString(m margin.Type) string { | ||
if (margin.Isolated|margin.NoMargin|margin.SpotIsolated)&m == m { | ||
if (allowedMarginTypes)&m == m { |
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.
if (allowedMarginTypes)&m == m { | |
if allowedMarginTypes&m == m { |
If one asset is enabled but has no enabled pairs, we should ignore that asset, even for non-pair related subscriptions. That matches the existing code, but wasn't happening in the context of asset.All subscriptions with just one asset in this state. If a user wanted to have non-pair subscriptions, they should use asset.Empty. Would expect other breakages with no pairs enabled, too. Note: No enabled pairs for an enabled asset is a transient issue which can occur due to enableOnePair racing against no available pairs. The second run, available pairs would be populated and enableOnePair would work. This situation could persist due to running with --dry or containerised, though. Fixes: ` Okx websocket: subscription failure, myOrders all : subscription template did not generate the expected number of pair records for spread: Got 1; Expected 0 ` Relates to thrasher-corp#1420
exchanges/okx/okx.go
Outdated
return nil, common.ErrEmptyParams | ||
} | ||
if arg.OrderType != "conditional" { | ||
return nil, fmt.Errorf("%w: order type value 'chase' is only supported for chase orders", order.ErrTypeIsInvalid) |
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 think this error is incorrect; Should be talking about
conditional
- Should contain the actual order type that's erroring
- Not convinced we need to be as verbose as saying what they have to use
return nil, fmt.Errorf("%w: order type value 'chase' is only supported for chase orders", order.ErrTypeIsInvalid) | |
return nil, fmt.Errorf("%w for TPSL: `%s`, order.ErrTypeIsInvalid, arg.OrderType) |
And then the same in Chase and TWAP.
Question: If we have fixed required OrderTypes for these funcs ... why not default the value if not present?
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.
It was just to not edit the value set by the user. I believe it is better to correct them with the error message than to edit the value of the OrderType; That is why I haven't set the OrderType value from inside the function.
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.
Yeah, I wasn't suggesting to edit the value, just to default it if it's not provided.
Entirely optional though. I'll resolve and you can do it if you want.
exchanges/okx/okx.go
Outdated
return ok.PlaceAlgoOrder(ctx, arg) | ||
} | ||
|
||
// TriggerAlgoOrder fetches algo trigger orders for SWAP market types |
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.
Should be PlaceTriggerAlgoOrder, shouldn't it?
exchanges/okx/okx.go
Outdated
} | ||
if arg.OrderType != "trigger" { | ||
return nil, errInvalidOrderType | ||
return nil, order.ErrTypeIsInvalid |
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.
Should be consistent with the other errors above
exchanges/okx/okx.go
Outdated
return nil, common.ErrEmptyParams | ||
} | ||
if args[x].AlgoOrderID == "" { | ||
return nil, fmt.Errorf("%w, algoId is required", order.ErrOrderIDNotSet) |
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.
Should not use json fields in reply to a go user.
return nil, fmt.Errorf("%w, algoId is required", order.ErrOrderIDNotSet) | |
return nil, fmt.Errorf("%w, AlgoOrderId is required", order.ErrOrderIDNotSet) |
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.
Same applies to all other error returns in this file :)
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 comment affects many lines I think
exchanges/okx/okx.go
Outdated
err := ok.SendHTTPRequest(ctx, exchange.RestSpot, rateLimit, http.MethodPost, route, &args, &resp, request.AuthenticatedRequest) | ||
if err != nil { | ||
if resp != nil && resp.StatusMessage != "" { | ||
return nil, fmt.Errorf("%w, SCode: %s, SMsg: %s", err, resp.StatusCode, resp.StatusMessage) |
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.
Suggest the terms SCode and SMessage are irrelevant to the user, so switch to legible:
return nil, fmt.Errorf("%w, SCode: %s, SMsg: %s", err, resp.StatusCode, resp.StatusMessage) | |
return nil, fmt.Errorf("%w, Code: %s, Message: %s", err, resp.StatusCode, resp.StatusMessage) |
exchanges/okx/okx.go
Outdated
|
||
// MassCancelOrder cancel all the MMP pending orders of an instrument family. | ||
// Only applicable to Option in Portfolio Margin mode, and MMP privilege is required | ||
func (ok *Okx) MassCancelOrder(ctx context.Context, instrumentType, instrumentFamily string, lockInterval int64) (*CancelMMPResponse, 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.
Suggest we need to hide their misleading naming.
func (ok *Okx) MassCancelOrder(ctx context.Context, instrumentType, instrumentFamily string, lockInterval int64) (*CancelMMPResponse, error) { | |
func (ok *Okx) CancelAllMMPOrders(ctx context.Context, instrumentType, instrumentFamily string, lockInterval int64) (*CancelMMPResponse, error) { |
especially since you've done that in the next func
exchanges/okx/okx.go
Outdated
// CancelAllMMPOrdersAfterCountdown cancel all MMP pending orders after the countdown timeout. | ||
// Only applicable to Option in Portfolio Margin mode, and MMP privilege is required | ||
func (ok *Okx) CancelAllMMPOrdersAfterCountdown(ctx context.Context, timeout int64, orderTag string) (*CancelResponse, 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.
Documentation doesn't seem to agree that this is an MMP endpoint
So I think this is CancelAllDelayed maybe. Or CancelAllAfter as-per-endpoint even.
exchanges/okx/okx.go
Outdated
return resp, ok.SendHTTPRequest(ctx, exchange.RestSpot, getTradeAccountRateLimitEPL, http.MethodGet, "trade/account-rate-limit", nil, &resp, request.AuthenticatedRequest) | ||
} | ||
|
||
// OrderPreCheck precheck the account information before and after placing the order. |
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.
- precheck => prechecks
- But actually outside the context of func naming, this is just "checks" or "returns"
- Switched "the order" to "a potential" to make it clear it's not actually placing the order
- I'd also suggest that to match
Place*
funcs, we name thisPreCheckOrder
.
// OrderPreCheck precheck the account information before and after placing the order. | |
// OrderPreCheck returns the account information before and after placing a potential order |
exchanges/okx/okx.go
Outdated
func (ok *Okx) CancelRfq(ctx context.Context, arg CancelRfqRequestParam) (*CancelRfqResponse, error) { | ||
if arg.RfqID == "" && arg.ClientRfqID == "" { | ||
return nil, errMissingRfqIDAndClientRfqID | ||
// CancelRFQ Cancel an existing active RFQ that you has previously created |
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.
has
=>have
- Implied terms: existing, active and previously created, so we can remove them all
// CancelRFQ Cancel an existing active RFQ that you has previously created | |
// CancelRFQ cancels an Request for quotation |
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.
Minor thing I noticed on recent changes
exchanges/okx/okx.go
Outdated
@@ -836,7 +836,7 @@ func (ok *Okx) CreateRFQ(ctx context.Context, arg *CreateRFQInput) (*RFQResponse | |||
return resp, ok.SendHTTPRequest(ctx, exchange.RestSpot, createRFQEPL, http.MethodPost, "rfq/create-rfq", &arg, &resp, request.AuthenticatedRequest) | |||
} | |||
|
|||
// CancelRFQ Cancel an existing active RFQ that you has previously created | |||
// CancelRFQ cancels an Request for quotation |
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.
// CancelRFQ cancels an Request for quotation | |
// CancelRFQ cancels a request for quotation |
arg.PositionSide != positionSideNet { | ||
return nil, errors.New("missing valid position side") | ||
if !slices.Contains([]string{positionSideLong, positionSideShort, positionSideNet}, arg.PositionSide) { | ||
return nil, fmt.Errorf("%w, position side is required", order.ErrSideIsInvalid) |
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.
Misleading message, because we possible have a side, it just isn't what you want.
Need to confirm the side that's wrong.
return nil, fmt.Errorf("%w, position side is required", order.ErrSideIsInvalid) | |
return nil, fmt.Errorf("%w: `%s`", order.ErrSideIsInvalid, arg.PositionSide) |
exchanges/okx/helpers.go
Outdated
return []asset.Item{aType}, nil | ||
} | ||
} | ||
return nil, fmt.Errorf("%w: no asset enabled with instrument ID `%v`", asset.ErrNotSupported, instrumentID) |
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.
return nil, fmt.Errorf("%w: no asset enabled with instrument ID `%v`", asset.ErrNotSupported, instrumentID) | |
return nil, fmt.Errorf("%w: no asset enabled with instrument ID `%v`", asset.ErrNotEnabled, instrumentID) |
Selam, ወንድሞች.
PR Description
This pull request incorporates modifications for the Okx exchange, encompassing changes to endpoint methods, rate limits, WebSocket subscriptions and handlers, wrapper functions, and conversion methods. Rigorous testing has been conducted on all facets of the functions to ensure their reliability. The code adheres to the GCT style standards, consistent with practices adopted by other exchanges. To enhance code quality, golangci-lint was utilized for linter testing, effectively rectifying a few errors. Furthermore, this implementation includes endpoint methods that the GCT wrapper did not initially support, ensuring compatibility for future use
The PR updated implementations for REST for trading and other endpoint methods, Web-socket streaming, and Trading through the web socket stream.
Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
x
in[]
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