Skip to content

Commit

Permalink
refactor(x/distribution)!: remove PreviousProposer and return early i…
Browse files Browse the repository at this point in the history
…f there are no fees to distribute (#20735)
  • Loading branch information
facundomedica authored Jul 4, 2024
1 parent b63d840 commit 01312df
Show file tree
Hide file tree
Showing 15 changed files with 149 additions and 343 deletions.
209 changes: 66 additions & 143 deletions api/cosmos/distribution/v1beta1/genesis.pulsar.go

Large diffs are not rendered by default.

8 changes: 0 additions & 8 deletions tests/integration/distribution/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
require.NoError(t, f.distrKeeper.Params.Set(f.sdkCtx, distrtypes.DefaultParams()))

delAddr := sdk.AccAddress(PKS[1].Address())
valConsAddr := sdk.ConsAddress(valConsPk0.Address())

valCommission := sdk.DecCoins{
sdk.NewDecCoinFromDec("mytoken", math.LegacyNewDec(5).Quo(math.LegacyNewDec(4))),
Expand Down Expand Up @@ -325,9 +324,6 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
}
height := f.app.LastBlockHeight()

_, err = f.distrKeeper.PreviousProposer.Get(f.sdkCtx)
assert.ErrorIs(t, err, collections.ErrNotFound)

for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -358,10 +354,6 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
assert.Assert(t, initBalance.IsAllLTE(curBalance))
}

