From 395132a9e733e4d97fa911e6288f2fcd2c3be65b Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 6 Jul 2023 21:38:43 +0200 Subject: [PATCH 1/2] fix: ICA account delegation wasn't accounted. Added a test case. --- x/staking/keeper/liquid_stake.go | 16 ++++----- x/staking/keeper/liquid_stake_test.go | 16 +++++---- x/staking/keeper/msg_server.go | 18 +++++----- x/staking/keeper/msg_server_test.go | 50 +++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 23 deletions(-) diff --git a/x/staking/keeper/liquid_stake.go b/x/staking/keeper/liquid_stake.go index e44f74b..c87a231 100644 --- a/x/staking/keeper/liquid_stake.go +++ b/x/staking/keeper/liquid_stake.go @@ -124,36 +124,36 @@ func (k Keeper) DecreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sdk.Int) // SafelyIncreaseValidatorTotalLiquidShares increments the total liquid shares on a validator, if: // the validator bond factor and validator liquid staking cap will not be exceeded by this delegation -func (k Keeper) SafelyIncreaseValidatorTotalLiquidShares(ctx sdk.Context, validator types.Validator, shares sdk.Dec) error { +func (k Keeper) SafelyIncreaseValidatorTotalLiquidShares(ctx sdk.Context, validator *types.Validator, shares sdk.Dec) error { // Confirm the validator bond factor and validator liquid staking cap will be not exceeded - if k.CheckExceedsValidatorBondCap(ctx, validator, shares) { + if k.CheckExceedsValidatorBondCap(ctx, *validator, shares) { return types.ErrInsufficientValidatorBondShares } - if k.CheckExceedsValidatorLiquidStakingCap(ctx, validator, shares) { + if k.CheckExceedsValidatorLiquidStakingCap(ctx, *validator, shares) { return types.ErrValidatorLiquidStakingCapExceeded } // Increment the validator's total liquid shares validator.TotalLiquidShares = validator.TotalLiquidShares.Add(shares) - k.SetValidator(ctx, validator) + k.SetValidator(ctx, *validator) return nil } // DecreaseValidatorTotalLiquidShares decrements the total liquid shares on a validator -func (k Keeper) DecreaseValidatorTotalLiquidShares(ctx sdk.Context, validator types.Validator, shares sdk.Dec) error { +func (k Keeper) DecreaseValidatorTotalLiquidShares(ctx sdk.Context, validator *types.Validator, shares sdk.Dec) error { if shares.GT(validator.TotalLiquidShares) { return types.ErrValidatorLiquidSharesUnderflow } validator.TotalLiquidShares = validator.TotalLiquidShares.Sub(shares) - k.SetValidator(ctx, validator) + k.SetValidator(ctx, *validator) return nil } // SafelyDecreaseValidatorBond decrements the total validator's self bond // so long as it will not cause the current delegations to exceed the threshold // set by validator bond factor -func (k Keeper) SafelyDecreaseValidatorBond(ctx sdk.Context, validator types.Validator, shares sdk.Dec) error { +func (k Keeper) SafelyDecreaseValidatorBond(ctx sdk.Context, validator *types.Validator, shares sdk.Dec) error { // Check if the decreased self bond will cause the validator bond threshold to be exceeded validatorBondFactor := k.ValidatorBondFactor(ctx) maxValTotalShare := validator.TotalValidatorBondShares.Sub(shares).Mul(validatorBondFactor) @@ -163,7 +163,7 @@ func (k Keeper) SafelyDecreaseValidatorBond(ctx sdk.Context, validator types.Val // Decrement the validator's total self bond validator.TotalValidatorBondShares = validator.TotalValidatorBondShares.Sub(shares) - k.SetValidator(ctx, validator) + k.SetValidator(ctx, *validator) return nil } diff --git a/x/staking/keeper/liquid_stake_test.go b/x/staking/keeper/liquid_stake_test.go index 69d4a0a..468796f 100644 --- a/x/staking/keeper/liquid_stake_test.go +++ b/x/staking/keeper/liquid_stake_test.go @@ -662,7 +662,8 @@ func TestSafelyIncreaseValidatorTotalLiquidShares(t *testing.T) { // Attempt to increase the validator liquid shares, it should throw an // error that the validator bond cap was exceeded - err := app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, initialValidator, firstIncreaseAmount) + validator := initialValidator + err := app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, firstIncreaseAmount) require.ErrorIs(t, err, types.ErrInsufficientValidatorBondShares) checkValidatorLiquidShares(initialLiquidShares, "shares after low bond factor") @@ -671,13 +672,15 @@ func TestSafelyIncreaseValidatorTotalLiquidShares(t *testing.T) { app.StakingKeeper.SetParams(ctx, params) // Try the increase again and check that it succeeded + validator = initialValidator expectedLiquidSharesAfterFirstStake := initialLiquidShares.Add(firstIncreaseAmount) - err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, initialValidator, firstIncreaseAmount) + err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, firstIncreaseAmount) require.NoError(t, err) checkValidatorLiquidShares(expectedLiquidSharesAfterFirstStake, "shares with cap loose bond cap") // Attempt another increase, it should fail from the liquid staking cap - err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, initialValidator, secondIncreaseAmount) + validator = initialValidator + err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, secondIncreaseAmount) require.ErrorIs(t, err, types.ErrValidatorLiquidStakingCapExceeded) checkValidatorLiquidShares(expectedLiquidSharesAfterFirstStake, "shares after liquid staking cap hit") @@ -686,8 +689,9 @@ func TestSafelyIncreaseValidatorTotalLiquidShares(t *testing.T) { app.StakingKeeper.SetParams(ctx, params) // Finally confirm that the increase succeeded this time + validator = initialValidator expectedLiquidSharesAfterSecondStake := initialLiquidShares.Add(secondIncreaseAmount) - err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, initialValidator, secondIncreaseAmount) + err = app.StakingKeeper.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, secondIncreaseAmount) require.NoError(t, err, "no error expected after increasing liquid staking cap") checkValidatorLiquidShares(expectedLiquidSharesAfterSecondStake, "shares after loose liquid stake cap") } @@ -711,7 +715,7 @@ func TestDecreaseValidatorTotalLiquidShares(t *testing.T) { app.StakingKeeper.SetValidator(ctx, initialValidator) // Decrease the validator liquid shares, and confirm the new share amount has been updated - err := app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, initialValidator, decreaseAmount) + err := app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, &initialValidator, decreaseAmount) require.NoError(t, err, "no error expected when decreasing validator total liquid shares") actualValidator, found := app.StakingKeeper.GetLiquidValidator(ctx, valAddress) @@ -719,7 +723,7 @@ func TestDecreaseValidatorTotalLiquidShares(t *testing.T) { require.Equal(t, initialLiquidShares.Sub(decreaseAmount), actualValidator.TotalLiquidShares, "total liquid shares") // Attempt to decrease by a larger amount than it has, it should fail - err = app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, actualValidator, initialLiquidShares) + err = app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, &actualValidator, initialLiquidShares) require.ErrorIs(t, err, types.ErrValidatorLiquidSharesUnderflow) } diff --git a/x/staking/keeper/msg_server.go b/x/staking/keeper/msg_server.go index 9949c5f..cb67adc 100644 --- a/x/staking/keeper/msg_server.go +++ b/x/staking/keeper/msg_server.go @@ -236,7 +236,7 @@ func (k msgServer) Delegate(goCtx context.Context, msg *types.MsgDelegate) (*typ if err := k.SafelyIncreaseTotalLiquidStakedTokens(ctx, tokens, false); err != nil { return nil, err } - if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, validator, shares); err != nil { + if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, shares); err != nil { return nil, err } } @@ -322,7 +322,7 @@ func (k msgServer) BeginRedelegate(goCtx context.Context, msg *types.MsgBeginRed // if this is a validator self-bond, the new liquid delegation cannot fall below the self-bond * bond factor if delegation.ValidatorBond { - if err := k.SafelyDecreaseValidatorBond(ctx, srcValidator, srcShares); err != nil { + if err := k.SafelyDecreaseValidatorBond(ctx, &srcValidator, srcShares); err != nil { return nil, err } } @@ -331,10 +331,10 @@ func (k msgServer) BeginRedelegate(goCtx context.Context, msg *types.MsgBeginRed // cannot exceed that validator's self-bond cap // The liquid shares from the source validator should get moved to the destination validator if k.AccountIsLiquidStakingProvider(delegatorAddress) { - if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, dstValidator, dstShares); err != nil { + if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, &dstValidator, dstShares); err != nil { return nil, err } - if err := k.DecreaseValidatorTotalLiquidShares(ctx, srcValidator, srcShares); err != nil { + if err := k.DecreaseValidatorTotalLiquidShares(ctx, &srcValidator, srcShares); err != nil { return nil, err } } @@ -421,7 +421,7 @@ func (k msgServer) Undelegate(goCtx context.Context, msg *types.MsgUndelegate) ( // if this is a validator self-bond, the new liquid delegation cannot fall below the self-bond * bond factor if delegation.ValidatorBond { - if err := k.SafelyDecreaseValidatorBond(ctx, validator, shares); err != nil { + if err := k.SafelyDecreaseValidatorBond(ctx, &validator, shares); err != nil { return nil, err } } @@ -432,7 +432,7 @@ func (k msgServer) Undelegate(goCtx context.Context, msg *types.MsgUndelegate) ( if err := k.DecreaseTotalLiquidStakedTokens(ctx, tokens); err != nil { return nil, err } - if err := k.DecreaseValidatorTotalLiquidShares(ctx, validator, shares); err != nil { + if err := k.DecreaseValidatorTotalLiquidShares(ctx, &validator, shares); err != nil { return nil, err } } @@ -528,7 +528,7 @@ func (k msgServer) CancelUnbondingDelegation(goCtx context.Context, msg *types.M if err := k.SafelyIncreaseTotalLiquidStakedTokens(ctx, tokens, false); err != nil { return nil, err } - if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, validator, shares); err != nil { + if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, shares); err != nil { return nil, err } } @@ -695,7 +695,7 @@ func (k msgServer) TokenizeShares(goCtx context.Context, msg *types.MsgTokenizeS if err := k.SafelyIncreaseTotalLiquidStakedTokens(ctx, msg.Amount.Amount, true); err != nil { return nil, err } - if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, validator, shares); err != nil { + if err := k.SafelyIncreaseValidatorTotalLiquidShares(ctx, &validator, shares); err != nil { return nil, err } } @@ -821,7 +821,7 @@ func (k msgServer) RedeemTokens(goCtx context.Context, msg *types.MsgRedeemToken if err := k.DecreaseTotalLiquidStakedTokens(ctx, tokens); err != nil { return nil, err } - if err := k.DecreaseValidatorTotalLiquidShares(ctx, validator, shares); err != nil { + if err := k.DecreaseValidatorTotalLiquidShares(ctx, &validator, shares); err != nil { return nil, err } } diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 54471f7..07517c5 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -923,3 +923,53 @@ func TestUnbondValidator(t *testing.T) { require.True(t, found) require.True(t, validator.Jailed) } + +// TestICADelegateUndelegate tests that an ICA account can undelegate +// sequentially right after delegating. +func TestICADelegateUndelegate(t *testing.T) { + _, app, ctx := createTestInput(t) + msgServer := keeper.NewMsgServerImpl(app.StakingKeeper) + + // Create a delegator and validator + delegateAmount := sdk.NewInt(1000) + delegateCoin := sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), delegateAmount) + + // Create ICA module account + icaAccountAddress := createICAAccount(app, ctx, "ica-module-account") + + // Fund module account + err := app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, sdk.NewCoins(delegateCoin)) + require.NoError(t, err) + err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, icaAccountAddress, sdk.NewCoins(delegateCoin)) + require.NoError(t, err) + + addresses := simapp.AddTestAddrs(app, ctx, 1, sdk.NewInt(0)) + pubKeys := simapp.CreateTestPubKeys(1) + validatorAddress := sdk.ValAddress(addresses[0]) + validator := teststaking.NewValidator(t, validatorAddress, pubKeys[0]) + + validator.DelegatorShares = sdk.NewDec(1_000_000) + validator.Tokens = sdk.NewInt(1_000_000) + validator.TotalLiquidShares = sdk.NewDec(0) + app.StakingKeeper.SetValidator(ctx, validator) + + delegateMsg := types.MsgDelegate{ + DelegatorAddress: icaAccountAddress.String(), + ValidatorAddress: validatorAddress.String(), + Amount: delegateCoin, + } + + undelegateMsg := types.MsgUndelegate{ + DelegatorAddress: icaAccountAddress.String(), + ValidatorAddress: validatorAddress.String(), + Amount: delegateCoin, + } + + // Delegate normally + _, err = msgServer.Delegate(sdk.WrapSDKContext(ctx), &delegateMsg) + require.NoError(t, err, "no error expected when delegating") + + // Try to undelegate + _, err = msgServer.Undelegate(sdk.WrapSDKContext(ctx), &undelegateMsg) + require.NoError(t, err, "no error expected when sequentially undelegating") +} From 1319381b3d3bc124862f8f7b9f1128568f932274 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 10 Jul 2023 12:55:36 +0200 Subject: [PATCH 2/2] fix: address comment and extend the test case (borrowed from v0.45.16-lsm-ics version). --- x/staking/keeper/msg_server_test.go | 31 +++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/x/staking/keeper/msg_server_test.go b/x/staking/keeper/msg_server_test.go index 07517c5..3700aff 100644 --- a/x/staking/keeper/msg_server_test.go +++ b/x/staking/keeper/msg_server_test.go @@ -930,14 +930,12 @@ func TestICADelegateUndelegate(t *testing.T) { _, app, ctx := createTestInput(t) msgServer := keeper.NewMsgServerImpl(app.StakingKeeper) - // Create a delegator and validator + // Create a delegator and validator (the delegator will be an ICA account) delegateAmount := sdk.NewInt(1000) delegateCoin := sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), delegateAmount) - - // Create ICA module account icaAccountAddress := createICAAccount(app, ctx, "ica-module-account") - // Fund module account + // Fund ICA account err := app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, sdk.NewCoins(delegateCoin)) require.NoError(t, err) err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, icaAccountAddress, sdk.NewCoins(delegateCoin)) @@ -969,7 +967,32 @@ func TestICADelegateUndelegate(t *testing.T) { _, err = msgServer.Delegate(sdk.WrapSDKContext(ctx), &delegateMsg) require.NoError(t, err, "no error expected when delegating") + // Confirm delegation record + _, found := app.StakingKeeper.GetDelegation(ctx, icaAccountAddress, validatorAddress) + require.True(t, found, "delegation should have been found") + + // Confirm liquid staking totals were incremented + expectedTotalLiquidStaked := delegateAmount.Int64() + actualTotalLiquidStaked := app.StakingKeeper.GetTotalLiquidStakedTokens(ctx).Int64() + require.Equal(t, expectedTotalLiquidStaked, actualTotalLiquidStaked, "total liquid staked tokens after delegation") + + validator, found = app.StakingKeeper.GetValidator(ctx, validatorAddress) + require.True(t, found, "validator should have been found") + require.Equal(t, delegateAmount.ToDec(), validator.TotalLiquidShares, "validator total liquid shares after delegation") + // Try to undelegate _, err = msgServer.Undelegate(sdk.WrapSDKContext(ctx), &undelegateMsg) require.NoError(t, err, "no error expected when sequentially undelegating") + + // Confirm delegation record was removed + _, found = app.StakingKeeper.GetDelegation(ctx, icaAccountAddress, validatorAddress) + require.False(t, found, "delegation not have been found") + + // Confirm liquid staking totals were decremented + actualTotalLiquidStaked = app.StakingKeeper.GetTotalLiquidStakedTokens(ctx).Int64() + require.Zero(t, actualTotalLiquidStaked, "total liquid staked tokens after undelegation") + + validator, found = app.StakingKeeper.GetValidator(ctx, validatorAddress) + require.True(t, found, "validator should have been found") + require.Equal(t, sdk.ZeroDec(), validator.TotalLiquidShares, "validator total liquid shares after undelegation") }