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

refactor(distribution)!: add cometinfo #20588

Merged
merged 3 commits into from
Jun 9, 2024
Merged

refactor(distribution)!: add cometinfo #20588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f71989
make changes to distribution to avoid sdkctx
tac0turtle Jun 6, 2024
fd7cd4c
add changelog
tac0turtle Jun 6, 2024
d7e60dd
fix test
tac0turtle Jun 9, 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
2 changes: 1 addition & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ func NewSimApp(

app.PoolKeeper = poolkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[pooltypes.StoreKey]), logger.With(log.ModuleKey, "x/protocolpool")), app.AuthKeeper, app.BankKeeper, app.StakingKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String())

app.DistrKeeper = distrkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[distrtypes.StoreKey]), logger.With(log.ModuleKey, "x/distribution")), app.AuthKeeper, app.BankKeeper, app.StakingKeeper, app.PoolKeeper, authtypes.FeeCollectorName, authtypes.NewModuleAddress(govtypes.ModuleName).String())
app.DistrKeeper = distrkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[distrtypes.StoreKey]), logger.With(log.ModuleKey, "x/distribution")), app.AuthKeeper, app.BankKeeper, app.StakingKeeper, app.PoolKeeper, cometService, authtypes.FeeCollectorName, authtypes.NewModuleAddress(govtypes.ModuleName).String())

app.SlashingKeeper = slashingkeeper.NewKeeper(runtime.NewEnvironment(runtime.NewKVStoreService(keys[slashingtypes.StoreKey]), logger.With(log.ModuleKey, "x/slashing")),
appCodec, legacyAmino, app.StakingKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(),
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/distribution/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func initFixture(t *testing.T) *fixture {
poolKeeper := poolkeeper.NewKeeper(cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[pooltypes.StoreKey]), log.NewNopLogger()), accountKeeper, bankKeeper, stakingKeeper, authority.String())

distrKeeper := distrkeeper.NewKeeper(
cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[distrtypes.StoreKey]), logger), accountKeeper, bankKeeper, stakingKeeper, poolKeeper, distrtypes.ModuleName, authority.String(),
cdc, runtime.NewEnvironment(runtime.NewKVStoreService(keys[distrtypes.StoreKey]), logger), accountKeeper, bankKeeper, stakingKeeper, poolKeeper, cometService, distrtypes.ModuleName, authority.String(),
)

authModule := auth.NewAppModule(cdc, accountKeeper, acctsModKeeper, authsims.RandomGenesisAccounts)
Expand All @@ -163,6 +163,7 @@ func initFixture(t *testing.T) *fixture {
},
},
},
ProposerAddress: valConsAddr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The addition of ProposerAddress is not followed by any further usage or assertions in the test suite. This indicates that the new field might not be utilized correctly.

  • Ensure that ProposerAddress is properly handled and validated in the test suite.
Analysis chain

Addition of ProposerAddress to the context is a significant change. Ensure that this new field is utilized correctly throughout the test suite.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `ProposerAddress` is utilized correctly in the test suite.

# Test: Search for the usage of `ProposerAddress`. Expect: Proper handling and assertions related to this field.
rg --type go 'ProposerAddress' tests/integration/distribution/keeper/msg_server_test.go

Length of output: 119

Script:

#!/bin/bash
# Description: Verify that `ProposerAddress` is utilized correctly throughout the test suite.

# Test: Search for the usage of `ProposerAddress` across the entire test suite. Expect: Proper handling and assertions related to this field.
rg --type go 'ProposerAddress' tests/integration/distribution/keeper/

Length of output: 158

})

