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)
  • Loading branch information
@julienrbrt
julienrbrt authored Jun 29, 2023
1 parent a30afc4 commit adc7451
Show file tree
Hide file tree
Showing 10 changed files with 246 additions and 137 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
* 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) [#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

Expand Down
57 changes: 38 additions & 19 deletions tests/integration/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 @@ -188,11 +194,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 adc7451

Please sign in to comment.