fix(x/bank): Better handling of negative spendable balances

x/bank/CHANGELOG.md

@@ -36,6 +36,12 @@ 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 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 spendable.
+* [#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.
+
 ### API Breaking Changes
 
 * [#19954](https://github.com/cosmos/cosmos-sdk/pull/19954) Removal of the Address.String() method and related changes:

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

x/bank/keeper/keeper_test.go

@@ -1529,6 +1529,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() {

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