prevProposerConsAddr, err := f.distrKeeper.PreviousProposer.Get(f.sdkCtx)
assert.NilError(t, err)
assert.Assert(t, prevProposerConsAddr.Empty() == false)
assert.DeepEqual(t, prevProposerConsAddr, valConsAddr)
var previousTotalPower int64
for _, vote := range f.sdkCtx.CometInfo().LastCommit.Votes {
previousTotalPower += vote.Validator.Power
Expand Down
1 change: 1 addition & 0 deletions x/distribution/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* [#20735](https://github.com/cosmos/cosmos-sdk/pull/20735) Remove PreviousProposer from the state machine.
* [#17657](https://github.com/cosmos/cosmos-sdk/pull/17657) Migrate community pool funds from `x/distribution` to `x/protocolpool`.
* [#17115](https://github.com/cosmos/cosmos-sdk/pull/17115) Migrate `PreviousProposer` to collections.
* [#18539](https://github.com/cosmos/cosmos-sdk/pull/18539) Introduce `FeePool.DecimalPool` to replace `FeePool.CommunityPool`, which temporarily holds fractional rewards until they are distributed to the community pool every 1000 blocks.
Expand Down
5 changes: 1 addition & 4 deletions x/distribution/keeper/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"cosmossdk.io/x/distribution/types"

"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
)

// BeginBlocker sets the proposer for determining distribution during endblock
Expand Down Expand Up @@ -38,7 +37,5 @@ func (k Keeper) BeginBlocker(ctx context.Context) error {
}
}

// record the proposer for when we payout on the next block
consAddr := sdk.ConsAddress(ci.ProposerAddress)
return k.PreviousProposer.Set(ctx, consAddr)
return nil
}
4 changes: 4 additions & 0 deletions x/distribution/keeper/allocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ func (k Keeper) AllocateTokens(ctx context.Context, totalPreviousPower int64, bo
// (and distributed to the previous proposer)
feeCollector := k.authKeeper.GetModuleAccount(ctx, k.feeCollectorName)
feesCollectedInt := k.bankKeeper.GetAllBalances(ctx, feeCollector.GetAddress())
// return early if no fees to distribute
if feesCollectedInt.Empty() {
return nil
}
feesCollected := sdk.NewDecCoinsFromCoins(feesCollectedInt...)

// transfer collected fees to the distribution module account
Expand Down
24 changes: 1 addition & 23 deletions x/distribution/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,6 @@ func (k Keeper) InitGenesis(ctx context.Context, data types.GenesisState) error
}
}

var previousProposer sdk.ConsAddress
if data.PreviousProposer != "" {
var err error
previousProposer, err = k.stakingKeeper.ConsensusAddressCodec().StringToBytes(data.PreviousProposer)
if err != nil {
return err
}
}

if err := k.PreviousProposer.Set(ctx, previousProposer); err != nil {
return err
}

for _, rew := range data.OutstandingRewards {
valAddr, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(rew.ValidatorAddress)
if err != nil {
Expand Down Expand Up @@ -177,11 +164,6 @@ func (k Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error)
return nil, err
}

pp, err := k.PreviousProposer.Get(ctx)
if err != nil {
return nil, err
}

outstanding := make([]types.ValidatorOutstandingRewardsRecord, 0)

err = k.ValidatorOutstandingRewards.Walk(ctx, nil, func(addr sdk.ValAddress, rewards types.ValidatorOutstandingRewards) (stop bool, err error) {
Expand Down Expand Up @@ -303,9 +285,5 @@ func (k Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error)
return nil, err
}

ppAddr, err := k.stakingKeeper.ConsensusAddressCodec().BytesToString(pp)
if err != nil {
return nil, err
}
return types.NewGenesisState(params, feePool, dwi, ppAddr, outstanding, acc, his, cur, dels, slashes), nil
return types.NewGenesisState(params, feePool, dwi, outstanding, acc, his, cur, dels, slashes), nil
}
2 changes: 0 additions & 2 deletions x/distribution/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ type Keeper struct {
ValidatorOutstandingRewards collections.Map[sdk.ValAddress, types.ValidatorOutstandingRewards]
// ValidatorHistoricalRewards key: valAddr+period | value: ValidatorHistoricalRewards
ValidatorHistoricalRewards collections.Map[collections.Pair[sdk.ValAddress, uint64], types.ValidatorHistoricalRewards]
PreviousProposer collections.Item[sdk.ConsAddress]
// ValidatorSlashEvents key: valAddr+height+period | value: ValidatorSlashEvent
ValidatorSlashEvents collections.Map[collections.Triple[sdk.ValAddress, uint64, uint64], types.ValidatorSlashEvent]

Expand Down Expand Up @@ -130,7 +129,6 @@ func NewKeeper(
collections.PairKeyCodec(sdk.LengthPrefixedAddressKey(sdk.ValAddressKey), sdk.LEUint64Key), //nolint: staticcheck // sdk.LengthPrefixedAddressKey is needed to retain state compatibility
codec.CollValue[types.ValidatorHistoricalRewards](cdc),
),
PreviousProposer: collections.NewItem(sb, types.ProposerKey, "previous_proposer", collcodec.KeyToValueCodec(sdk.ConsAddressKey)),
ValidatorSlashEvents: collections.NewMap(
sb,
types.ValidatorSlashEventPrefix,
Expand Down
33 changes: 3 additions & 30 deletions x/distribution/migrations/v4/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,46 +6,19 @@ import (

gogotypes "github.com/cosmos/gogoproto/types"

"cosmossdk.io/collections"
collcodec "cosmossdk.io/collections/codec"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/store"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
)

var (
OldProposerKey = []byte{0x01}
NewProposerKey = collections.NewPrefix(1)
)
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)
bz, err := store.Get(OldProposerKey)
if err != nil {
return err
}

if bz == nil {
// previous proposer not set, nothing to do
return nil
}

addrValue := gogotypes.BytesValue{}
err = cdc.Unmarshal(bz, &addrValue)
if err != nil {
return err
}

sb := collections.NewSchemaBuilder(env.KVStoreService)
prevProposer := collections.NewItem(sb, NewProposerKey, "previous_proposer", collcodec.KeyToValueCodec(sdk.ConsAddressKey))
_, err = sb.Build()
if err != nil {
return err
}

return prevProposer.Set(ctx, addrValue.GetValue())
return store.Delete(OldProposerKey)
}

// GetPreviousProposerConsAddr returns the proposer consensus address for the
Expand Down
15 changes: 4 additions & 11 deletions x/distribution/migrations/v4/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (

"github.com/stretchr/testify/require"

"cosmossdk.io/collections"
collcodec "cosmossdk.io/collections/codec"
"cosmossdk.io/core/log"
storetypes "cosmossdk.io/store/types"
"cosmossdk.io/x/distribution"
Expand All @@ -32,6 +30,7 @@ func TestMigration(t *testing.T) {
addr1 := secp256k1.GenPrivKey().PubKey().Address()
consAddr1 := sdk.ConsAddress(addr1)

// Set and check the previous proposer
err := v4.SetPreviousProposerConsAddr(ctx, storeService, cdc, consAddr1)
require.NoError(t, err)

Expand All @@ -42,13 +41,7 @@ func TestMigration(t *testing.T) {
err = v4.MigrateStore(ctx, env, cdc)
require.NoError(t, err)

sb := collections.NewSchemaBuilder(storeService)
prevProposer := collections.NewItem(sb, v4.NewProposerKey, "previous_proposer", collcodec.KeyToValueCodec(sdk.ConsAddressKey))
_, err = sb.Build()
require.NoError(t, err)

newAddr, err := prevProposer.Get(ctx)
require.NoError(t, err)

require.Equal(t, consAddr1, newAddr)
// Check that the previous proposer has been removed
_, err = v4.GetPreviousProposerConsAddr(ctx, storeService, cdc)
require.ErrorContains(t, err, "previous proposer not set")
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,6 @@ message GenesisState {
repeated DelegatorWithdrawInfo delegator_withdraw_infos = 3
[(gogoproto.nullable) = false, (amino.dont_omitempty) = true];

// fee_pool defines the previous proposer at genesis.
string previous_proposer = 4 [(cosmos_proto.scalar) = "cosmos.AddressString"];

// fee_pool defines the outstanding rewards of all validators at genesis.
repeated ValidatorOutstandingRewardsRecord outstanding_rewards = 5
[(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
Expand All @@ -152,4 +149,6 @@ message GenesisState {
// fee_pool defines the validator slash events at genesis.
repeated ValidatorSlashEventRecord validator_slash_events = 10
[(gogoproto.nullable) = false, (amino.dont_omitempty) = true];

reserved 4; // previous_proposer
}
3 changes: 0 additions & 3 deletions x/distribution/simulation/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ func NewDecodeStore(cdc codec.Codec) func(kvA, kvB kv.Pair) string {
cdc.MustUnmarshal(kvB.Value, &feePoolB)
return fmt.Sprintf("%v\n%v", feePoolA, feePoolB)

case bytes.Equal(kvA.Key[:1], types.ProposerKey):
return fmt.Sprintf("%v\n%v", sdk.ConsAddress(kvA.Value), sdk.ConsAddress(kvB.Value))

case bytes.Equal(kvA.Key[:1], types.ValidatorOutstandingRewardsPrefix):
var rewardsA, rewardsB types.ValidatorOutstandingRewards
cdc.MustUnmarshal(kvA.Value, &rewardsA)
Expand Down
7 changes: 2 additions & 5 deletions x/distribution/simulation/decoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ import (
)

var (
delPk1 = ed25519.GenPrivKey().PubKey()
valAddr1 = sdk.ValAddress(delPk1.Address())
consAddr1 = sdk.ConsAddress(delPk1.Address().Bytes())
delPk1 = ed25519.GenPrivKey().PubKey()
valAddr1 = sdk.ValAddress(delPk1.Address())
)

func TestDecodeDistributionStore(t *testing.T) {
Expand All @@ -38,7 +37,6 @@ func TestDecodeDistributionStore(t *testing.T) {
kvPairs := kv.Pairs{
Pairs: []kv.Pair{
{Key: types.FeePoolKey, Value: cdc.MustMarshal(&feePool)},
{Key: types.ProposerKey, Value: consAddr1.Bytes()},
{Key: types.GetValidatorSlashEventKeyPrefix(valAddr1, 13), Value: cdc.MustMarshal(&slashEvent)},
{Key: []byte{0x99}, Value: []byte{0x99}},
},
Expand All @@ -49,7 +47,6 @@ func TestDecodeDistributionStore(t *testing.T) {
expectedLog string
}{
{"FeePool", fmt.Sprintf("%v\n%v", feePool, feePool)},
{"Proposer", fmt.Sprintf("%v\n%v", consAddr1, consAddr1)},
{"ValidatorSlashEvent", fmt.Sprintf("%v\n%v", slashEvent, slashEvent)},
{"other", ""},
}
Expand Down
4 changes: 1 addition & 3 deletions x/distribution/types/genesis.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package types

func NewGenesisState(
params Params, fp FeePool, dwis []DelegatorWithdrawInfo, pp string, r []ValidatorOutstandingRewardsRecord,
params Params, fp FeePool, dwis []DelegatorWithdrawInfo, r []ValidatorOutstandingRewardsRecord,
acc []ValidatorAccumulatedCommissionRecord, historical []ValidatorHistoricalRewardsRecord,
cur []ValidatorCurrentRewardsRecord, dels []DelegatorStartingInfoRecord, slashes []ValidatorSlashEventRecord,
) *GenesisState {
return &GenesisState{
Params: params,
FeePool: fp,
DelegatorWithdrawInfos: dwis,
PreviousProposer: pp,
OutstandingRewards: r,
ValidatorAccumulatedCommissions: acc,
ValidatorHistoricalRewards: historical,
Expand All @@ -25,7 +24,6 @@ func DefaultGenesisState() *GenesisState {
FeePool: InitialFeePool(),
Params: DefaultParams(),
DelegatorWithdrawInfos: []DelegatorWithdrawInfo{},
PreviousProposer: "",
OutstandingRewards: []ValidatorOutstandingRewardsRecord{},
ValidatorAccumulatedCommissions: []ValidatorAccumulatedCommissionRecord{},
ValidatorHistoricalRewards: []ValidatorHistoricalRewardsRecord{},
Expand Down
Loading

0 comments on commit 01312df

Please sign in to comment.