-
Notifications
You must be signed in to change notification settings - Fork 874
bybit: fix UpdateAccountInfo free and hold calculations #1980
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
base: master
Are you sure you want to change the base?
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.
Pull Request Overview
This PR fixes the calculation of free and hold amounts in the UpdateAccountInfo method for Bybit exchange integration. The issue was that the previous calculations were using incorrect field mappings for determining available balances and locked amounts.
- Updated free balance calculation to account for borrowed amounts and locked funds
- Changed hold calculation to use the locked amount directly instead of deriving from wallet balance difference
- Updated corresponding test assertions to match the corrected calculations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
exchanges/bybit/bybit_wrapper.go | Fixed free and hold balance calculations in UpdateAccountInfo method |
exchanges/bybit/bybit_test.go | Updated test assertions to match corrected balance calculations |
exchanges/bybit/bybit_wrapper.go
Outdated
@@ -572,9 +570,9 @@ func (e *Exchange) UpdateAccountInfo(ctx context.Context, assetType asset.Item) | |||
balance := account.Balance{ | |||
Currency: balances.List[i].Coin[c].Coin, | |||
Total: balances.List[i].Coin[c].WalletBalance.Float64(), | |||
Free: balances.List[i].Coin[c].AvailableToWithdraw.Float64(), | |||
Free: (balances.List[i].Coin[c].BorrowAmount.Float64() + balances.List[i].Coin[c].WalletBalance.Float64()) - balances.List[i].Coin[c].Locked.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.
The free balance calculation is complex and difficult to understand. Consider extracting this calculation into a separate variable or adding a comment explaining the logic (borrowed + wallet - locked = free).
Copilot uses AI. Check for mistakes.
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'm fine with not addressing this, because of the overlap with #1923
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 disagree on the basis that 1923 has not yet been reviewed by everyone and will probably take awhile. Plus merging more readable code now is better than waiting. The rebase impact to you will be the same as it is now - very painful 😭
So I think improving readability here is best
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.
For me, readable would be:
Free: (balances.List[i].Coin[c].BorrowAmount.Float64() + balances.List[i].Coin[c].WalletBalance.Float64()) - balances.List[i].Coin[c].Locked.Float64(), | |
Free: b.BorrowAmount.Float64() + b.WalletBalance.Float64() - b.Locked.Float64(), |
I just realised: What's with the parenthesis around the addition ... 🤷
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.
exchanges/bybit/bybit_test.go
Outdated
assert.Equal(t, 30723.630216383714, r.Accounts[0].Currencies[x].Borrowed, "Borrowed amount should match") | ||
assert.Equal(t, 0.0, r.Accounts[0].Currencies[x].Free, "Free amount should match") | ||
assert.Equal(t, 3.714376362040639e-09, r.Accounts[0].Currencies[x].Free, "Free amount should match") |
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.
The expected value uses scientific notation with extreme precision (3.714376362040639e-09). Consider using a more readable format or defining this as a constant with a descriptive name to clarify why this specific value is expected.
assert.Equal(t, 3.714376362040639e-09, r.Accounts[0].Currencies[x].Free, "Free amount should match") | |
assert.Equal(t, expectedFreeAmountUSDC, r.Accounts[0].Currencies[x].Free, "Free amount should match") |
Copilot uses AI. Check for mistakes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1980 +/- ##
=======================================
Coverage 39.66% 39.67%
=======================================
Files 432 432
Lines 171920 171920
=======================================
+ Hits 68198 68202 +4
+ Misses 96598 96596 -2
+ Partials 7124 7122 -2
🚀 New features to boost your workflow:
|
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.
Looks good to me.
I'll update #1923 accordingly once this merges 👍
One possible simplification on the mock test precision.
exchanges/bybit/bybit_test.go
Outdated
assert.Equal(t, 30723.630216383714, r.Accounts[0].Currencies[x].Borrowed, "Borrowed amount should match") | ||
assert.Equal(t, 0.0, r.Accounts[0].Currencies[x].Free, "Free amount should match") | ||
assert.Equal(t, 3.714376362040639e-09, r.Accounts[0].Currencies[x].Free, "Free amount should match") |
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.
These are mock tests, right?
Can we just simplify the amount we're expecting here or do we really need to exercise this level of precision?
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.
Even better would be changing the result to one that populates all the fields that have been updated for calculations. Better for testing
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.
Well it depends, it seems like this is a real world example. I don't have borrow amounts to inject an update but this does. The problem with adjusting to be exact would obfuscate the truncation that takes place on the total "walletBalance":"-30723.63021638"
to 8 decimal places but the borrowed amount "borrowAmount":"30723.630216383711792744"
is truncated to 18 decimal places. I could strip the result on the mock by truncating the borrowAmount or if I detect a negative value I flip free to zero, but that might be disingenuous. How I am calculating this could be completely incorrect as well. But it seems more correct than what it was. Thoughts?
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.
@gloriousCode @gbjk bump
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'm fine if you don't address this.
But:
seems like this is a real world example
- If I'm not really sure about the source, I never trust mock data- Truncation is going to happen or it isn't but it doesn't feel like it's part of our code or testing - Nothing we're doing is going to be exercised by having bigger numbers. I think you're just exercising Go, and I think they might already have some unit tests.
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.
addressed: e42403d
exchanges/bybit/bybit_wrapper.go
Outdated
@@ -572,9 +570,9 @@ func (e *Exchange) UpdateAccountInfo(ctx context.Context, assetType asset.Item) | |||
balance := account.Balance{ | |||
Currency: balances.List[i].Coin[c].Coin, | |||
Total: balances.List[i].Coin[c].WalletBalance.Float64(), | |||
Free: balances.List[i].Coin[c].AvailableToWithdraw.Float64(), | |||
Free: (balances.List[i].Coin[c].BorrowAmount.Float64() + balances.List[i].Coin[c].WalletBalance.Float64()) - balances.List[i].Coin[c].Locked.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.
I'm fine with not addressing this, because of the overlap with #1923
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.
Looks good, I agree with the AI 🤖
exchanges/bybit/bybit_wrapper.go
Outdated
@@ -572,9 +570,9 @@ func (e *Exchange) UpdateAccountInfo(ctx context.Context, assetType asset.Item) | |||
balance := account.Balance{ | |||
Currency: balances.List[i].Coin[c].Coin, | |||
Total: balances.List[i].Coin[c].WalletBalance.Float64(), | |||
Free: balances.List[i].Coin[c].AvailableToWithdraw.Float64(), | |||
Free: (balances.List[i].Coin[c].BorrowAmount.Float64() + balances.List[i].Coin[c].WalletBalance.Float64()) - balances.List[i].Coin[c].Locked.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.
I disagree on the basis that 1923 has not yet been reviewed by everyone and will probably take awhile. Plus merging more readable code now is better than waiting. The rebase impact to you will be the same as it is now - very painful 😭
So I think improving readability here is best
exchanges/bybit/bybit_test.go
Outdated
assert.Equal(t, 30723.630216383714, r.Accounts[0].Currencies[x].Borrowed, "Borrowed amount should match") | ||
assert.Equal(t, 0.0, r.Accounts[0].Currencies[x].Free, "Free amount should match") | ||
assert.Equal(t, 3.714376362040639e-09, r.Accounts[0].Currencies[x].Free, "Free amount should match") |
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.
Even better would be changing the result to one that populates all the fields that have been updated for calculations. Better for testing
PR Description
For free field available to withdraw is a separate to tradable amounts. New calculation derives diff from borrow amount as wallet balance includes negative amounts that are borrowed. Then reduces the result by the locked amount which can be a limit order.
Hold is just the locked amount.
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