From e28b9d8d6105d539fd2a6b00123661c356039b60 Mon Sep 17 00:00:00 2001 From: dongsam Date: Mon, 21 Feb 2022 18:53:44 +0900 Subject: [PATCH] fix: allow delegate only spendable coins (cherry picked from commit 47933d027a21cdf193d4a573866e485728ae695c) --- x/auth/legacy/v043/store_test.go | 86 ++++++++++++++++------------- x/staking/keeper/delegation.go | 4 +- x/staking/simulation/operations.go | 4 +- x/staking/types/expected_keepers.go | 2 + 4 files changed, 53 insertions(+), 43 deletions(-) diff --git a/x/auth/legacy/v043/store_test.go b/x/auth/legacy/v043/store_test.go index 3daa2d52fc53..e729c819bb1d 100644 --- a/x/auth/legacy/v043/store_test.go +++ b/x/auth/legacy/v043/store_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/stretchr/testify/require" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" @@ -110,14 +111,17 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) + // failed because spendable balances is 100 _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(300), stakingtypes.Unbonded, validator, true) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) + _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) require.NoError(t, err) }, cleartTrackingFields, 300, - 200, 100, 0, + 0, }, { "delayed vesting has not vested, multiple delegations which exceed the vested amount", @@ -131,16 +135,17 @@ func TestMigrateVestingAccounts(t *testing.T) { _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) require.NoError(t, err) + // failed because of no more spendable balances _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) }, cleartTrackingFields, 300, - 200, 100, 0, + 0, }, { "not end time", @@ -151,16 +156,17 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) + // failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) }, cleartTrackingFields, 300, - 300, + 0, 0, 0, }, @@ -173,12 +179,13 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) + // failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(300), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) }, cleartTrackingFields, 300, - 300, + 0, 0, 0, }, @@ -218,11 +225,11 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(300), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) }, cleartTrackingFields, 300, - 300, + 0, 0, 0, }, @@ -240,14 +247,17 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) + // failed because spendable balances is 100 _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(300), stakingtypes.Unbonded, validator, true) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) + _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(100), stakingtypes.Unbonded, validator, true) require.NoError(t, err) }, cleartTrackingFields, 300, - 200, 100, 0, + 0, }, { "continuous vesting, start time and endtime passed", @@ -309,9 +319,8 @@ func TestMigrateVestingAccounts(t *testing.T) { - periodic vesting account starts at time 1601042400 - account balance and original vesting: 3666666670000 - nothing has vested, we put the block time slightly after start time - - expected vested: original vesting amount + - expected vested: zero - expected free: zero - - we're delegating the full original vesting */ startTime := int64(1601042400) baseAccount := authtypes.NewBaseAccountWithAddress(delegatorAddr) @@ -335,13 +344,13 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) - // delegation of the original vesting + // delegation of the original vesting, failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(3666666670000), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) }, cleartTrackingFields, 3666666670000, - 3666666670000, + 0, 0, 1601042400 + 1, }, @@ -399,9 +408,9 @@ func TestMigrateVestingAccounts(t *testing.T) { - periodic vesting account starts at time 1601042400 - account balance and original vesting: 3666666670000 - first period have vested, so we set the block time at initial time + time of the first periods + 1 => 1601042400 + 31536000 + 1 - - expected vested: original vesting - first period amount - - expected free: first period amount - - we're delegating the full original vesting + - expected vested: first period amount + - expected free: zero + - we're delegating the vested amount */ startTime := int64(1601042400) baseAccount := authtypes.NewBaseAccountWithAddress(delegatorAddr) @@ -427,14 +436,17 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) - // delegation of the original vesting + // delegation of the original vesting, failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(3666666670000), stakingtypes.Unbonded, validator, true) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) + // delegation of vested spendable amount + _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(1833333335000), stakingtypes.Unbonded, validator, true) require.NoError(t, err) }, cleartTrackingFields, 3666666670000, - 3666666670000 - 1833333335000, 1833333335000, + 0, 1601042400 + 31536000 + 1, }, { @@ -445,9 +457,9 @@ func TestMigrateVestingAccounts(t *testing.T) { - periodic vesting account starts at time 1601042400 - account balance and original vesting: 3666666670000 - first 2 periods have vested, so we set the block time at initial time + time of the two periods + 1 => 1601042400 + 31536000 + 15638400 + 1 - - expected vested: original vesting - (sum of the first two periods amounts) - - expected free: sum of the first two periods - - we're delegating the full original vesting + - expected vested: amount of the second periods + - expected free: amount of the second periods + - we're delegating the first period have vested */ startTime := int64(1601042400) baseAccount := authtypes.NewBaseAccountWithAddress(delegatorAddr) @@ -473,14 +485,17 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) - // delegation of the original vesting + // delegation of the original vesting, failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(3666666670000), stakingtypes.Unbonded, validator, true) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) + // delegation of the first period have vested + _, err = app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(1833333335000), stakingtypes.Unbonded, validator, true) require.NoError(t, err) }, cleartTrackingFields, 3666666670000, - 3666666670000 - 1833333335000 - 916666667500, - 1833333335000 + 916666667500, + 916666667500, + 916666667500, 1601042400 + 31536000 + 15638400 + 1, }, { @@ -493,22 +508,15 @@ func TestMigrateVestingAccounts(t *testing.T) { app.AccountKeeper.SetAccount(ctx, delayedAccount) - // delegation of the original vesting + // delegation of the original vesting, failed because of no spendable balances _, err := app.StakingKeeper.Delegate(ctx, delegatorAddr, sdk.NewInt(300), stakingtypes.Unbonded, validator, true) - require.NoError(t, err) + require.ErrorIs(t, err, sdkerrors.ErrInsufficientFunds) ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0)) - - valAddr, err := sdk.ValAddressFromBech32(validator.OperatorAddress) - require.NoError(t, err) - - // un-delegation of the original vesting - _, err = app.StakingKeeper.Undelegate(ctx, delegatorAddr, valAddr, sdk.NewDecFromInt(sdk.NewInt(300))) - require.NoError(t, err) }, cleartTrackingFields, 450, - 300, + 0, 0, 0, }, diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 6ed92e37aab9..186d37520063 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -593,7 +593,7 @@ func (k Keeper) Delegate( } coins := sdk.NewCoins(sdk.NewCoin(k.BondDenom(ctx), bondAmt)) - if err := k.bankKeeper.DelegateCoinsFromAccountToModule(ctx, delegatorAddress, sendName, coins); err != nil { + if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, delegatorAddress, sendName, coins); err != nil { return sdk.Dec{}, err } } else { @@ -779,7 +779,7 @@ func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAd // track undelegation only when remaining or truncated shares are non-zero if !entry.Balance.IsZero() { amt := sdk.NewCoin(bondDenom, entry.Balance) - if err := k.bankKeeper.UndelegateCoinsFromModuleToAccount( + if err := k.bankKeeper.SendCoinsFromModuleToAccount( ctx, types.NotBondedPoolName, delegatorAddress, sdk.NewCoins(amt), ); err != nil { return nil, err diff --git a/x/staking/simulation/operations.go b/x/staking/simulation/operations.go index ce5ef3dc38ac..966004336a19 100644 --- a/x/staking/simulation/operations.go +++ b/x/staking/simulation/operations.go @@ -106,7 +106,7 @@ func SimulateMsgCreateValidator(ak types.AccountKeeper, bk types.BankKeeper, k k denom := k.GetParams(ctx).BondDenom - balance := bk.GetBalance(ctx, simAccount.Address, denom).Amount + balance := bk.SpendableCoins(ctx, simAccount.Address).AmountOf(denom) if !balance.IsPositive() { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgCreateValidator, "balance is negative"), nil, nil } @@ -248,7 +248,7 @@ func SimulateMsgDelegate(ak types.AccountKeeper, bk types.BankKeeper, k keeper.K return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgDelegate, "validator's invalid echange rate"), nil, nil } - amount := bk.GetBalance(ctx, simAccount.Address, denom).Amount + amount := bk.SpendableCoins(ctx, simAccount.Address).AmountOf(denom) if !amount.IsPositive() { return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgDelegate, "balance is negative"), nil, nil } diff --git a/x/staking/types/expected_keepers.go b/x/staking/types/expected_keepers.go index 313db78e7b1c..586bacca19da 100644 --- a/x/staking/types/expected_keepers.go +++ b/x/staking/types/expected_keepers.go @@ -35,6 +35,8 @@ type BankKeeper interface { SendCoinsFromModuleToModule(ctx sdk.Context, senderPool, recipientPool string, amt sdk.Coins) error UndelegateCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error DelegateCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error + SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error + SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error BurnCoins(ctx sdk.Context, name string, amt sdk.Coins) error }