integrationApp := integration.NewIntegrationApp(ctx, logger, keys, cdc,
Expand Down
4 changes: 3 additions & 1 deletion x/distribution/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes


* [#20588](https://github.com/cosmos/cosmos-sdk/pull/20588) `x/distribution` now takes cometService in order to get consensus related information.
* [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method
* `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`.
* `NewGenesisState` now takes a string as argument instead of `sdk.ConsAddress`.
Expand Down Expand Up @@ -74,4 +76,4 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* [#19301](https://github.com/cosmos/cosmos-sdk/pull/19301) Fix vulnerability in `incrementReferenceCount` in distribution.
* [#19301](https://github.com/cosmos/cosmos-sdk/pull/19301) Fix vulnerability in `incrementReferenceCount` in distribution.
9 changes: 6 additions & 3 deletions x/distribution/depinject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package distribution
import (
modulev1 "cosmossdk.io/api/cosmos/distribution/module/v1"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/comet"
"cosmossdk.io/depinject"
"cosmossdk.io/depinject/appconfig"
authtypes "cosmossdk.io/x/auth/types"
Expand All @@ -27,9 +28,10 @@ func init() {
type ModuleInputs struct {
depinject.In

Config *modulev1.Module
Environment appmodule.Environment
Cdc codec.Codec
Config *modulev1.Module
Environment appmodule.Environment
Cdc codec.Codec
CometService comet.Service

AccountKeeper types.AccountKeeper
BankKeeper types.BankKeeper
Expand Down Expand Up @@ -69,6 +71,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
in.BankKeeper,
in.StakingKeeper,
in.PoolKeeper,
in.CometService,
feeCollectorName,
authorityAddr,
)
Expand Down
16 changes: 10 additions & 6 deletions x/distribution/keeper/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"context"

"cosmossdk.io/x/distribution/types"

"github.com/cosmos/cosmos-sdk/telemetry"
Expand All @@ -10,31 +12,33 @@
// BeginBlocker sets the proposer for determining distribution during endblock
// and distribute rewards for the previous block.
// TODO: use context.Context after including the comet service
func (k Keeper) BeginBlocker(ctx sdk.Context) error {
func (k Keeper) BeginBlocker(ctx context.Context) error {
defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.Now(), telemetry.MetricKeyBeginBlocker)

// determine the total power signing the block
var previousTotalPower int64
for _, vote := range ctx.CometInfo().LastCommit.Votes {
header := k.HeaderService.HeaderInfo(ctx)
ci := k.cometService.CometInfo(ctx)
for _, vote := range ci.LastCommit.Votes {
previousTotalPower += vote.Validator.Power
}

// TODO this is Tendermint-dependent
// ref https://github.com/cosmos/cosmos-sdk/issues/3095
if ctx.BlockHeight() > 1 {
if err := k.AllocateTokens(ctx, previousTotalPower, ctx.CometInfo().LastCommit.Votes); err != nil {
if header.Height > 1 {
if err := k.AllocateTokens(ctx, previousTotalPower, ci.LastCommit.Votes); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling here is appropriate, but consider adding telemetry or logging to capture the error details for better observability.

return err
}

// every 1000 blocks send whole coins from decimal pool to community pool
if ctx.BlockHeight()%1000 == 0 {
if header.Height%1000 == 0 {
if err := k.sendDecimalPoolToCommunityPool(ctx); err != nil {
return err
}
}
}

// record the proposer for when we payout on the next block
consAddr := sdk.ConsAddress(ctx.BlockHeader().ProposerAddress)
consAddr := sdk.ConsAddress(ci.ProposerAddress)
return k.PreviousProposer.Set(ctx, consAddr)
}
15 changes: 15 additions & 0 deletions x/distribution/keeper/allocation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -28,6 +29,17 @@ import (
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
)

var _ comet.Service = (*emptyCometService)(nil)

type emptyCometService struct{}

// CometInfo implements comet.Service.
func (e *emptyCometService) CometInfo(context.Context) comet.Info {
return comet.Info{}
}

var testCometService = &emptyCometService{}

func TestAllocateTokensToValidatorWithCommission(t *testing.T) {
ctrl := gomock.NewController(t)
key := storetypes.NewKVStoreKey(disttypes.StoreKey)
Expand Down Expand Up @@ -58,6 +70,7 @@ func TestAllocateTokensToValidatorWithCommission(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -124,6 +137,7 @@ func TestAllocateTokensToManyValidators(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -264,6 +278,7 @@ func TestAllocateTokensTruncation(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down
9 changes: 9 additions & 0 deletions x/distribution/keeper/delegation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ func TestCalculateRewardsBasic(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The testCometService is used in x/distribution/keeper/delegation_test.go but is not initialized or imported within this file. It is defined and initialized in x/distribution/keeper/allocation_test.go. This indicates that testCometService is not properly initialized before use in delegation_test.go.

  • Ensure testCometService is properly initialized in x/distribution/keeper/delegation_test.go.
Analysis chain

Ensure testCometService is properly initialized before use in tests.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `testCometService` is properly initialized in all test functions where it's used.

# Test: Search for the initialization of `testCometService`. Expect: Initialization before use in each test function.
rg --type go 'testCometService ='

Length of output: 120

Script:

#!/bin/bash
# Description: Verify that `testCometService` is properly initialized in the test file `x/distribution/keeper/delegation_test.go`.

# Test: Search for the initialization of `testCometService` in `x/distribution/keeper/delegation_test.go`.
rg --type go 'testCometService =' x/distribution/keeper/delegation_test.go

Length of output: 76

Script:

#!/bin/bash
# Description: Verify the usage and initialization of `testCometService` in `x/distribution/keeper/delegation_test.go`.

# Test: Search for the usage of `testCometService` in `x/distribution/keeper/delegation_test.go`.
rg --type go 'testCometService' x/distribution/keeper/delegation_test.go

# Test: Search for the initialization of `testCometService` in setup functions or imports in `x/distribution/keeper/delegation_test.go`.
rg --type go 'func.*setup' x/distribution/keeper/delegation_test.go
rg --type go 'import' x/distribution/keeper/delegation_test.go

Length of output: 394

Script:

#!/bin/bash
# Description: Verify the import of `testCometService` in `x/distribution/keeper/delegation_test.go`.

# Test: Search for the import of `testCometService` in `x/distribution/keeper/delegation_test.go`.
rg --type go 'testCometService' x/distribution/keeper/delegation_test.go

Length of output: 252

Script:

#!/bin/bash
# Description: Verify the definition of `testCometService` in the codebase.

# Test: Search for the definition of `testCometService` in the codebase.
rg --type go 'var testCometService'

Length of output: 122

"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -167,6 +168,7 @@ func TestCalculateRewardsAfterSlash(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -283,6 +285,7 @@ func TestCalculateRewardsAfterManySlashes(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -420,6 +423,7 @@ func TestCalculateRewardsMultiDelegator(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -530,6 +534,7 @@ func TestWithdrawDelegationRewardsBasic(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -618,6 +623,7 @@ func TestCalculateRewardsAfterManySlashesInSameBlock(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -747,6 +753,7 @@ func TestCalculateRewardsMultiDelegatorMultiSlash(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -900,6 +907,7 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down Expand Up @@ -1114,6 +1122,7 @@ func Test100PercentCommissionReward(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
"fee_collector",
authorityAddr,
)
Expand Down
13 changes: 11 additions & 2 deletions x/distribution/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"cosmossdk.io/collections"
collcodec "cosmossdk.io/collections/codec"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/comet"
"cosmossdk.io/core/event"
errorsmod "cosmossdk.io/errors"
"cosmossdk.io/x/distribution/types"
Expand All @@ -21,6 +22,8 @@ import (
type Keeper struct {
appmodule.Environment

cometService comet.Service

cdc codec.BinaryCodec
authKeeper types.AccountKeeper
bankKeeper types.BankKeeper
Expand Down Expand Up @@ -57,8 +60,13 @@ type Keeper struct {

// NewKeeper creates a new distribution Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, env appmodule.Environment,
ak types.AccountKeeper, bk types.BankKeeper, sk types.StakingKeeper, pk types.PoolKeeper,
cdc codec.BinaryCodec,
env appmodule.Environment,
ak types.AccountKeeper,
bk types.BankKeeper,
sk types.StakingKeeper,
pk types.PoolKeeper,
cometService comet.Service,
feeCollectorName, authority string,
) Keeper {
// ensure distribution module account is set
Expand All @@ -69,6 +77,7 @@ func NewKeeper(
sb := collections.NewSchemaBuilder(env.KVStoreService)
k := Keeper{
Environment: env,
cometService: cometService,
cdc: cdc,
authKeeper: ak,
bankKeeper: bk,
Expand Down
1 change: 1 addition & 0 deletions x/distribution/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ func initFixture(t *testing.T) (sdk.Context, []sdk.AccAddress, keeper.Keeper, de
bankKeeper,
stakingKeeper,
poolKeeper,
testCometService,
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The integration of testCometService in initFixture is not consistently applied across all instances. Specifically, the following files need attention:

  • x/feegrant/keeper/genesis_test.go
  • x/circuit/keeper/keeper_test.go
  • x/circuit/keeper/query_test.go
  • x/circuit/keeper/msg_server_test.go
  • x/circuit/ante/circuit_test.go
  • tests/integration/staking/keeper/genesis_test.go
  • tests/integration/staking/keeper/vote_extensions_test.go
  • tests/integration/staking/keeper/validator_test.go
  • tests/integration/staking/keeper/slash_test.go
  • tests/integration/staking/keeper/unbonding_test.go
  • tests/integration/staking/keeper/msg_server_test.go
  • tests/integration/staking/keeper/grpc_query_test.go
  • tests/integration/staking/keeper/common_test.go
  • tests/integration/staking/keeper/delegation_test.go
  • tests/integration/runtime/query_test.go
  • tests/integration/slashing/keeper/keeper_test.go
  • tests/integration/gov/keeper/tally_test.go
  • tests/integration/gov/keeper/grpc_query_test.go
  • tests/integration/gov/keeper/keeper_test.go
  • tests/integration/distribution/keeper/msg_server_test.go
  • tests/integration/distribution/keeper/grpc_query_test.go
  • tests/integration/evidence/keeper/infraction_test.go
  • tests/integration/auth/keeper/msg_server_test.go
  • collections/genesis_test.go
  • client/v2/autocli/query_test.go
  • client/v2/autocli/msg_test.go
  • client/v2/autocli/common_test.go

Ensure these files are updated to include the testCometService parameter in the initFixture function.

Analysis chain

The integration of testCometService in initFixture aligns with the PR's objective to incorporate cometService. Ensure all dependent tests are updated to handle this new parameter.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all tests using `initFixture` have been updated to include `testCometService`.

# Test: Search for the function usage without the new parameter. Expect: No occurrences.
rg --type go $'initFixture\(' | grep -v 'testCometService'

Length of output: 9095

"fee_collector",
authorityAddr,
)
Expand Down
9 changes: 9 additions & 0 deletions x/distribution/migrations/v4/migrate_funds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"

"cosmossdk.io/core/comet"
"cosmossdk.io/log"
storetypes "cosmossdk.io/store/types"
"cosmossdk.io/x/auth"
Expand All @@ -31,6 +32,13 @@ import (
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
)

type emptyCometService struct{}

// CometInfo implements comet.Service.
func (e *emptyCometService) CometInfo(context.Context) comet.Info {
return comet.Info{}
}

func TestFundsMigration(t *testing.T) {
keys := storetypes.NewKVStoreKeys(
authtypes.StoreKey, banktypes.StoreKey, disttypes.StoreKey,
Expand Down Expand Up @@ -90,6 +98,7 @@ func TestFundsMigration(t *testing.T) {
bankKeeper,
stakingKeeper,
poolKeeper,
&emptyCometService{},
disttypes.ModuleName,
authority,
)
Expand Down
Loading