Skip to content

Conversation

shazbert
Copy link
Collaborator

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.

  • 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

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.

  • 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 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 with my changes
  • Any dependent changes have been merged and published in downstream modules

@shazbert shazbert requested a review from a team July 24, 2025 03:52
@shazbert shazbert self-assigned this Jul 24, 2025
@shazbert shazbert added bug review me This pull request is ready for review labels Jul 24, 2025
@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 03:52
Copy link
Contributor

@Copilot Copilot AI left a 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

@@ -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(),
Copy link
Preview

Copilot AI Jul 24, 2025

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.

Copy link
Collaborator

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

Copy link
Collaborator

@gloriousCode gloriousCode Aug 4, 2025

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

Copy link
Collaborator

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:

Suggested change
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 ... 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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")
Copy link
Preview

Copilot AI Jul 24, 2025

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.

Suggested change
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.

Copy link

codecov bot commented Jul 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.67%. Comparing base (0fd3334) to head (9931503).

Additional details and impacted files

Impacted file tree graph

@@           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     
Files with missing lines Coverage Δ
exchanges/bybit/bybit_wrapper.go 47.32% <100.00%> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

shazbert pushed a commit to shazbert/gocryptotrader that referenced this pull request Jul 25, 2025
Copy link
Collaborator

@gbjk gbjk left a 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.

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")
Copy link
Collaborator

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?

Copy link
Collaborator

@gloriousCode gloriousCode Aug 4, 2025

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

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed: e42403d

@@ -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(),
Copy link
Collaborator

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

Copy link
Collaborator

@gloriousCode gloriousCode left a 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 🤖

@@ -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(),
Copy link
Collaborator

@gloriousCode gloriousCode Aug 4, 2025

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

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")
Copy link
Collaborator

@gloriousCode gloriousCode Aug 4, 2025

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

@shazbert shazbert requested a review from gloriousCode August 6, 2025 00:27
@thrasher- thrasher- self-requested a review August 19, 2025 06:07
@shazbert shazbert requested a review from gbjk August 26, 2025 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants