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 (backport #16741) (#16763)

Co-authored-by: Julien Robert <julien@rbrt.fr>
  • Loading branch information
@mergify @julienrbrt
mergify[bot] and julienrbrt authored Jun 29, 2023
1 parent 1a7f4dc commit 979c5d7
Show file tree
Hide file tree
Showing 11 changed files with 287 additions and 137 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ 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.
* (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.

## [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
22 changes: 15 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 @@ -170,6 +176,10 @@ func (s msgServer) CreatePeriodicVestingAccount(goCtx context.Context, msg *type
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "invalid period length of %d in period %d, length must be greater than 0", period.Length, i)
}

if err := validateAmount(period.Amount); err != nil {
return nil, err
}

totalCoins = totalCoins.Add(period.Amount...)
}

Expand All @@ -184,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
39 changes: 38 additions & 1 deletion x/auth/vesting/msg_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"

"cosmossdk.io/math"
storetypes "cosmossdk.io/store/types"

"github.com/cosmos/cosmos-sdk/runtime"
Expand Down Expand Up @@ -96,7 +97,18 @@ func (s *VestingTestSuite) TestCreateVestingAccount() {
expErr: true,
expErrMsg: "invalid 'to' address",
},
"": {
"invalid coins": {
input: vestingtypes.NewMsgCreateVestingAccount(
fromAddr,
to1Addr,
sdk.Coins{sdk.Coin{Denom: "stake", Amount: math.NewInt(-1)}},
time.Now().Unix(),
true,
),
expErr: true,
expErrMsg: "-1stake: invalid coins",
},
"invalid end time": {
input: vestingtypes.NewMsgCreateVestingAccount(
fromAddr,
to1Addr,
Expand Down Expand Up @@ -199,6 +211,15 @@ func (s *VestingTestSuite) TestCreatePermanentLockedAccount() {
expErr: true,
expErrMsg: "invalid 'to' address",
},
"invalid coins": {
input: vestingtypes.NewMsgCreatePermanentLockedAccount(
fromAddr,
to1Addr,
sdk.Coins{sdk.Coin{Denom: "stake", Amount: math.NewInt(-1)}},
),
expErr: true,
expErrMsg: "-1stake: invalid coins",
},
"create for existing account": {
preRun: func() {
toAcc := s.accountKeeper.NewAccountWithAddress(s.ctx, to1Addr)
Expand Down Expand Up @@ -319,6 +340,22 @@ func (s *VestingTestSuite) TestCreatePeriodicVestingAccount() {
expErr: true,
expErrMsg: "invalid period",
},
{
name: "invalid coins",
input: vestingtypes.NewMsgCreatePeriodicVestingAccount(
fromAddr,
to1Addr,
time.Now().Unix(),
[]vestingtypes.Period{
{
Length: 1,
Amount: sdk.Coins{sdk.Coin{Denom: "stake", Amount: math.NewInt(-1)}},
},
},
),
expErr: true,
expErrMsg: "-1stake: invalid coins",
},
{
name: "create for existing account",
preRun: func() {
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 979c5d7

Please sign in to comment.