refactor: poa fixes, tweaks, and chores (#237)

* H-01: deny duplicate submission for a pending validator

* M-01(refactor): simplify removing validators by grabbing directly

* test: pending and duplicate removal

* L-01: Params.Validate staking

* L-02: panic if floor > ciel

* L-03: fix validators if commission is below updated commission rate

* L-04: duplicate validator verification on InitGenesis

* chore: lint

* fix: simulation (no duplicate create validators)

* fix lint

* lint remove fmt prints

* refactor/lint: remove typecast from int64 -> uint64

ante/ante_test.go

@@ -122,6 +122,12 @@ func TestAnteNested(t *testing.T) {
 	ctx := sdk.Context{}
 	ctx = setBlockHeader(ctx, 2)
 
+	t.Run("fail: floor > ceil on create panic", func(t *testing.T) {
+		require.Panics(t, func() {
+			NewCommissionLimitDecorator(true, math.LegacyMustNewDecFromStr("0.50"), math.LegacyMustNewDecFromStr("0.10"))
+		})
+	})
+
 	const invalidRequestErr = "messages contains *types.Any which is not a sdk.MsgRequest"
 	cases := []struct {
 		name      string
@@ -247,7 +253,7 @@ func TestAnteStakingFilter(t *testing.T) {
 		})
 
 		t.Run(fmt.Sprintf("fail: staking action not allowed after gentx (%s)", k), func(t *testing.T) {
-			for h := uint64(2); h < 10; h++ {
+			for h := int64(2); h < 10; h++ {
 				ctx = setBlockHeader(ctx, h)
 				_, err := sf.AnteHandle(ctx, tx, false, EmptyAnte)
 				require.Error(t, err)
@@ -278,7 +284,7 @@ func TestAnteDisableWithdrawRewards(t *testing.T) {
 		})
 
 		t.Run(fmt.Sprintf("fail: withdraw rewards not allowed after gentx (%s)", k), func(t *testing.T) {
-			for h := uint64(2); h < 10; h++ {
+			for h := int64(2); h < 10; h++ {
 				ctx = setBlockHeader(ctx, h)
 				_, err := dwr.AnteHandle(ctx, tx, false, EmptyAnte)
 				require.Error(t, err)
@@ -287,9 +293,9 @@ func TestAnteDisableWithdrawRewards(t *testing.T) {
 	}
 }
 
-func setBlockHeader(ctx sdk.Context, height uint64) sdk.Context {
+func setBlockHeader(ctx sdk.Context, height int64) sdk.Context {
 	h := ctx.BlockHeader()
-	h.Height = int64(height)
+	h.Height = height
 
 	return ctx.WithBlockHeader(h)
 }

ante/commission_limit.go

@@ -24,6 +24,10 @@ type CommissionLimitDecorator struct {
 }
 
 func NewCommissionLimitDecorator(doGenTxRateValidation bool, rateFloor, rateCiel math.LegacyDec) CommissionLimitDecorator {
+	if rateFloor.GT(rateCiel) {
+		panic(fmt.Sprintf("NewCommissionLimitDecorator: rateFloor %v is greater than rateCiel %v", rateFloor, rateCiel))
+	}
+
 	return CommissionLimitDecorator{
 		DoGenTxRateValidation: doGenTxRateValidation,
 		RateFloor:             rateFloor,

errors.go

@@ -10,4 +10,5 @@ var (
 	ErrNotAnAuthority                     = sdkerrors.Register(ModuleName, 3, "not an authority")
 	ErrUnsafePower                        = sdkerrors.Register(ModuleName, 4, "unsafe: msg.Power is >30%% of total power, set unsafe=true to override")
 	ErrWithdrawDelegatorRewardsNotAllowed = sdkerrors.Register(ModuleName, 5, "withdraw delegator rewards is not allowed on this chain")
+	ErrValidatorAlreadyPending            = sdkerrors.Register(ModuleName, 6, "this validator is already pending")
 )

keeper/expected_keepers.go

@@ -52,6 +52,7 @@ type StakingKeeper interface {
 	MinCommissionRate(ctx context.Context) (math.LegacyDec, error)
 	GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (validator stakingtypes.Validator, err error)
 	SetParams(ctx context.Context, params stakingtypes.Params) error
+	GetParams(ctx context.Context) (stakingtypes.Params, error)
 	GetAllDelegations(ctx context.Context) (delegations []stakingtypes.Delegation, err error)
 	BondDenom(ctx context.Context) (string, error)
 	ValidatorAddressCodec() addresscodec.Codec

keeper/genesis.go

@@ -2,12 +2,21 @@ package keeper
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/strangelove-ventures/poa"
 )
 
 // InitGenesis initializes the module's state from a genesis state.
 func (k *Keeper) InitGenesis(ctx context.Context, data *poa.GenesisState) error {
+	found := make(map[string]bool)
+	for _, vals := range data.Vals {
+		if found[vals.OperatorAddress] {
+			return fmt.Errorf("duplicate validator found in genesis state: %s", vals.OperatorAddress)
+		}
+		found[vals.OperatorAddress] = true
+	}
+
 	if err := k.PendingValidators.Set(ctx, poa.Validators{
 		Validators: data.Vals,
 	}); err != nil {

keeper/genesis_test.go

@@ -23,6 +23,18 @@ func TestInitGenesis(t *testing.T) {
 		require.NoError(err)
 	})
 
+	t.Run("duplicate validators found in state", func(t *testing.T) {
+		data := &poa.GenesisState{
+			Vals: []poa.Validator{{
+				OperatorAddress: "cosmos1abc",
+			}, {
+				OperatorAddress: "cosmos1abc",
+			}},
+		}
+		err := fixture.k.InitGenesis(fixture.ctx, data)
+		require.Error(err)
+	})
+
 	t.Run("pending validator export", func(t *testing.T) {
 		acc := GenAcc()
 		valAddr := sdk.ValAddress(acc.addr)

keeper/keeper_test.go

@@ -221,7 +221,7 @@ func (f *testFixture) createBaseStakingValidators(t *testing.T, baseValShares in
 			fmt.Sprintf("val-%d", idx),
 			valAddr,
 			pubKey,
-			bondCoin.Amount.Int64(),
+			bondCoin.Amount.Uint64(),
 		))
 
 		if err := f.k.AddPendingValidator(f.ctx, val, pubKey); err != nil {
@@ -277,7 +277,7 @@ func (f *testFixture) createBaseStakingValidators(t *testing.T, baseValShares in
 	}
 }
 
-func CreateNewValidator(moniker string, opAddr string, pubKey cryptotypes.PubKey, amt int64) poa.Validator {
+func CreateNewValidator(moniker string, opAddr string, pubKey cryptotypes.PubKey, amt uint64) poa.Validator {
 	var pkAny *codectypes.Any
 	if pubKey != nil {
 		var err error
@@ -286,13 +286,15 @@ func CreateNewValidator(moniker string, opAddr string, pubKey cryptotypes.PubKey
 		}
 	}
 
+	sdkIntAmt := sdkmath.NewIntFromUint64(amt)
+
 	return poa.Validator{
 		OperatorAddress: opAddr,
 		ConsensusPubkey: pkAny,
 		Jailed:          false,
 		Status:          poa.Bonded,
-		Tokens:          sdkmath.NewInt(amt),
-		DelegatorShares: sdkmath.LegacyNewDecFromInt(sdkmath.NewInt(amt)),
+		Tokens:          sdkIntAmt,
+		DelegatorShares: sdkmath.LegacyNewDecFromInt(sdkIntAmt),
 		Description:     poa.NewDescription(moniker, "", "", "", ""),
 		UnbondingHeight: 0,
 		UnbondingTime:   time.Time{},
@@ -313,7 +315,7 @@ func (f *testFixture) CreatePendingValidator(name string, power uint64) sdk.ValA
 		name,
 		valAddr.String(),
 		val.valKey.PubKey(),
-		int64(power),
+		power,
 	))
 
 	if err := f.k.AddPendingValidator(f.ctx, v, val.valKey.PubKey()); err != nil {

keeper/msg_server.go

@@ -45,7 +45,7 @@ func (ms msgServer) SetPower(ctx context.Context, msg *poa.MsgSetPower) (*poa.Ms
 	}
 
 	// Sets the new POA power to the validator.
-	if _, err := ms.k.SetPOAPower(ctx, msg.ValidatorAddress, int64(msg.Power)); err != nil {
+	if _, err := ms.k.SetPOAPower(ctx, msg.ValidatorAddress, msg.Power); err != nil {
 		return nil, err
 	}
 
@@ -97,34 +97,23 @@ func (ms msgServer) RemoveValidator(ctx context.Context, msg *poa.MsgRemoveValid
 		}
 	}
 
-	vals, err := ms.k.stakingKeeper.GetAllValidators(ctx)
+	valAddr, err := sdk.ValAddressFromBech32(msg.ValidatorAddress)
 	if err != nil {
-		return nil, err
-	}
-
-	if len(vals) == 1 {
-		return nil, fmt.Errorf("cannot remove the last validator in the set")
-	}
-
-	// Ensure the validator exists and is bonded.
-	found := false
-	for _, val := range vals {
-		if val.OperatorAddress == msg.ValidatorAddress {
-			if !val.IsBonded() {
-				return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "validator %s is not bonded", msg.ValidatorAddress)
-			}
-
-			found = true
-			break
-		}
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", err)
 	}
 
-	if !found {
+	val, err := ms.k.stakingKeeper.GetValidator(ctx, valAddr)
+	if err != nil {
+		// validator not found in the set.
 		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "validator %s does not exist", msg.ValidatorAddress)
 	}
+	// Validator must exist and be bonded for us to set to remove it from the set
+	if !val.IsBonded() {
+		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "validator %s is not bonded", msg.ValidatorAddress)
+	}
 
 	// Remove the validator from the active set with 0 consensus power.
-	val, err := ms.k.SetPOAPower(ctx, msg.ValidatorAddress, 0)
+	val, err = ms.k.SetPOAPower(ctx, msg.ValidatorAddress, 0)
 	if err != nil {
 		return nil, err
 	}
@@ -244,6 +233,40 @@ func (ms msgServer) UpdateStakingParams(ctx context.Context, msg *poa.MsgUpdateS
 		return nil, poa.ErrNotAnAuthority
 	}
 
+	prevStakingParams, err := ms.k.stakingKeeper.GetParams(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	// https://github.com/liftedinit/cosmos-sdk/blob/a877e3e8048a5acb07a0bff92bd8498cd24d1a01/x/staking/keeper/msg_server.go#L619-L642
+	// when min commission rate is updated, we need to update the commission rate of all validators
+	if !prevStakingParams.MinCommissionRate.Equal(msg.Params.MinCommissionRate) {
+		minRate := msg.Params.MinCommissionRate
+
+		vals, err := ms.k.stakingKeeper.GetAllValidators(ctx)
+		if err != nil {
+			return nil, err
+		}
+
+		blockTime := sdk.UnwrapSDKContext(ctx).BlockHeader().Time
+
+		for _, val := range vals {
+			// set the commission rate to min rate
+			if val.Commission.CommissionRates.Rate.LT(minRate) {
+				val.Commission.CommissionRates.Rate = minRate
+				// set the max rate to minRate if it is less than min rate
+				if val.Commission.CommissionRates.MaxRate.LT(minRate) {
+					val.Commission.CommissionRates.MaxRate = minRate
+				}
+
+				val.Commission.UpdateTime = blockTime
+				if err := ms.k.stakingKeeper.SetValidator(ctx, val); err != nil {
+					return nil, fmt.Errorf("failed to set validator after MinCommissionRate param change: %w", err)
+				}
+			}
+		}
+	}
+
 	stakingParams := stakingtypes.Params{
 		UnbondingTime:     msg.Params.UnbondingTime,
 		MaxValidators:     msg.Params.MaxValidators,
@@ -253,5 +276,9 @@ func (ms msgServer) UpdateStakingParams(ctx context.Context, msg *poa.MsgUpdateS
 		MinCommissionRate: msg.Params.MinCommissionRate,
 	}
 
+	if err := stakingParams.Validate(); err != nil {
+		return nil, err
+	}
+
 	return &poa.MsgUpdateStakingParamsResponse{}, ms.k.stakingKeeper.SetParams(ctx, stakingParams)
 }

keeper/msg_server_test.go

@@ -61,6 +61,52 @@ func TestUpdateStakingParams(t *testing.T) {
 	}
 }
 
+// Verifies that if the minimum commission rate is brought up, all validators with too low of a commission are updated.
+// i.e.:
+// - MinCommission is 0, change is now 1%, all validators below 1% are updated to 1%.
+// - MinCommission is 1%, but now changed back down to 0%. No changes.
+func TestUpdateStakingParamChangeValidatorMinCommission(t *testing.T) {
+	f := SetupTest(t, 2_000_000)
+	require := require.New(t)
+
+	// current staking params
+	sp, err := f.k.GetStakingKeeper().GetParams(f.ctx)
+	require.NoError(err)
+	require.True(sp.MinCommissionRate.Equal(sdkmath.LegacyZeroDec()))
+
+	// verify the validator is at the current minimum commission rate (0%)
+	vals, err := f.stakingKeeper.GetValidators(f.ctx, 1)
+	require.NoError(err)
+	require.True(vals[0].Commission.Rate.Equal(sp.MinCommissionRate))
+
+	// increase the min commission rate to 1%
+	p := poa.DefaultStakingParams()
+	p.MinCommissionRate = sdkmath.LegacyMustNewDecFromStr("0.01")
+	_, err = f.msgServer.UpdateStakingParams(f.ctx, &poa.MsgUpdateStakingParams{
+		Sender: f.authorityAddr,
+		Params: p,
+	})
+	require.NoError(err)
+
+	// valiadte the validator is now at 1% (the new updated mincommission)
+	vals, err = f.stakingKeeper.GetValidators(f.ctx, 1)
+	require.NoError(err)
+	require.True(vals[0].Commission.Rate.Equal(p.MinCommissionRate))
+
+	// set commission rate back to 0
+	p.MinCommissionRate = sdkmath.LegacyZeroDec()
+	_, err = f.msgServer.UpdateStakingParams(f.ctx, &poa.MsgUpdateStakingParams{
+		Sender: f.authorityAddr,
+		Params: p,
+	})
+	require.NoError(err)
+
+	// no updates made to the validator. It is still at 1%.
+	postChangeVals, err := f.stakingKeeper.GetValidators(f.ctx, 1)
+	require.NoError(err)
+	require.NotEqual(postChangeVals[0].Commission.Rate, p.MinCommissionRate)
+}
+
 func TestSetPowerAndCreateValidator(t *testing.T) {
 	f := SetupTest(t, 2_000_000)
 	require := require.New(t)
@@ -215,12 +261,12 @@ func TestRemoveValidator(t *testing.T) {
 	require.NoError(err)
 
 	for _, v := range vals {
-		power := 10_000_000
+		var power uint64 = 10_000_000
 
 		_, err = f.msgServer.SetPower(f.ctx, &poa.MsgSetPower{
 			Sender:           f.addrs[0].String(),
 			ValidatorAddress: v.OperatorAddress,
-			Power:            uint64(power),
+			Power:            power,
 			Unsafe:           true,
 		})
 		require.NoError(err)

keeper/pending.go

@@ -32,6 +32,12 @@ func (k Keeper) AddPendingValidator(ctx context.Context, newVal stakingtypes.Val
 		return err
 	}
 
+	for _, val := range vals.Validators {
+		if val.OperatorAddress == poaVal.OperatorAddress {
+			return poa.ErrValidatorAlreadyPending
+		}
+	}
+
 	vals.Validators = append(vals.Validators, poaVal)
 
 	return k.PendingValidators.Set(ctx, vals)

keeper/pending_test.go

@@ -0,0 +1,42 @@
+package keeper_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/strangelove-ventures/poa"
+)
+
+func TestAddPending(t *testing.T) {
+	f := SetupTest(t, 2_000_000)
+	require := require.New(t)
+
+	val := GenAcc()
+	valAddr := sdk.ValAddress(val.addr)
+	v := poa.ConvertPOAToStaking(CreateNewValidator(
+		"myval",
+		valAddr.String(),
+		val.valKey.PubKey(),
+		1_000_000,
+	))
+
+	// successful add
+	err := f.k.AddPendingValidator(f.ctx, v, val.valKey.PubKey())
+	require.NoError(err)
+
+	// duplicate (fails)
+	err = f.k.AddPendingValidator(f.ctx, v, val.valKey.PubKey())
+	require.Error(err)
+	require.Equal(poa.ErrValidatorAlreadyPending, err)
+
+	// remove pending
+	err = f.k.RemovePendingValidator(f.ctx, v.OperatorAddress)
+	require.NoError(err)
+
+	pending, err := f.k.GetPendingValidators(f.ctx)
+	require.NoError(err)
+	require.Empty(pending.Validators)
+}