-
Notifications
You must be signed in to change notification settings - Fork 829
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
GateIO: Split Futures into USDT and CoinM #1786
base: master
Are you sure you want to change the base?
Conversation
This allows easier development of non-stacked version upgrades. Though the PRs still need to be merged sequentially, or renumbered right before merging
* Fix CancelBatchOrders using wrong endpoint for CoinMarginedFutures * Fix TestGetActiveOrders expecting currency.ErrCurrencyPairsEmpty
002f3c5
to
b45be62
Compare
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.
Nice cleanup and some minor nits! Thanks. 🚀
func (c *FuturesContract) openInterest() float64 { | ||
return c.QuantoMultiplier.Float64() * float64(c.PositionSize) * c.IndexPrice.Float64() | ||
} |
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 you but this looks like notional value not open interest. Open interest can be derived from GetFutureStats
this call
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.
Okay. I think that's outside the scope of this PR, in that it's an existing bug from #1417 which isn't made worse by this change.
It does throw shade on the abstraction work done to support this bug though 😂
@gloriousCode Could you glance and confirm that you agree this is incorrect?
I'll open the issue and address it in a separate PR, if so.
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 ran the tests, looked at the numbers, carried the one, compared with the website and confirmed it is wrong
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 the changes! 👍
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.
tACK. Thanks!
Rationale
GateIO has divergent URLs for handling CoinM and USDT futures, and Delivery Futures.
The changes originally in GateIO made sense prior to shazbert's asset based routing system, but it's clear now that in contexts where it's possible, we want to ensure asset splits that allow request/response routing.
Delivery Futures are actually USDT Delivery futures, and if GateIO get around to introducing CoinM Delivery Futures we'll have to split that too.
This is a pre-cursor to better websocket connection/subscription routing for this exchange (and others)
Progresses #1764
Changes
Type of change
How has this been tested