Skip to content

refactor(vesting)!: improve validate functions and return error in constructor #16741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julienrbrt merged 6 commits into from
Jun 29, 2023
Merged

refactor(vesting)!: improve validate functions and return error in constructor #16741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8832ca4
feat(x/auth/vesting): improve validate functions and return error in …
julienrbrt Jun 28, 2023
ce027f8
updates
julienrbrt Jun 28, 2023
4c444dc
simplify
julienrbrt Jun 28, 2023
d7b2131
updates
julienrbrt Jun 28, 2023
747b3a4
updates
julienrbrt Jun 28, 2023
3b59f2d
updates
julienrbrt Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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