Skip to content
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

Jump to bottom

refactor(x/distribution): audit QA (backport #21316) #21338

Merged
merged 1 commit into from
Aug 17, 2024
Merged

refactor(x/distribution): audit QA (backport #21316) #21338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions x/distribution/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ it can be updated with governance or the address with authority.
* Params: `0x09 | ProtocolBuffer(Params)`

```protobuf reference
https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/distribution.proto#L12-L42
https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/distribution.proto#L12-L44
```

## Begin Block
Expand Down Expand Up @@ -272,7 +272,7 @@ The withdraw address cannot be any of the module accounts. These accounts are bl
Response:

```protobuf reference
https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L49-L60
https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L62-L73
```

```go
Expand Down Expand Up @@ -324,7 +324,7 @@ The final calculated stake is equivalent to the actual staked coins in the deleg
Response:

```protobuf reference
https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L66-L77
https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L79-L90
```

### WithdrawValidatorCommission
Expand Down Expand Up @@ -368,7 +368,7 @@ func (k Keeper) initializeDelegation(ctx context.Context, val sdk.ValAddress, de
Distribution module params can be updated through `MsgUpdateParams`, which can be done using governance proposal and the signer will always be gov module account address.

```protobuf reference
https://github.com/cosmos/cosmos-sdk/blob/v0.47.0-rc1/proto/cosmos/distribution/v1beta1/tx.proto#L133-L147
https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/distribution/proto/cosmos/distribution/v1beta1/tx.proto#L159:L172
```

The message handling can fail if:
Expand Down
128 changes: 128 additions & 0 deletions x/distribution/keeper/allocation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,3 +375,131 @@ func TestAllocateTokensTruncation(t *testing.T) {
require.NoError(t, err)
require.True(t, val2OutstandingRewards.Rewards.IsValid())
}

func TestAllocateTokensToValidatorWithoutCommission(t *testing.T) {
ctrl := gomock.NewController(t)
key := storetypes.NewKVStoreKey(disttypes.StoreKey)
testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
cdcOpts := codectestutil.CodecOptions{}
encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, distribution.AppModule{})
ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()})

bankKeeper := distrtestutil.NewMockBankKeeper(ctrl)
stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl)
accountKeeper := distrtestutil.NewMockAccountKeeper(ctrl)

env := runtime.NewEnvironment(runtime.NewKVStoreService(key), coretesting.NewNopLogger())

valCodec := address.NewBech32Codec("cosmosvaloper")

accountKeeper.EXPECT().GetModuleAddress("distribution").Return(distrAcc.GetAddress())
stakingKeeper.EXPECT().ValidatorAddressCodec().Return(valCodec).AnyTimes()

authorityAddr, err := cdcOpts.GetAddressCodec().BytesToString(authtypes.NewModuleAddress("gov"))
require.NoError(t, err)

distrKeeper := keeper.NewKeeper(
encCfg.Codec,
env,
accountKeeper,
bankKeeper,
stakingKeeper,
testCometService,
"fee_collector",
authorityAddr,
)

// create validator with 0% commission
operatorAddr, err := stakingKeeper.ValidatorAddressCodec().BytesToString(valConsPk0.Address())
require.NoError(t, err)
val, err := distrtestutil.CreateValidator(valConsPk0, operatorAddr, math.NewInt(100))
require.NoError(t, err)
val.Commission = stakingtypes.NewCommission(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0))
stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk0)).Return(val, nil).AnyTimes()

// allocate tokens
tokens := sdk.DecCoins{
{Denom: sdk.DefaultBondDenom, Amount: math.LegacyNewDec(10)},
}
require.NoError(t, distrKeeper.AllocateTokensToValidator(ctx, val, tokens))

// check commission
var expectedCommission sdk.DecCoins = nil

valBz, err := valCodec.StringToBytes(val.GetOperator())
require.NoError(t, err)

valCommission, err := distrKeeper.ValidatorsAccumulatedCommission.Get(ctx, valBz)
require.NoError(t, err)
require.Equal(t, expectedCommission, valCommission.Commission)

// check current rewards
expectedRewards := tokens

currentRewards, err := distrKeeper.ValidatorCurrentRewards.Get(ctx, valBz)
require.NoError(t, err)
require.Equal(t, expectedRewards, currentRewards.Rewards)
}

func TestAllocateTokensWithZeroTokens(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

key := storetypes.NewKVStoreKey(disttypes.StoreKey)
testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
cdcOpts := codectestutil.CodecOptions{}
encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, distribution.AppModule{})
ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()})

bankKeeper := distrtestutil.NewMockBankKeeper(ctrl)
stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl)
accountKeeper := distrtestutil.NewMockAccountKeeper(ctrl)

env := runtime.NewEnvironment(runtime.NewKVStoreService(key), coretesting.NewNopLogger())

valCodec := address.NewBech32Codec("cosmosvaloper")

accountKeeper.EXPECT().GetModuleAddress("distribution").Return(distrAcc.GetAddress())
stakingKeeper.EXPECT().ValidatorAddressCodec().Return(valCodec).AnyTimes()

authorityAddr, err := cdcOpts.GetAddressCodec().BytesToString(authtypes.NewModuleAddress("gov"))
require.NoError(t, err)

distrKeeper := keeper.NewKeeper(
encCfg.Codec,
env,
accountKeeper,
bankKeeper,
stakingKeeper,
testCometService,
"fee_collector",
authorityAddr,
)

// create validator with 50% commission
operatorAddr, err := stakingKeeper.ValidatorAddressCodec().BytesToString(valConsPk0.Address())
require.NoError(t, err)
val, err := distrtestutil.CreateValidator(valConsPk0, operatorAddr, math.NewInt(100))
require.NoError(t, err)
val.Commission = stakingtypes.NewCommission(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0))
stakingKeeper.EXPECT().ValidatorByConsAddr(gomock.Any(), sdk.GetConsAddress(valConsPk0)).Return(val, nil).AnyTimes()

// allocate zero tokens
tokens := sdk.DecCoins{}
require.NoError(t, distrKeeper.AllocateTokensToValidator(ctx, val, tokens))

// check commission
var expected sdk.DecCoins = nil

valBz, err := valCodec.StringToBytes(val.GetOperator())
require.NoError(t, err)

valCommission, err := distrKeeper.ValidatorsAccumulatedCommission.Get(ctx, valBz)
require.NoError(t, err)
require.Equal(t, expected, valCommission.Commission)

// check current rewards
currentRewards, err := distrKeeper.ValidatorCurrentRewards.Get(ctx, valBz)
require.NoError(t, err)
require.Equal(t, expected, currentRewards.Rewards)
}
2 changes: 1 addition & 1 deletion x/distribution/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type Keeper struct {
DelegatorStartingInfo collections.Map[collections.Pair[sdk.ValAddress, sdk.AccAddress], types.DelegatorStartingInfo]
// ValidatorsAccumulatedCommission key: valAddr | value: ValidatorAccumulatedCommission
ValidatorsAccumulatedCommission collections.Map[sdk.ValAddress, types.ValidatorAccumulatedCommission]
// ValidatorOutstandingRewards key: valAddr | value: ValidatorOustandingRewards
// ValidatorOutstandingRewards key: valAddr | value: ValidatorOutstandingRewards
ValidatorOutstandingRewards collections.Map[sdk.ValAddress, types.ValidatorOutstandingRewards]
// ValidatorHistoricalRewards key: valAddr+period | value: ValidatorHistoricalRewards
ValidatorHistoricalRewards collections.Map[collections.Pair[sdk.ValAddress, uint64], types.ValidatorHistoricalRewards]
Expand Down
12 changes: 8 additions & 4 deletions x/distribution/keeper/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

const (
maxReferenceCount = 2
)

// initialize rewards for a new validator
func (k Keeper) initializeValidator(ctx context.Context, val sdk.ValidatorI) error {
valBz, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(val.GetOperator())
Expand Down Expand Up @@ -126,8 +130,8 @@ func (k Keeper) incrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr
}

historical.ReferenceCount++
if historical.ReferenceCount > 2 {
panic("reference count should never exceed 2")
if historical.ReferenceCount > maxReferenceCount {
return fmt.Errorf("reference count should never exceed %d", maxReferenceCount)
}
return k.ValidatorHistoricalRewards.Set(ctx, collections.Join(valAddr, period), historical)
}
Expand All @@ -140,7 +144,7 @@ func (k Keeper) decrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr
}

if historical.ReferenceCount == 0 {
panic("cannot set negative reference count")
return fmt.Errorf("cannot set negative reference count")
}
historical.ReferenceCount--
if historical.ReferenceCount == 0 {
Expand All @@ -152,7 +156,7 @@ func (k Keeper) decrementReferenceCount(ctx context.Context, valAddr sdk.ValAddr

func (k Keeper) updateValidatorSlashFraction(ctx context.Context, valAddr sdk.ValAddress, fraction math.LegacyDec) error {
if fraction.GT(math.LegacyOneDec()) || fraction.IsNegative() {
panic(fmt.Sprintf("fraction must be >=0 and <=1, current fraction: %v", fraction))
return fmt.Errorf("fraction must be >=0 and <=1, current fraction: %v", fraction)
}

headerinfo := k.HeaderService.HeaderInfo(ctx)
Expand Down
16 changes: 8 additions & 8 deletions x/distribution/migrations/v4/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ import (
var OldProposerKey = []byte{0x01}

// MigrateStore removes the last proposer from store.
func MigrateStore(ctx context.Context, env appmodule.Environment, cdc codec.BinaryCodec) error {
store := env.KVStoreService.OpenKVStore(ctx)
return store.Delete(OldProposerKey)
func MigrateStore(ctx context.Context, env appmodule.Environment, _ codec.BinaryCodec) error {
kvStore := env.KVStoreService.OpenKVStore(ctx)
return kvStore.Delete(OldProposerKey)
}

// GetPreviousProposerConsAddr returns the proposer consensus address for the
// current block.
func GetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStoreService, cdc codec.BinaryCodec) (sdk.ConsAddress, error) {
store := storeService.OpenKVStore(ctx)
bz, err := store.Get(OldProposerKey)
kvStore := storeService.OpenKVStore(ctx)
bz, err := kvStore.Get(OldProposerKey)
if err != nil {
return nil, err
}
Expand All @@ -43,9 +43,9 @@ func GetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStore
return addrValue.GetValue(), nil
}

// set the proposer public key for this block
// SetPreviousProposerConsAddr set the proposer public key for this block.
func SetPreviousProposerConsAddr(ctx context.Context, storeService store.KVStoreService, cdc codec.BinaryCodec, consAddr sdk.ConsAddress) error {
store := storeService.OpenKVStore(ctx)
kvStore := storeService.OpenKVStore(ctx)
bz := cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr})
return store.Set(OldProposerKey, bz)
return kvStore.Set(OldProposerKey, bz)
}
Loading