From 2cd8a1a8a710535b0ade5a2dbc9d44bb0af09018 Mon Sep 17 00:00:00 2001 From: Daniel Wedul Date: Tue, 10 Sep 2024 12:12:48 -0600 Subject: [PATCH] fix(x/bank): Better handling of negative spendable balances (#21407) (cherry picked from commit 71fa043ef48867b8be71f1bd9cb5f37d73e89e05) --- .../bank/keeper/deterministic_test.go | 2 +- x/bank/CHANGELOG.md | 7 ++++ x/bank/keeper/grpc_query.go | 25 ++++++----- x/bank/keeper/keeper_test.go | 22 ++++++++++ x/bank/keeper/view.go | 41 +++++++++++-------- 5 files changed, 68 insertions(+), 29 deletions(-) diff --git a/tests/integration/bank/keeper/deterministic_test.go b/tests/integration/bank/keeper/deterministic_test.go index de25231adbf2..575fdaead315 100644 --- a/tests/integration/bank/keeper/deterministic_test.go +++ b/tests/integration/bank/keeper/deterministic_test.go @@ -266,7 +266,7 @@ func TestGRPCQuerySpendableBalances(t *testing.T) { assert.NilError(t, err) req := banktypes.NewQuerySpendableBalancesRequest(addr1Str, nil) - testdata.DeterministicIterations(t, f.ctx, req, f.queryClient.SpendableBalances, 1777, false) + testdata.DeterministicIterations(t, f.ctx, req, f.queryClient.SpendableBalances, 1420, false) } func TestGRPCQueryTotalSupply(t *testing.T) { diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md index 2531ccc2bb3c..f226eac88503 100644 --- a/x/bank/CHANGELOG.md +++ b/x/bank/CHANGELOG.md @@ -36,6 +36,13 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#20517](https://github.com/cosmos/cosmos-sdk/pull/20517) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`. * [#20354](https://github.com/cosmos/cosmos-sdk/pull/20354) Reduce the number of `ValidateDenom` calls in `bank.SendCoins`. +### Bug Fixes + +* [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix handling of negative spendable balances. + * The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page. + * The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`). + * The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin). + ### API Breaking Changes * [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes: diff --git a/x/bank/keeper/grpc_query.go b/x/bank/keeper/grpc_query.go index df7c5f312178..c6d499143d83 100644 --- a/x/bank/keeper/grpc_query.go +++ b/x/bank/keeper/grpc_query.go @@ -92,22 +92,25 @@ func (k BaseKeeper) SpendableBalances(ctx context.Context, req *types.QuerySpend } zeroAmt := math.ZeroInt() - - balances, pageRes, err := query.CollectionPaginate(ctx, k.Balances, req.Pagination, func(key collections.Pair[sdk.AccAddress, string], _ math.Int) (coin sdk.Coin, err error) { - return sdk.NewCoin(key.K2(), zeroAmt), nil + 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) } - result := sdk.NewCoins() - spendable := k.SpendableCoins(ctx, addr) - - for _, c := range balances { - result = append(result, sdk.NewCoin(c.Denom, spendable.AmountOf(c.Denom))) - } - - return &types.QuerySpendableBalancesResponse{Balances: result, Pagination: pageRes}, nil + return &types.QuerySpendableBalancesResponse{Balances: balances, Pagination: pageRes}, nil } // SpendableBalanceByDenom implements a gRPC query handler for retrieving an account's diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index a5680bfb43ba..120e8a2537f7 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -1527,6 +1527,28 @@ func (suite *KeeperTestSuite) TestSpendableCoins() { suite.mockSpendableCoins(ctx, vacc) require.Equal(origCoins.Sub(lockedCoins...)[0], suite.bankKeeper.SpendableCoin(ctx, accAddrs[0], "stake")) + + acc2 := authtypes.NewBaseAccountWithAddress(accAddrs[2]) + lockedCoins2 := sdk.NewCoins(sdk.NewInt64Coin("stake", 50), sdk.NewInt64Coin("tarp", 40), sdk.NewInt64Coin("rope", 30)) + balanceCoins2 := sdk.NewCoins(sdk.NewInt64Coin("stake", 49), sdk.NewInt64Coin("tarp", 40), sdk.NewInt64Coin("rope", 31), sdk.NewInt64Coin("pole", 20)) + expCoins2 := sdk.NewCoins(sdk.NewInt64Coin("rope", 1), sdk.NewInt64Coin("pole", 20)) + vacc2, err := vesting.NewPermanentLockedAccount(acc2, lockedCoins2) + suite.Require().NoError(err) + + // Go back to the suite's context since mockFundAccount uses that; FundAccount would fail for bad mocking otherwise. + ctx = sdk.UnwrapSDKContext(suite.ctx) + suite.mockFundAccount(accAddrs[2]) + require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[2], balanceCoins2)) + suite.mockSpendableCoins(ctx, vacc2) + require.Equal(expCoins2, suite.bankKeeper.SpendableCoins(ctx, accAddrs[2])) + suite.mockSpendableCoins(ctx, vacc2) + require.Equal(sdk.NewInt64Coin("stake", 0), suite.bankKeeper.SpendableCoin(ctx, accAddrs[2], "stake")) + suite.mockSpendableCoins(ctx, vacc2) + require.Equal(sdk.NewInt64Coin("tarp", 0), suite.bankKeeper.SpendableCoin(ctx, accAddrs[2], "tarp")) + suite.mockSpendableCoins(ctx, vacc2) + require.Equal(sdk.NewInt64Coin("rope", 1), suite.bankKeeper.SpendableCoin(ctx, accAddrs[2], "rope")) + suite.mockSpendableCoins(ctx, vacc2) + require.Equal(sdk.NewInt64Coin("pole", 20), suite.bankKeeper.SpendableCoin(ctx, accAddrs[2], "pole")) } func (suite *KeeperTestSuite) TestVestingAccountSend() { diff --git a/x/bank/keeper/view.go b/x/bank/keeper/view.go index 48a725989b3b..54236a60aa07 100644 --- a/x/bank/keeper/view.go +++ b/x/bank/keeper/view.go @@ -190,7 +190,23 @@ func (k BaseViewKeeper) LockedCoins(ctx context.Context, addr sdk.AccAddress) sd // by address. If the account has no spendable coins, an empty Coins slice is // returned. func (k BaseViewKeeper) SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins { - spendable, _ := k.spendableCoins(ctx, addr) + 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 } @@ -199,23 +215,14 @@ func (k BaseViewKeeper) SpendableCoins(ctx context.Context, addr sdk.AccAddress) // is returned. func (k BaseViewKeeper) SpendableCoin(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin { balance := k.GetBalance(ctx, addr, denom) - locked := k.LockedCoins(ctx, addr) - return balance.SubAmount(locked.AmountOf(denom)) -} - -// spendableCoins returns the coins the given address can spend alongside the total amount of coins it holds. -// It exists for gas efficiency, in order to avoid to have to get balance multiple times. -func (k BaseViewKeeper) spendableCoins(ctx context.Context, addr sdk.AccAddress) (spendable, total sdk.Coins) { - total = k.GetAllBalances(ctx, addr) - locked := k.LockedCoins(ctx, addr) - - spendable, hasNeg := total.SafeSub(locked...) - if hasNeg { - spendable = sdk.NewCoins() - return + lockedAmt := k.LockedCoins(ctx, addr).AmountOf(denom) + if !lockedAmt.IsPositive() { + return balance } - - return + if lockedAmt.LT(balance.Amount) { + return balance.SubAmount(lockedAmt) + } + return sdk.NewCoin(denom, math.ZeroInt()) } // ValidateBalance validates all balances for a given account address returning