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

CHANGELOG.md

@@ -56,6 +56,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* [#24060](https://github.com/cosmos/cosmos-sdk/pull/24060) 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).
 * (baseapp) [#24042](https://github.com/cosmos/cosmos-sdk/pull/24042) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests.
 * (client/server) [#24059](https://github.com/cosmos/cosmos-sdk/pull/24059) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server.
 * (client/keys) [#24041](https://github.com/cosmos/cosmos-sdk/pull/24041) `keys delete` won't terminate when a key is not found, but will log the error.

tests/integration/bank/keeper/deterministic_test.go

@@ -63,6 +63,7 @@ type deterministicFixture struct {
 }
 
 func initDeterministicFixture(t *testing.T) *deterministicFixture {
+	t.Helper()
 	keys := storetypes.NewKVStoreKeys(authtypes.StoreKey, banktypes.StoreKey)
 	cdc := moduletestutil.MakeTestEncodingConfig(auth.AppModuleBasic{}, bank.AppModuleBasic{}).Codec
 
@@ -225,7 +226,7 @@ func TestGRPCQuerySpendableBalances(t *testing.T) {
 	assert.NilError(t, err)
 
 	req := banktypes.NewQuerySpendableBalancesRequest(addr1, nil)
-	testdata.DeterministicIterations(f.ctx, t, req, f.queryClient.SpendableBalances, 2032, false)
+	testdata.DeterministicIterations(f.ctx, t, req, f.queryClient.SpendableBalances, 1675, false)
 }
 
 func TestGRPCQueryTotalSupply(t *testing.T) {
@@ -311,7 +312,8 @@ func TestGRPCQueryParams(t *testing.T) {
 			DefaultSendEnabled: rapid.Bool().Draw(rt, "send"),
 		}
 
-		f.bankKeeper.SetParams(f.ctx, params)
+		err := f.bankKeeper.SetParams(f.ctx, params)
+		assert.NilError(t, err)
 
 		req := &banktypes.QueryParamsRequest{}
 		testdata.DeterministicIterations(f.ctx, t, req, f.queryClient.Params, 0, true)
@@ -327,7 +329,8 @@ func TestGRPCQueryParams(t *testing.T) {
 		DefaultSendEnabled: false,
 	}
 
-	f.bankKeeper.SetParams(f.ctx, params)
+	err := f.bankKeeper.SetParams(f.ctx, params)
+	assert.NilError(t, err)
 
 	req := &banktypes.QueryParamsRequest{}
 	testdata.DeterministicIterations(f.ctx, t, req, f.queryClient.Params, 1003, false)

x/bank/keeper/grpc_query.go

@@ -95,22 +95,25 @@ func (k BaseKeeper) SpendableBalances(ctx context.Context, req *types.QuerySpend
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 
 	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(sdkCtx, 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(sdkCtx, 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

@@ -1473,12 +1473,27 @@ func (suite *KeeperTestSuite) TestMsgMultiSendEvents() {
 	require.Equal(abci.Event(event3), events[29])
 }
 
+// TestSpendableCoins verifies the behavior of SpendableCoins under various scenarios:
+// 1. Normal non-vesting account:
+//   - The account has no locked coins so that the full balance is spendable.
+//
+// 2. Vesting account with partial unlock:
+//   - After advancing time to mid-vesting, part of the total coins are locked.
+//     The spendable coins are calculated as total minus locked coins.
+//
+// 3. Vesting account with mixed locked and unlocked coins:
+//   - Some denominations are over-locked (resulting in a negative subtraction) so that those
+//     denominations yield a zero spendable amount, while others return the positive unlocked value.
+//
+// 4. Vesting account with all coins locked:
+//   - When vesting has not started (all coins locked), the spendable coins are zero.
 func (suite *KeeperTestSuite) TestSpendableCoins() {
 	ctx := sdk.UnwrapSDKContext(suite.ctx)
 	require := suite.Require()
 	now := cmttime.Now()
 	endTime := now.Add(24 * time.Hour)
 
+	// Case 1: Normal non-vesting account with full spendable balance.
 	origCoins := sdk.NewCoins(sdk.NewInt64Coin("stake", 100))
 	lockedCoins := sdk.NewCoins(sdk.NewInt64Coin("stake", 50))
 
@@ -1499,12 +1514,50 @@ func (suite *KeeperTestSuite) TestSpendableCoins() {
 	suite.mockSpendableCoins(ctx, acc1)
 	require.Equal(origCoins[0], suite.bankKeeper.SpendableCoin(ctx, accAddrs[1], "stake"))
 
+	// Case 2: Vesting account with partial unlock.
 	ctx = ctx.WithBlockTime(now.Add(12 * time.Hour))
 	suite.mockSpendableCoins(ctx, vacc)
 	require.Equal(origCoins.Sub(lockedCoins...), suite.bankKeeper.SpendableCoins(ctx, accAddrs[0]))
 
 	suite.mockSpendableCoins(ctx, vacc)
 	require.Equal(origCoins.Sub(lockedCoins...)[0], suite.bankKeeper.SpendableCoin(ctx, accAddrs[0], "stake"))
+
+	// Case 3: Vesting account with mixed locked and unlocked coins.
+	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"))
+
+	// Case 4: All coins locked.
+	// Create a vesting account that has not started vesting yet (all coins locked).
+	acc3 := authtypes.NewBaseAccountWithAddress(accAddrs[3])
+	// Set vesting start to one hour in the future so that all coins are locked.
+	futureStart := now.Add(1 * time.Hour)
+	futureEnd := futureStart.Add(24 * time.Hour)
+	vaccAllLocked, err := vesting.NewContinuousVestingAccount(acc3, sdk.NewCoins(sdk.NewInt64Coin("stake", 50)), futureStart.Unix(), futureEnd.Unix())
+	suite.Require().NoError(err)
+	suite.mockFundAccount(accAddrs[3])
+	require.NoError(banktestutil.FundAccount(ctx, suite.bankKeeper, accAddrs[3], sdk.NewCoins(sdk.NewInt64Coin("stake", 50))))
+	// Since vesting has not started, all coins remain locked and spendable coins should be zero.
+	suite.mockSpendableCoins(ctx, vaccAllLocked)
+	require.Equal(sdk.NewCoins(), suite.bankKeeper.SpendableCoins(ctx, accAddrs[3]))
 }
 
 func (suite *KeeperTestSuite) TestVestingAccountSend() {

x/bank/keeper/view.go

@@ -190,11 +190,34 @@ func (k BaseViewKeeper) LockedCoins(ctx context.Context, addr sdk.AccAddress) sd
 	return sdk.NewCoins()
 }
 
-// SpendableCoins returns the total balances of spendable coins for an account
-// by address. If the account has no spendable coins, an empty Coins slice is
-// returned.
+// SpendableCoins returns the total balances of spendable coins (i.e. unlocked) for an account by address.
+// It works as follows:
+//  1. It retrieves the total balance (all coins) for the account.
+//  2. It gets the set of locked coins (coins that are not spendable, e.g. due to vesting).
+//  3. If there are no locked coins, it returns the total balance as all coins are spendable.
+//  4. Otherwise, it attempts to subtract the locked coins from the total balance using SafeSub.
+//     - If SafeSub does not produce any negative values, the result (unlocked coins) is returned.
+//     - If subtraction results in any negative values for a denomination (i.e. there are more locked coins than available in that denom),
+//     then for that denomination the spendable amount is considered to be zero. In this case, the function iterates over each coin in the
+//     unlocked result and includes only those coins with a positive amount in the final spendable coins set.
 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
 }
 
@@ -203,23 +226,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