Skip to content

Commit

Permalink
refactor(vesting)!: improve validate functions and return error in co…
Browse files Browse the repository at this point in the history 
…nstructor (#16741)

(cherry picked from commit adc7451)

# Conflicts:
#	CHANGELOG.md
  • Loading branch information
@julienrbrt @mergify
julienrbrt authored and mergify[bot] committed Jun 29, 2023
1 parent c452721 commit 465677c
Show file tree
Hide file tree
Showing 10 changed files with 272 additions and 136 deletions.
28 changes: 28 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,35 @@ Ref: https://keepachangelog.com/en/1.0.0/

* (x/staking) [#16324](https://github.com/cosmos/cosmos-sdk/pull/16324) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. Notable changes:
* `Validator` method now returns `types.ErrNoValidatorFound` instead of `nil` when not found.
<<<<<<< HEAD
* (x/auth) [#16621](https://github.com/cosmos/cosmos-sdk/pull/16621) Pass address codec to auth new keeper constructor.
=======
* (x/distribution) [#16440](https://github.com/cosmos/cosmos-sdk/pull/16440) use collections for `DelegatorWithdrawAddresState`:
* remove `Keeper`: `SetDelegatorWithdrawAddr`, `DeleteDelegatorWithdrawAddr`, `IterateDelegatorWithdrawAddrs`.
* (x/distribution) [#16459](https://github.com/cosmos/cosmos-sdk/pull/16459) use collections for `ValidatorCurrentRewards` state management:
* remove `Keeper`: `IterateValidatorCurrentRewards`, `GetValidatorCurrentRewards`, `SetValidatorCurrentRewards`, `DeleteValidatorCurrentRewards`
* (x/authz) [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`.
* (x/distribution) [#16483](https://github.com/cosmos/cosmos-sdk/pull/16483) use collections for `DelegatorStartingInfo` state management:
* remove `Keeper`: `IterateDelegatorStartingInfo`, `GetDelegatorStartingInfo`, `SetDelegatorStartingInfo`, `DeleteDelegatorStartingInfo`, `HasDelegatorStartingInfo`
* (x/distribution) [#16571](https://github.com/cosmos/cosmos-sdk/pull/16571) use collections for `ValidatorAccumulatedCommission` state management:
* remove `Keeper`: `IterateValidatorAccumulatedCommission`, `GetValidatorAccumulatedCommission`, `SetValidatorAccumulatedCommission`, `DeleteValidatorAccumulatedCommission`
* (x/distribution) [#16590](https://github.com/cosmos/cosmos-sdk/pull/16590) use collections for `ValidatorOutstandingRewards` state management:
* remove `Keeper`: `IterateValidatorOutstandingRewards`, `GetValidatorOutstandingRewards`, `SetValidatorOutstandingRewards`, `DeleteValidatorOutstandingRewards`
* (x/distribution) [#16607](https://github.com/cosmos/cosmos-sdk/pull/16607) use collections for `ValidatorHistoricalRewards` state management:
* remove `Keeper`: `IterateValidatorHistoricalRewards`, `GetValidatorHistoricalRewards`, `SetValidatorHistoricalRewards`, `DeleteValidatorHistoricalRewards`, `DeleteValidatorHistoricalReward`, `DeleteAllValidatorHistoricalRewards`
* (x/auth) [#16621](https://github.com/cosmos/cosmos-sdk/pull/16621) Pass address codec to auth new keeper constructor.
* (x/auth/vesting) [#16741](https://github.com/cosmos/cosmos-sdk/pull/16741) Vesting account constructor now return an error with the result of their validate function.

### Bug Fixes

* (x/auth/vesting) [#16733](https://github.com/cosmos/cosmos-sdk/pull/16733) panic on overflowing and negative EndTimes when creating a PeriodicVestingAccount
* [#16547](https://github.com/cosmos/cosmos-sdk/pull/16547) Ensure a transaction's gas limit cannot exceed the block gas limit.
* (x/auth/types) [#16554](https://github.com/cosmos/cosmos-sdk/pull/16554) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking.
* (baseapp) [#16613](https://github.com/cosmos/cosmos-sdk/pull/16613) Ensure each message in a transaction has a registered handler, otherwise `CheckTx` will fail.
* (x/consensus) [#16713](https://github.com/cosmos/cosmos-sdk/pull/16713) Add missing ABCI param in MsgUpdateParams.
* [#16639](https://github.com/cosmos/cosmos-sdk/pull/16639) Make sure we don't execute blocks beyond the halt height.
* (baseapp) [#16700](https://github.com/cosmos/cosmos-sdk/pull/16700) Fix: Consensus Failure in returning no response to malformed transactions
>>>>>>> adc7451ad (refactor(vesting)!: improve validate functions and return error in constructor (#16741))
## [v0.50.0-alpha.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.0-alpha.0) - 2023-06-07

Expand Down
57 changes: 38 additions & 19 deletions x/auth/migrations/v2/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(200)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand Down Expand Up @@ -127,7 +128,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
require.NoError(t, err)
baseAccount := createBaseAccount(delegatorAddr)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(200)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand All @@ -149,7 +151,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(200)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand All @@ -175,7 +178,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(200)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand All @@ -195,7 +199,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(200)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand All @@ -219,7 +224,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand All @@ -243,7 +249,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(1, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand All @@ -263,7 +270,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().Unix())
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand All @@ -287,7 +295,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
delayedAccount, err := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand All @@ -311,7 +320,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
delayedAccount, err := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand All @@ -335,7 +345,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdkmath.NewInt(300)))
delayedAccount := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
delayedAccount, err := types.NewContinuousVestingAccount(baseAccount, vestedCoins, startTime, endTime)
require.NoError(t, err)

ctx = ctx.WithBlockTime(ctx.BlockTime().AddDate(1, 0, 0))

Expand Down Expand Up @@ -367,7 +378,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
},
}

account := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, start, periods)
account, err := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, start, periods)
require.NoError(t, err)

accountKeeper.SetAccount(ctx, account)

Expand Down Expand Up @@ -412,7 +424,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
},
}

delayedAccount := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
delayedAccount, err := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand Down Expand Up @@ -458,7 +471,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
},
}

delayedAccount := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
delayedAccount, err := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
require.NoError(t, err)

ctx = ctx.WithBlockTime(time.Unix(1601042400+31536000+15897600+15897600+1, 0))

Expand Down Expand Up @@ -506,7 +520,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
},
}

delayedAccount := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
delayedAccount, err := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
require.NoError(t, err)

ctx = ctx.WithBlockTime(time.Unix(1601042400+31536000+1, 0))

Expand Down Expand Up @@ -554,7 +569,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
},
}

delayedAccount := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
delayedAccount, err := types.NewPeriodicVestingAccount(baseAccount, vestedCoins, startTime, periods)
require.NoError(t, err)

ctx = ctx.WithBlockTime(time.Unix(1601042400+31536000+15638400+1, 0))

Expand All @@ -578,7 +594,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdk.NewInt(300)))

delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)

Expand Down Expand Up @@ -609,7 +626,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdk.NewInt(300)))

delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)
},
Expand All @@ -627,7 +645,8 @@ func TestMigrateVestingAccounts(t *testing.T) {
require.NoError(t, err)
vestedCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, sdk.NewInt(300)))

delayedAccount := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
delayedAccount, err := types.NewDelayedVestingAccount(baseAccount, vestedCoins, ctx.BlockTime().AddDate(10, 0, 0).Unix())
require.NoError(t, err)

accountKeeper.SetAccount(ctx, delayedAccount)
},
Expand Down
5 changes: 4 additions & 1 deletion x/auth/simulation/genesis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ func RandomGenesisAccounts(simState *module.SimulationState) types.GenesisAccoun
endTime = int64(simulation.RandIntBetween(simState.Rand, int(startTime)+1, int(startTime+(60*60*12))))
}

bva := vestingtypes.NewBaseVestingAccount(bacc, initialVesting, endTime)
bva, err := vestingtypes.NewBaseVestingAccount(bacc, initialVesting, endTime)
if err != nil {
panic(err)
}

if simState.Rand.Intn(100) < 50 {
genesisAccs[i] = vestingtypes.NewContinuousVestingAccountRaw(bva, startTime)
Expand Down
18 changes: 11 additions & 7 deletions x/auth/vesting/msg_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCre

baseAccount := authtypes.NewBaseAccountWithAddress(to)
baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
baseVestingAccount := types.NewBaseVestingAccount(baseAccount, msg.Amount.Sort(), msg.EndTime)
baseVestingAccount, err := types.NewBaseVestingAccount(baseAccount, msg.Amount.Sort(), msg.EndTime)
if err != nil {
return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
}

var vestingAccount sdk.AccountI
if msg.Delayed {
Expand Down Expand Up @@ -124,7 +127,10 @@ func (s msgServer) CreatePermanentLockedAccount(goCtx context.Context, msg *type

baseAccount := authtypes.NewBaseAccountWithAddress(to)
baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
vestingAccount := types.NewPermanentLockedAccount(baseAccount, msg.Amount)
vestingAccount, err := types.NewPermanentLockedAccount(baseAccount, msg.Amount)
if err != nil {
return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
}

s.AccountKeeper.SetAccount(ctx, vestingAccount)

Expand Down Expand Up @@ -184,11 +190,9 @@ func (s msgServer) CreatePeriodicVestingAccount(goCtx context.Context, msg *type

baseAccount := authtypes.NewBaseAccountWithAddress(to)
baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
vestingAccount := types.NewPeriodicVestingAccount(baseAccount, totalCoins.Sort(), msg.StartTime, msg.VestingPeriods)

// Enforce and sanity check that we don't have any negative endTime.
if bva := vestingAccount.BaseVestingAccount; bva != nil && bva.EndTime < 0 {
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "cumulative endtime is negative")
vestingAccount, err := types.NewPeriodicVestingAccount(baseAccount, totalCoins.Sort(), msg.StartTime, msg.VestingPeriods)
if err != nil {
return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
}

s.AccountKeeper.SetAccount(ctx, vestingAccount)
Expand Down
3 changes: 2 additions & 1 deletion x/auth/vesting/types/genesis_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ var (
func TestValidateGenesisInvalidAccounts(t *testing.T) {
acc1 := authtypes.NewBaseAccountWithAddress(sdk.AccAddress(addr1))
acc1Balance := sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 150))
baseVestingAcc := NewBaseVestingAccount(acc1, acc1Balance, 1548775410)
baseVestingAcc, err := NewBaseVestingAccount(acc1, acc1Balance, 1548775410)
require.NoError(t, err)

// invalid delegated vesting
baseVestingAcc.DelegatedVesting = acc1Balance.Add(acc1Balance...)
Expand Down
Loading

0 comments on commit 465677c

Please sign in to comment.