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 #21316

Merged
merged 12 commits into from
Aug 17, 2024
Merged

refactor(x/distribution): audit QA #21316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2f1bcec
refactor(x/distribution): audit QA
lucaslopezf Aug 15, 2024
29be888
Linting issues
lucaslopezf Aug 16, 2024
2756bab
Linting issues
lucaslopezf Aug 16, 2024
9a1da06
Merge branch 'main' into lucas/distribution-audit
lucaslopezf Aug 16, 2024
de2f457
feat: export genesis in simapp v2 (#21199)
randygrok Aug 16, 2024
b9c41e9
chore: fix spelling errors (#21327)
github-prbot Aug 16, 2024
9ab0a5e
refactor(x/mint): v0.52 audit x/mint (#21301)
JulianToledano Aug 16, 2024
81c30bb
merge
lucaslopezf Aug 16, 2024
08d22e5
fix(x/authz): bring back msg response in `DispatchActions` (#21044)
julienrbrt Aug 16, 2024
c67fbb4
feat(schema): specify JSON mapping (#21243)
aaronc Aug 16, 2024
bd8bd34
Merge branch 'main' into lucas/distribution-audit
lucaslopezf Aug 16, 2024
86b23ec
Solve some comments
lucaslopezf Aug 16, 2024
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, but I think we should be consistent

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant that has been there forever, not sure either, I guess it's fine. Maybe it gives us an opportunity to add a small comment on why it's 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I didn't explain well hehe, I talked about the panic replace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the function returning an error before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the function has three returns: two of them return an error, and this one returned a panic. That's why I think we should return an error to maintain consistency. However, I'm a bit confused about when we should use panic and when we shouldn't, or if this is a technical debt we've had from before

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can look at the file history to see when it happened then we can easily know if it was on purpose on not. I haven't checked the code path, mostly returning an error or panicking results to the same thing (since v0.47) while before we had to panic, so it is likely that it is just something that should have been refactored but haven't but it would be good to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the history, the panic has been there since 01/23/19, from the very first time the function was created. Therefore, I think the refactor is fine. Now, just to understand, so today in the SDK it's the same to return panic as to return an error, but we are standardizing everything to return errors, right?

}
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