fix(x/bank): Better handling of negative spendable balances #21407
Conversation
…fit from the pagination by subtracting locked coins instead of selecting the desired entries out of the k.SpendableCoins results. That k.SpendableCoins method first gets all balances, then subtracts the locked coins. So this query was effectively getting all balances every time instead of only info on the current page's entries.
…e denom is negative; now it'll treat just that one as zero instead of the entire spendable balance. Also update SpendableCoin to not panic when there's more locked than available; just return a zero coin in that case.
…uld fail without this change).
WalkthroughWalkthroughThe changes in the Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
SpicyLemon
changed the title
…ar to when not looking.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
x/bank/CHANGELOG.md (1)
41-41: Correct the preposition.The preposition "on" seems more appropriate than "in" in this context.
- restrict the balance lookups to only the denoms in the page being returned. + restrict the balance lookups to only the denoms on the page being returned.Tools
LanguageTool
[uncategorized] ~41-~41: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... the balance lookups to only the denoms in the page being returned. * [#21407](htt...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
x/bank/keeper/view.go (2)
193-209: Add comments for clarity.The logic in the
SpendableCoinsmethod is correct, but adding comments explaining each step would improve readability.func (k BaseViewKeeper) SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins { total := k.GetAllBalances(ctx, addr) allLocked := k.LockedCoins(ctx, addr) if allLocked.IsZero() { return total } unlocked, hasNeg := total.SafeSub(allLocked...) if !hasNeg { return unlocked } spendable := sdk.Coins{} for _, coin := range unlocked { if coin.IsPositive() { spendable = append(spendable, coin) } } return spendable }
218-225: Add comments for clarity.The logic in the
SpendableCoinmethod is correct, but adding comments explaining each step would improve readability.func (k BaseViewKeeper) SpendableCoin(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin { balance := k.GetBalance(ctx, addr, denom) lockedAmt := k.LockedCoins(ctx, addr).AmountOf(denom) if !lockedAmt.IsPositive() { return balance } if lockedAmt.LT(balance.Amount) { return balance.SubAmount(lockedAmt) } return sdk.NewCoin(denom, math.ZeroInt()) }x/bank/keeper/grpc_query.go (1)
95-113: Add comments for clarity.The logic in the
SpendableBalancesmethod is correct, but adding comments explaining each step would improve readability.func (k BaseKeeper) SpendableBalances(ctx context.Context, req *types.QuerySpendableBalancesRequest) (*types.QuerySpendableBalancesResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "empty request") } addr, err := k.ak.AddressCodec().StringToBytes(req.Address) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "invalid address: %s", err.Error()) } zeroAmt := math.ZeroInt() allLocked := k.LockedCoins(ctx, addr) balances, pageRes, err := query.CollectionPaginate(ctx, k.Balances, req.Pagination, func(key collections.Pair[sdk.AccAddress, string], balanceAmt math.Int) (sdk.Coin, error) { denom := key.K2() coin := sdk.NewCoin(denom, zeroAmt) lockedAmt := allLocked.AmountOf(denom) switch { case !lockedAmt.IsPositive(): coin.Amount = balanceAmt case lockedAmt.LT(balanceAmt): coin.Amount = balanceAmt.Sub(lockedAmt) } return coin, nil }, query.WithCollectionPaginationPairPrefix[sdk.AccAddress, string](addr)) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "paginate: %v", err) } return &types.QuerySpendableBalancesResponse{Balances: balances, Pagination: pageRes}, nil }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/bank/CHANGELOG.md (1 hunks)
- x/bank/keeper/grpc_query.go (1 hunks)
- x/bank/keeper/keeper_test.go (1 hunks)
- x/bank/keeper/view.go (2 hunks)
Additional context used
Path-based instructions (4)
x/bank/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/bank/keeper/view.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
LanguageTool
x/bank/CHANGELOG.md
[uncategorized] ~41-~41: The preposition “on” seems more likely in this position than the preposition “in”.
Context: ... the balance lookups to only the denoms in the page being returned. * [#21407](htt...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_ON)
Additional comments not posted (3)
x/bank/CHANGELOG.md (2)
42-42: LGTM!The changelog entry is clear and correctly describes the fix.
43-43: LGTM!The changelog entry is clear and correctly describes the fix.
x/bank/keeper/keeper_test.go (1)
1532-1553: LGTM!The new test cases for the second account in
TestSpendableCoinsenhance the robustness of the test suite by ensuring the behavior of the bank keeper is validated for multiple accounts under different conditions.The code changes are approved.
…gas cost went down with this change.
julienrbrt
added
the
backport/v0.52.x
x/bank/CHANGELOG.md
Outdated
| * [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix the `SpendableBalances` query to correctly report spendable balances when one or more are negative. Also restrict the balance lookups to only the denoms in the page being returned. | ||
| * [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix the `SpendableCoins` keeper method to correctly return the positive spendable balances when one or more denoms have more locked than available. | ||
| * [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix the `SpendableCoin` keeper method to return a zero coin if there's more locked than available. |
There was a problem hiding this comment.
could this be just one entry??
(cherry picked from commit 71fa043)
Clients can't do the math on their side because there's no way to look up the amount that is locked. And even if that amount were available, anyone redoing it client-side would almost certainly be doing that as a workaround for the bugs being fixed here. I don't feel that api breaking or client breaking entries are necessary for this PR. These fixes will not require most people to alter how this stuff is used. The only ones that might want to make changes are those that noticed the bugs and previously tried to account for or work around them. But I feel that the changelog entries under "Bug Fixes" sufficiently communicate that there's some cleanup those folks might be able to do. Also, I don't think this test is very worthwhile: "If a client [is replicating this stuff] on its side, then it might break that client." That's true of most changes, and isn't in line with the definitions of "Client Breaking" or "API Breaking" . |
Description
This PR fixes a few things related to spendable balances.
SpendableCoinskeeper method to return all positive balances instead of returning zero when one denom has more locked than available. Before this PR, if someone had10denomaand15denomb, but had11denomalocked, this method would return an emptyCoinseven though there's actually15denombthat they could spend. After this PR, it'd return that15denomb.SpendableCoinkeeper method to report zero spendable if there's more locked than available for that denom. Before this PR, if there were more locked than available, this method would return a negative coin. In the SDK, the only place this method is used is in theSpendableBalanceByDenomquery, so the SDK didn't have any further bugs here, but there could easily be such bugs in other chains.SpendableBalancesquery. Before this PR, it was using the pagination stuff to identify which denoms to include in the results, then calling theSpendableCoinskeeper method to get the amounts. But that keeper method callsGetAllBalancesand gets the balances of all denoms anyway, defeating the purpose of the pagination. With this PR, it only gets the page of balances and reduces each by their locked amounts.SpendableBalancesquery so that it returns the correct amounts when an account has one or more denoms with more locked than available. Before this PR, if an account had an amount of two denoms, but has more locked than available for one of them, this query would return two zero-coin entries, one for each denom. After this PR, it'll return a zero-coin entry just for the one denom, and the actual spendable amount for the other.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
SpendableCoinsmethod to return only positive spendable balances.Tests