diff --git a/tests/integration/poa_test.go b/tests/integration/poa_test.go index a7b7f7d..49b72e0 100644 --- a/tests/integration/poa_test.go +++ b/tests/integration/poa_test.go @@ -609,7 +609,7 @@ func (s *TestSuite) TestRemoveValidator_UnexistingValidator() { }{ { name: "remove unexisting validator - random address - with balance", - valAddress: randomValAddr.String(), + valAddress: sdktypes.AccAddress(randomValAddr).String(), expectedError: poatypes.ErrAddressIsNotAValidator, beforeRun: func() { _, err := s.Network().GetStakingClient().Validator( @@ -758,7 +758,7 @@ func (s *TestSuite) TestRemoveValidator_ExistingValidator_StatusBonded() { authority := sdktypes.AccAddress(address.Module("gov")) msg := poatypes.NewMsgRemoveValidator( authority.String(), - valAddr.String(), + sdktypes.AccAddress(valAddr).String(), ) proposal, err := utils.SubmitAndAwaitProposalResolution(s.factory, s.Network(), s.keyring.GetKeys(), "test", msg) @@ -871,7 +871,7 @@ func (s *TestSuite) TestRemoveValidator_ExistingValidator_Jailed() { authority := sdktypes.AccAddress(address.Module("gov")) msg := poatypes.NewMsgRemoveValidator( authority.String(), - valAddr.String(), + sdktypes.AccAddress(valAddr).String(), ) proposal, err := utils.SubmitAndAwaitProposalResolution(s.factory, s.Network(), s.keyring.GetKeys(), "test", msg) @@ -1014,7 +1014,7 @@ func (s *TestSuite) TestRemoveValidator_ExistingValidator_Tombstoned() { authority := sdktypes.AccAddress(address.Module("gov")) msg := poatypes.NewMsgRemoveValidator( authority.String(), - valAddr.String(), + sdktypes.AccAddress(valAddr).String(), ) proposal, err := utils.SubmitAndAwaitProposalResolution(s.factory, s.Network(), s.keyring.GetKeys(), "test", msg) diff --git a/x/poa/keeper/keeper.go b/x/poa/keeper/keeper.go index 5a6af7b..3b5bff0 100644 --- a/x/poa/keeper/keeper.go +++ b/x/poa/keeper/keeper.go @@ -85,10 +85,10 @@ func (k Keeper) GetAuthority() string { func (k Keeper) ExecuteAddValidator(ctx sdk.Context, msg *types.MsgAddValidator) error { // Check if the new validator already has staking power in the bank account accAddress, err := sdk.AccAddressFromBech32(msg.ValidatorAddress) - valAddress := sdk.ValAddress(accAddress) if err != nil { return err } + valAddress := sdk.ValAddress(accAddress) params, err := k.sk.GetParams(ctx) if err != nil { return err @@ -207,10 +207,11 @@ func (k Keeper) ExecuteAddValidator(ctx sdk.Context, msg *types.MsgAddValidator) } func (k Keeper) ExecuteRemoveValidator(ctx sdk.Context, validatorAddress string) error { - valAddress, err := sdk.ValAddressFromBech32(validatorAddress) + accAddress, err := sdk.AccAddressFromBech32(validatorAddress) if err != nil { return err } + valAddress := sdk.ValAddress(accAddress) params, err := k.sk.GetParams(ctx) if err != nil { return err @@ -272,7 +273,7 @@ func (k Keeper) ExecuteRemoveValidator(ctx sdk.Context, validatorAddress string) } // Unbond self-delegation so the validator is removed after being unbonded - _, err = k.sk.Unbond(ctx, sdk.AccAddress(valAddress), valAddress, changedVal.DelegatorShares) + _, err = k.sk.Unbond(ctx, accAddress, valAddress, changedVal.DelegatorShares) if err != nil { return err } diff --git a/x/poa/keeper/keeper_test.go b/x/poa/keeper/keeper_test.go index d2f340e..582d96f 100644 --- a/x/poa/keeper/keeper_test.go +++ b/x/poa/keeper/keeper_test.go @@ -408,7 +408,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - staking keeper returns error on GetParams", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("staking params error"), stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{}, errors.New("staking params error")) @@ -418,7 +418,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { { name: "should fail - staking keeper returns error on GetValidator", expectedError: types.ErrAddressIsNotAValidator, - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ BondDenom: "BND", @@ -429,7 +429,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - staking keeper returns error on call GetUnbondingDelegationsFromValidator", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("staking keeper get unbonding delegations from validator error"), stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ @@ -446,7 +446,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - staking keeper returns error on call SlashUnbondingDelegation", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("staking keeper slash unbonding delegation error"), stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ @@ -471,7 +471,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - staking keeper returns error on RemoveValidatorTokens call", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("staking keeper remove validator tokens error"), stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ @@ -498,7 +498,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { //nolint:dupl { name: "should fail - bank keeper returns error on call BurnCoins for status bonded", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("bank keeper burn coins error"), //nolint:dupl stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { @@ -529,7 +529,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { //nolint:dupl { name: "should fail - bank keeper returns error on call BurnCoins for status unbonding/unbonded", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("bank keeper burn coins error"), //nolint:dupl stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { @@ -559,7 +559,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - bank keeper returns error for invalid validator status", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: types.ErrInvalidValidatorStatus, //nolint:dupl stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { @@ -587,7 +587,7 @@ func TestKeeper_ExecuteRemoveValidator(t *testing.T) { }, { name: "should fail - staking keeper returns error on call Unbond", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", expectedError: errors.New("staking keeper unbond error"), stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ diff --git a/x/poa/keeper/msg_server_remove_validator_test.go b/x/poa/keeper/msg_server_remove_validator_test.go index 83a1047..ebdbb1b 100644 --- a/x/poa/keeper/msg_server_remove_validator_test.go +++ b/x/poa/keeper/msg_server_remove_validator_test.go @@ -10,10 +10,10 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "github.com/xrplevm/node/v10/x/poa/testutil" "github.com/xrplevm/node/v10/x/poa/types" + "go.uber.org/mock/gomock" ) func TestMsgServer_RemoveValidator(t *testing.T) { @@ -31,7 +31,7 @@ func TestMsgServer_RemoveValidator(t *testing.T) { { name: "should fail - invalid authority address", authority: "invalidauthority", - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", stakingMocks: func(_ sdk.Context, _ *testutil.MockStakingKeeper) {}, bankMocks: func(_ sdk.Context, _ *testutil.MockBankKeeper) {}, expectedErr: govtypes.ErrInvalidSigner, @@ -47,7 +47,7 @@ func TestMsgServer_RemoveValidator(t *testing.T) { { name: "should pass", authority: poaAuthority, - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", stakingMocks: func(ctx sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(ctx).Return(stakingtypes.Params{ BondDenom: "BND", @@ -73,7 +73,7 @@ func TestMsgServer_RemoveValidator(t *testing.T) { { name: "should pass - BeforeValidatorModified hook error is swallowed and logged", authority: poaAuthority, - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", stakingMocks: func(_ sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { // gomock.Any() for ctx because the test swaps the logger after // setup, producing a different sdk.Context value. @@ -102,7 +102,7 @@ func TestMsgServer_RemoveValidator(t *testing.T) { { name: "should pass - BeforeValidatorSlashed hook error is swallowed and logged", authority: poaAuthority, - validatorAddress: "ethmvaloper1a0pd5cyew47pvgf7rd7axxy3humv9ev0urudmu", + validatorAddress: "ethm1a0pd5cyew47pvgf7rd7axxy3humv9ev0nnkprp", stakingMocks: func(_ sdk.Context, stakingKeeper *testutil.MockStakingKeeper) { stakingKeeper.EXPECT().GetParams(gomock.Any()).Return(stakingtypes.Params{ BondDenom: "BND", diff --git a/x/poa/types/message_add_validator.go b/x/poa/types/message_add_validator.go index 8306917..1b5ea59 100644 --- a/x/poa/types/message_add_validator.go +++ b/x/poa/types/message_add_validator.go @@ -4,11 +4,13 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) var ( _ sdk.Msg = &MsgAddValidator{} + _ sdk.HasValidateBasic = &MsgAddValidator{} _ codectypes.UnpackInterfacesMessage = (*MsgAddValidator)(nil) ) @@ -28,6 +30,23 @@ func NewMsgAddValidator(authority string, address string, pubKey cryptotypes.Pub }, nil } +// ValidateBasic performs stateless validation of the message fields. +func (msg *MsgAddValidator) ValidateBasic() error { + if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) + } + if _, err := sdk.AccAddressFromBech32(msg.ValidatorAddress); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", err) + } + if msg.Pubkey == nil { + return sdkerrors.ErrInvalidPubKey.Wrap("validator pubkey is required") + } + if _, ok := msg.Pubkey.GetCachedValue().(cryptotypes.PubKey); !ok { + return sdkerrors.ErrInvalidPubKey.Wrapf("expecting cryptotypes.PubKey, got %T", msg.Pubkey.GetCachedValue()) + } + return nil +} + // UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces func (msg *MsgAddValidator) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { var pubKey cryptotypes.PubKey diff --git a/x/poa/types/message_add_validator_test.go b/x/poa/types/message_add_validator_test.go new file mode 100644 index 0000000..6ec617a --- /dev/null +++ b/x/poa/types/message_add_validator_test.go @@ -0,0 +1,64 @@ +package types + +import ( + "testing" + + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/require" +) + +func TestMsgAddValidator_ValidateBasic(t *testing.T) { + validAddr := sdk.AccAddress("12345678901234567890").String() + + validPkAny, err := codectypes.NewAnyWithValue(ed25519.GenPrivKey().PubKey()) + require.NoError(t, err) + + wrongTypeAny, err := codectypes.NewAnyWithValue(&MsgRemoveValidator{}) + require.NoError(t, err) + + tt := []struct { + name string + msg *MsgAddValidator + expectedErr string + }{ + { + name: "should pass - valid message", + msg: &MsgAddValidator{Authority: validAddr, ValidatorAddress: validAddr, Pubkey: validPkAny}, + }, + { + name: "should fail - invalid authority address", + msg: &MsgAddValidator{Authority: "invalid", ValidatorAddress: validAddr, Pubkey: validPkAny}, + expectedErr: "invalid authority address", + }, + { + name: "should fail - invalid validator address", + msg: &MsgAddValidator{Authority: validAddr, ValidatorAddress: "invalid", Pubkey: validPkAny}, + expectedErr: "invalid validator address", + }, + { + name: "should fail - nil pubkey", + msg: &MsgAddValidator{Authority: validAddr, ValidatorAddress: validAddr, Pubkey: nil}, + expectedErr: "validator pubkey is required", + }, + { + name: "should fail - pubkey is not a cryptotypes.PubKey", + msg: &MsgAddValidator{Authority: validAddr, ValidatorAddress: validAddr, Pubkey: wrongTypeAny}, + expectedErr: "expecting cryptotypes.PubKey", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + err := tc.msg.ValidateBasic() + + if tc.expectedErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedErr) + return + } + require.NoError(t, err) + }) + } +} diff --git a/x/poa/types/message_remove_validator.go b/x/poa/types/message_remove_validator.go index 85af3f2..bc7bf8b 100644 --- a/x/poa/types/message_remove_validator.go +++ b/x/poa/types/message_remove_validator.go @@ -2,9 +2,13 @@ package types import ( sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -var _ sdk.Msg = &MsgRemoveValidator{} +var ( + _ sdk.Msg = &MsgRemoveValidator{} + _ sdk.HasValidateBasic = &MsgRemoveValidator{} +) func NewMsgRemoveValidator(authority string, address string) *MsgRemoveValidator { return &MsgRemoveValidator{ @@ -12,3 +16,14 @@ func NewMsgRemoveValidator(authority string, address string) *MsgRemoveValidator ValidatorAddress: address, } } + +// ValidateBasic performs stateless validation of the message fields. +func (msg *MsgRemoveValidator) ValidateBasic() error { + if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err) + } + if _, err := sdk.AccAddressFromBech32(msg.ValidatorAddress); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", err) + } + return nil +} diff --git a/x/poa/types/message_remove_validator_test.go b/x/poa/types/message_remove_validator_test.go new file mode 100644 index 0000000..2306ce0 --- /dev/null +++ b/x/poa/types/message_remove_validator_test.go @@ -0,0 +1,64 @@ +package types + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/require" +) + +func TestMsgRemoveValidator_ValidateBasic(t *testing.T) { + validAddr := sdk.AccAddress("12345678901234567890").String() + + tt := []struct { + name string + authority string + validator string + expectedErr string + }{ + { + name: "should pass - valid authority and validator address", + authority: validAddr, + validator: validAddr, + }, + { + name: "should fail - invalid authority address", + authority: "invalid", + validator: validAddr, + expectedErr: "invalid authority address", + }, + { + name: "should fail - empty authority address", + authority: "", + validator: validAddr, + expectedErr: "invalid authority address", + }, + { + name: "should fail - invalid validator address", + authority: validAddr, + validator: "invalid", + expectedErr: "invalid validator address", + }, + { + name: "should fail - empty validator address", + authority: validAddr, + validator: "", + expectedErr: "invalid validator address", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + msg := NewMsgRemoveValidator(tc.authority, tc.validator) + + err := msg.ValidateBasic() + + if tc.expectedErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedErr) + return + } + require.NoError(t, err) + }) + } +}