-
Notifications
You must be signed in to change notification settings - Fork 837
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
trade: remove exchangeName for AddTradesToBuffer #1820
Conversation
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 opening this PR @junnplus I like the other changes you brought in. For reference the trade package will change from this conversation and it will be getting some love and attention soon.
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.
PR Overview
This PR removes the redundant use of an exchange name parameter from the AddTradesToBuffer calls in multiple exchange implementations, simplifying the trade buffering logic. Key changes include:
- Removal of the extra exchange name argument passed to AddTradesToBuffer.
- Minor refactoring to use more idiomatic Go (e.g. using short variable declarations).
- Consistent update of various exchange websocket and wrapper files to reflect the change.
Reviewed Changes
File | Description |
---|---|
exchanges/exchange.go | Removed the extra exchange name parameter from AddTradesToBuffer. |
exchanges/gemini/gemini_websocket.go | Removed the redundant exchange name parameter in websocket handlers. |
exchanges/kraken/kraken_websocket.go | Updated trade buffering and improved variable declaration style. |
exchanges/stream/websocket.go | Removed exchangeName parameter from Trade.Setup for stream websocket. |
(Other exchange files) | Similar changes applied to remove the exchange name parameter uniformly. |
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
exchanges/exchange.go:1196
- Ensure that the removal of the exchangeName argument from AddTradesToBuffer is safe and that trade.Data constructions still correctly include the exchange information.
return trade.AddTradesToBuffer(trades...)
exchanges/kraken/kraken_websocket.go:557
- [nitpick] Using a short variable declaration here makes the code more idiomatic; ensure that similar changes across the codebase follow consistent style conventions.
tSide := order.Buy
exchanges/stream/websocket.go:203
- Verify that removing the exchangeName parameter from the Trade.Setup call does not affect downstream behavior or break any assumptions about trade identification.
w.Trade.Setup(s.TradeFeed, w.DataHandler)
exchanges/okx/okx_websocket.go:696
- Ensure that tests cover the updated trade buffering logic to verify that the exchange information is correctly propagated within the trade data.
return trade.AddTradesToBuffer(trades...)
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.
Will need a rebase/merge, but it all looks good to me
Signed-off-by: Ye Sijun <[email protected]>
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 for your contribution!
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.
Approved with an optional side quest. Thanks for the PR!
var ( | ||
errParsingWSField = errors.New("error parsing WS field") | ||
) | ||
var errParsingWSField = errors.New("error parsing WS field") |
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.
var errParsingWSField = errors.New("error parsing WS field") | |
var errParsingWSField = errors.New("error parsing WS field") |
Up to you whether you want to do this in this PR, but you can delete this and use common.ErrParsingWSField instead. Looks like there's 4 areas where this is used in bitfinex_websocket.go
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 can create another PR to deal with it.
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.
Oh it's fine, already have some cleanup PR's of my own so will include it in that. Will merge this one as is
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.
Good work, thank you 🙏
It's a step in the right direction for trades
.
Code looks good to me 🎉
PR Description
remove
exchangeName
forAddTradesToBuffer
,exchangeName
is redundant sincedata[i].Exchange
already holds the exchange info.Type of change
Please delete options that are not relevant and add an
x
in[]
as 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