feat(distribution): add fee-distribution and avs-reward distribution #117
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe update integrates a new fee distribution module into the Exocore application, enhancing its reward and incentive mechanisms. Key modifications include the introduction of a token allocation system for distributing fees among validators and stakers, adjustments to commission rates for operators, and improvements to the testing framework to support these changes. Additionally, various interfaces and keeper functions have been updated to facilitate better interactions within the system. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mikebraver
changed the title
mikebraver
force-pushed
the
distribution
branch
4 times, most recently
from
df8aa3a to
1f05b2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
proto/exocore/avs/tx.proto
Outdated
| string epoch_identifier = 8; | ||
| // registered operator of avs | ||
| repeated string operator_address = 9; | ||
|
|
||
| // Effective current epoch, accounting for current_epoch + 1 | ||
| // and current_epoch is the integer identifier of the epoch module | ||
| int64 starting_epoch = 10; | ||
| // asset_reward_commission_epoch_basis is the avs reward distribution based on asset per eopch end. | ||
| map<string, int64> asset_reward_amount_epoch_basis = 11; |
There was a problem hiding this comment.
map in message might cause error when parse within cli, better to change to list(repeated).
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (3)
testutil/keeper/feedistribute.go (2)
32-34: Improve readability by grouping related initializations.Group the codec and registry initializations together for better readability.
- registry := codectypes.NewInterfaceRegistry() - cdc := codec.NewProtoCodec(registry) encCfg := moduletestutil.MakeTestEncodingConfig(auth.AppModule{}, bank.AppModule{}, distribution.AppModule{}) + registry := codectypes.NewInterfaceRegistry() + cdc := codec.NewProtoCodec(registry)
54-55: Ensure default parameters are correctly set.The
NewParamsfunction in thefeedistributionmodule returns an emptyParamsstruct. This might not correctly initialize the default parameters for the fee distribution keeper.
x/feedistribution/types/params.go: func NewParams() Params { return Params{} }Please verify and ensure that the
Paramsstruct is initialized with the appropriate default values.Analysis chain
Ensure default parameters are correctly set.
Verify that
types.DefaultParams()returns the expected default parameters for the fee distribution keeper.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default parameters for the fee distribution keeper. # Test: Search for the definition of DefaultParams. Expect: Correct default parameters. rg --type go 'func DefaultParams' -A 10Length of output: 4417
Script:
#!/bin/bash # Description: Verify the NewParams function in the feedistribution module. # Search for the definition of NewParams in the feedistribution types file. rg --type go 'func NewParams' -A 10 x/feedistribution/types/params.goLength of output: 227
x/feedistribution/keeper/keeper.go (1)
50-57: Initialize all required fields in the Keeper struct.The
NewKeeperfunction inx/feedistribution/keeper/keeper.godoes not initialize theauthKeeper,epochsKeeper, andpoolKeeperfields in theKeeperstruct. Ensure these fields are properly initialized in theNewKeeperfunction.
x/feedistribution/keeper/keeper.go,NewKeeperfunction:return Keeper{ cdc: cdc, authority: authority, storeKey: storeKey, logger: logger, bankKeeper: bankKeeper, // Add missing initializations here authKeeper: authKeeper, epochsKeeper: epochsKeeper, poolKeeper: poolKeeper, }Analysis chain
Initialize all required fields in the Keeper struct.
Ensure that all required fields, such as
authKeeper,epochsKeeper, andpoolKeeper, are initialized in theNewKeeperfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all required fields are initialized in the Keeper struct. # Test: Search for the Keeper struct definition. Expect: All fields are initialized. rg --type go 'type Keeper struct' -A 20Length of output: 6877
Script:
#!/bin/bash # Description: Verify that all required fields are initialized in the Keeper struct within x/feedistribution/keeper/keeper.go. # Test: Search for the Keeper struct definition in x/feedistribution/keeper/keeper.go. Expect: All fields such as authKeeper, epochsKeeper, and poolKeeper are initialized. rg 'type Keeper struct' -A 20 x/feedistribution/keeper/keeper.goLength of output: 66
Script:
#!/bin/bash # Description: Verify that all required fields are initialized in the Keeper struct within x/feedistribution/keeper/keeper.go. # Test: Search for the Keeper struct definition in x/feedistribution/keeper/keeper.go and check the initialization of fields. rg 'Keeper struct' -A 30 x/feedistribution/keeper/keeper.goLength of output: 1215
Script:
#!/bin/bash # Description: Verify that all required fields are initialized in the NewKeeper function within x/feedistribution/keeper/keeper.go. # Test: Search for the NewKeeper function definition and its implementation in x/feedistribution/keeper/keeper.go. rg 'func NewKeeper' -A 30 x/feedistribution/keeper/keeper.goLength of output: 747
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumx/feedistribution/types/distribution.pb.gois excluded by!**/*.pb.gox/feedistribution/types/params.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- go.mod (11 hunks)
- proto/exocore/feedistribute/distribution.proto (1 hunks)
- proto/exocore/feedistribute/params.proto (1 hunks)
- testutil/keeper/feedistribute.go (1 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/keeper.go (1 hunks)
- x/feedistribution/keeper/msg_update_params.go (1 hunks)
- x/feedistribution/keeper/params.go (1 hunks)
- x/feedistribution/types/expected_keepers.go (1 hunks)
Additional context used
GitHub Check: lint
proto/exocore/feedistribute/params.proto
[failure] 12-12:
symbol "exocore.feedistribute.Params" already defined at exocore/feedistribute/distribution.proto:13:9
[failure] 18-18:
symbol "exocore.feedistribute.Params.community_tax" already defined at exocore/feedistribute/distribution.proto:16:10
Additional comments not posted (20)
x/feedistribution/keeper/msg_update_params.go (2)
13-14: Review authority validation and error message.The authority validation is correct, and the error message is informative.
17-20: LGTM! Parameter setting logic is correct.The parameter setting logic is correct and handles errors properly.
x/feedistribution/keeper/params.go (2)
9-17: LGTM! Parameter retrieval logic is correct.The function correctly retrieves and unmarshals parameters from the store.
19-25: LGTM! Parameter setting logic is correct.The function correctly marshals and sets parameters in the store.
testutil/keeper/feedistribute.go (1)
57-57: LGTM!The function correctly initializes the fee distribution keeper and returns the keeper instance and context.
x/feedistribution/keeper/keeper.go (2)
60-63: LGTM!The function correctly returns the authority string from the
Keeperstruct.
65-68: LGTM!The function correctly returns a module-specific logger with the module name.
x/feedistribution/types/expected_keepers.go (5)
10-13: LGTM!The
EpochsKeeperinterface correctly defines the method for getting epoch information.
18-26: LGTM!The
AccountKeeperinterface correctly defines methods for managing accounts.
28-41: LGTM!The
BankKeeperinterface correctly defines methods for managing bank operations.
49-55: LGTM!The
PoolKeeperinterface correctly defines methods for managing pool balances.
15-16: Verify the purpose of the empty interface.The
FeeDistributionHooksinterface does not include any methods. Verify if this is intentional or if methods are missing.x/feedistribution/keeper/allocation.go (1)
48-56: Check for potential issues withSetToDistribute.The comment
// TODO: this should be distribution module accountsuggests that there might be a temporary or incorrect implementation in place. It's important to resolve this TODO before moving to production.Would you like me to open a GitHub issue to ensure this TODO is addressed in a future PR?
proto/exocore/feedistribute/distribution.proto (5)
13-19: LGTM!The
Paramsmessage is well-defined and doesn't contain any issues.
33-40: LGTM!The
ValidatorHistoricalRewardsmessage is well-defined and doesn't contain any issues.
45-52: LGTM!The
ValidatorCurrentRewardsmessage is well-defined and doesn't contain any issues.
56-62: LGTM!The
ValidatorAccumulatedCommissionmessage is well-defined and doesn't contain any issues.
66-72: LGTM!The
ValidatorOutstandingRewardsmessage is well-defined and doesn't contain any issues.go.mod (2)
Line range hint
12-47:
LGTM!The added dependencies, including
github.com/bufbuild/buf,golang.org/x/tools,google.golang.org/grpc/cmd/protoc-gen-go-grpc, and others, are appropriate and do not introduce any conflicts or issues.
Line range hint
80-257:
LGTM!The added indirect dependencies, including
github.com/docker/distribution,go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc, and others, are appropriate and do not introduce any conflicts or issues.
| "github.com/ExocoreNetwork/exocore/x/feedistribution/types" | ||
| ) | ||
|
|
||
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { |
There was a problem hiding this comment.
Handle potential null values in req.
Ensure that the function handles null values in req to prevent potential runtime errors.
+ if req == nil {
+ return nil, errorsmod.New("request cannot be nil")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | |
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | |
| if req == nil { | |
| return nil, errorsmod.New("request cannot be nil") | |
| } |
| // GetCommunityTax returns the current distribution community tax. | ||
| func (k Keeper) GetCommunityTax(ctx sdk.Context) (math.LegacyDec, error) { | ||
| params := k.GetParams(ctx) | ||
|
|
||
| return params.CommunityTax, nil |
There was a problem hiding this comment.
Incomplete function for community tax retrieval.
The GetCommunityTax function retrieves the community tax but lacks error handling.
Add error handling to ensure the function returns meaningful errors if retrieval fails.
- return params.CommunityTax, nil
+ if params.CommunityTax == (math.LegacyDec{}) {
+ return math.LegacyDec{}, errorsmod.New("community tax not set")
+ }
+ return params.CommunityTax, nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetCommunityTax returns the current distribution community tax. | |
| func (k Keeper) GetCommunityTax(ctx sdk.Context) (math.LegacyDec, error) { | |
| params := k.GetParams(ctx) | |
| return params.CommunityTax, nil | |
| // GetCommunityTax returns the current distribution community tax. | |
| func (k Keeper) GetCommunityTax(ctx sdk.Context) (math.LegacyDec, error) { | |
| params := k.GetParams(ctx) | |
| if params.CommunityTax == (math.LegacyDec{}) { | |
| return math.LegacyDec{}, errorsmod.New("community tax not set") | |
| } | |
| return params.CommunityTax, nil |
testutil/keeper/feedistribute.go
Outdated
| func FeedistributeKeeper(t testing.TB) (distrkeeper.Keeper, sdk.Context) { | ||
| storeKey := storetypes.NewKVStoreKey(types.StoreKey) | ||
|
|
There was a problem hiding this comment.
Add error handling for store initialization.
The MountStoreWithDB function can return an error, which should be handled appropriately.
- stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db)
+ if err := stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db); err != nil {
+ t.Fatalf("failed to mount store: %v", err)
+ }Committable suggestion was skipped due to low confidence.
testutil/keeper/feedistribute.go
Outdated
| blockedAddresses := map[string]bool{ | ||
| accountkeeper.AccountKeeper{}.GetAuthority(): false, | ||
| } |
There was a problem hiding this comment.
Remove redundant code.
The blockedAddresses map is initialized but not used anywhere in the function.
- blockedAddresses := map[string]bool{
- accountkeeper.AccountKeeper{}.GetAuthority(): false,
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| blockedAddresses := map[string]bool{ | |
| accountkeeper.AccountKeeper{}.GetAuthority(): false, | |
| } |
| if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.ModuleName, feesCollectedInt); err != nil { | ||
| return err | ||
| } | ||
| feePool := k.FeePool |
There was a problem hiding this comment.
Remove unused variable feePool.
The variable feePool is defined but not used anywhere in the function. This might be an oversight or could be a leftover from a previous version of the code.
- feePool := k.FeePoolCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| feePool := k.FeePool |
| // FeePool is the global fee pool for distribution. | ||
| // It holds decimal coins. Once whole those coins can be burned or distributed to the community pool. | ||
| message FeePool { | ||
| repeated cosmos.base.v1beta1.DecCoin community_pool = 1 [ | ||
| (gogoproto.nullable) = false, | ||
| (amino.dont_omitempty) = true, | ||
| (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins", | ||
| deprecated = true | ||
| ]; | ||
|
|
||
| repeated cosmos.base.v1beta1.DecCoin decimal_pool = 2 | ||
| [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins"]; | ||
| } |
There was a problem hiding this comment.
Consider removing the deprecated community_pool field.
The community_pool field is marked as deprecated. If this field is not required by legacy systems, consider removing it to clean up the API.
- repeated cosmos.base.v1beta1.DecCoin community_pool = 1 [
- (gogoproto.nullable) = false,
- (amino.dont_omitempty) = true,
- (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins",
- deprecated = true
- ];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FeePool is the global fee pool for distribution. | |
| // It holds decimal coins. Once whole those coins can be burned or distributed to the community pool. | |
| message FeePool { | |
| repeated cosmos.base.v1beta1.DecCoin community_pool = 1 [ | |
| (gogoproto.nullable) = false, | |
| (amino.dont_omitempty) = true, | |
| (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins", | |
| deprecated = true | |
| ]; | |
| repeated cosmos.base.v1beta1.DecCoin decimal_pool = 2 | |
| [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins"]; | |
| } | |
| // FeePool is the global fee pool for distribution. | |
| // It holds decimal coins. Once whole those coins can be burned or distributed to the community pool. | |
| message FeePool { | |
| repeated cosmos.base.v1beta1.DecCoin decimal_pool = 2 | |
| [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.DecCoins"]; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumx/feedistribution/types/distribution.pb.gois excluded by!**/*.pb.gox/feedistribution/types/params.pb.gois excluded by!**/*.pb.gox/reward/types/types.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- go.mod (11 hunks)
- proto/exocore/feedistribute/distribution.proto (1 hunks)
- proto/exocore/feedistribute/params.proto (1 hunks)
- proto/exocore/reward/types.proto (1 hunks)
- testutil/keeper/feedistribute.go (1 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/keeper.go (1 hunks)
- x/feedistribution/keeper/msg_update_params.go (1 hunks)
- x/feedistribution/keeper/params.go (1 hunks)
- x/feedistribution/types/expected_keepers.go (1 hunks)
- x/reward/types/types.go (2 hunks)
Files skipped from review due to trivial changes (1)
- testutil/keeper/feedistribute.go
Files skipped from review as they are similar to previous changes (7)
- go.mod
- proto/exocore/feedistribute/distribution.proto
- x/feedistribution/keeper/allocation.go
- x/feedistribution/keeper/keeper.go
- x/feedistribution/keeper/msg_update_params.go
- x/feedistribution/keeper/params.go
- x/feedistribution/types/expected_keepers.go
Additional context used
GitHub Check: lint
proto/exocore/feedistribute/params.proto
[failure] 12-12:
symbol "exocore.feedistribute.Params" already defined at exocore/feedistribute/distribution.proto:13:9
[failure] 18-18:
symbol "exocore.feedistribute.Params.community_tax" already defined at exocore/feedistribute/distribution.proto:16:10
Additional comments not posted (4)
proto/exocore/reward/types.proto (2)
24-26: LGTM!The
nameandrewardsfields in thePoolmessage remain unchanged and are correctly defined.
21-23: Verify the purpose of the new fields.The addition of
earnings_addrandValidatorfields changes the structure of theRewardmessage. Ensure that these fields are correctly used in the rest of the codebase.Verification successful
The new fields
earnings_addrandValidatorare correctly integrated and used in the codebase.
- The
earnings_addrfield is used in multiple files, includinglocal_node.sh,x/reward/types/types.pb.go,x/operator/types/tx.pb.go, andproto/exocore/operator/v1/tx.proto.- The
Validatorfield is used inx/reward/types/types.pb.go,x/feedistribution/types/keys.go, and various other files related to validator updates and distribution.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `earnings_addr` and `Validator` fields in the codebase. # Test: Search for the usage of the new fields. Expect: Correct usage in the codebase. rg --type proto -A 5 $'earnings_addr' rg --type proto -A 5 $'Validator'Length of output: 129
Script:
#!/bin/bash # Description: Verify the usage of `earnings_addr` and `Validator` fields in the codebase. # Test: Search for the usage of the new fields. Expect: Correct usage in the codebase. rg -A 5 'earnings_addr' rg -A 5 'Validator'Length of output: 419613
x/reward/types/types.go (2)
31-33: Verify address format validation.Ensure that the address format validation using
sdk.VerifyAddressFormatis consistent with the rest of the codebase.Verification successful
Address format validation is consistent.
The usage of
sdk.VerifyAddressFormatis consistent across the codebase, with appropriate error handling following each validation.
x/reward/types/types.go: Returns a formatted error message.x/evm/keeper/keeper.go: Panics on error.utils/utils.go: Returnsnil, erron error.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `sdk.VerifyAddressFormat` in the codebase. # Test: Search for the usage of `sdk.VerifyAddressFormat` in the codebase. Expect: Consistent usage for address format validation. rg --type go -A 5 $'sdk.VerifyAddressFormat'Length of output: 1051
22-24: Ensure proper error handling for duplicate validators.The current logic checks for duplicate validators using the raw
validatorAddr. Ensure that this approach is consistent with the rest of the codebase.Verification successful
The error handling for duplicate validators using
validatorAddris consistent within the codebase.
- The only instance of checking for duplicate validators is in
x/reward/types/types.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `validatorAddr` for duplicate checking in the codebase. # Test: Search for the usage of `validatorAddr` in the codebase. Expect: Consistent usage for duplicate checking. rg --type go -A 5 $'validatorAddr'Length of output: 1290
Script:
#!/bin/bash # Description: Verify the usage of duplicate validator checks across the codebase. # Test: Search for duplicate validator checks in the codebase. Expect: Consistent usage for duplicate checking. rg --type go -A 5 'duplicate validator'Length of output: 375
proto/exocore/reward/types.proto
Outdated
| // earnings_addr is the earnings address. | ||
| string earnings_addr = 3; | ||
| string Validator = 4; |
There was a problem hiding this comment.
Ensure consistency in field naming.
The field Validator should follow consistent naming conventions. Consider renaming it to validator to match the naming style of other fields.
- string Validator = 4;
+ string validator = 4;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // earnings_addr is the earnings address. | |
| string earnings_addr = 3; | |
| string Validator = 4; | |
| // earnings_addr is the earnings address. | |
| string earnings_addr = 3; | |
| string validator = 4; |
| message Params { | ||
| option (amino.name) = "exocore/x/feedistribution/Params"; | ||
| // period is the amount of past periods, based on the epochs per period param | ||
| uint64 period = 1; | ||
| // epoch_identifier for fee distribution | ||
| string epoch_identifier = 2; | ||
| string community_tax = 3 [ | ||
| (cosmos_proto.scalar) = "cosmos.Dec", | ||
| (gogoproto.customtype) = "cosmossdk.io/math.LegacyDec", | ||
| (amino.dont_omitempty) = true, | ||
| (gogoproto.nullable) = false | ||
| ]; |
There was a problem hiding this comment.
Address symbol conflicts for Params and community_tax.
Static analysis tools indicate symbol conflicts for Params and community_tax already defined in distribution.proto.
Ensure that the Params message and its fields do not conflict with existing definitions in distribution.proto.
- message Params {
- option (amino.name) = "exocore/x/feedistribution/Params";
- uint64 period = 1;
- string epoch_identifier = 2;
- string community_tax = 3 [
- (cosmos_proto.scalar) = "cosmos.Dec",
- (gogoproto.customtype) = "cosmossdk.io/math.LegacyDec",
- (amino.dont_omitempty) = true,
- (gogoproto.nullable) = false
- ];
- }Consider renaming the message or fields to resolve conflicts.
Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: lint
[failure] 12-12:
symbol "exocore.feedistribute.Params" already defined at exocore/feedistribute/distribution.proto:13:9
[failure] 18-18:
symbol "exocore.feedistribute.Params.community_tax" already defined at exocore/feedistribute/distribution.proto:16:10
mikebraver
force-pushed
the
distribution
branch
2 times, most recently
from
528c21b to
1b5c21b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumx/feedistribution/types/distribution.pb.gois excluded by!**/*.pb.gox/feedistribution/types/params.pb.gois excluded by!**/*.pb.gox/reward/types/types.pb.gois excluded by!**/*.pb.go
Files selected for processing (12)
- app/app.go (1 hunks)
- go.mod (11 hunks)
- proto/exocore/feedistribute/distribution.proto (1 hunks)
- proto/exocore/feedistribute/params.proto (1 hunks)
- proto/exocore/reward/types.proto (1 hunks)
- testutil/keeper/feedistribute.go (1 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/keeper.go (1 hunks)
- x/feedistribution/keeper/msg_update_params.go (1 hunks)
- x/feedistribution/keeper/params.go (1 hunks)
- x/feedistribution/types/expected_keepers.go (1 hunks)
- x/reward/types/types.go (2 hunks)
Files skipped from review due to trivial changes (1)
- testutil/keeper/feedistribute.go
Files skipped from review as they are similar to previous changes (6)
- proto/exocore/feedistribute/distribution.proto
- x/feedistribution/keeper/allocation.go
- x/feedistribution/keeper/keeper.go
- x/feedistribution/keeper/msg_update_params.go
- x/feedistribution/keeper/params.go
- x/feedistribution/types/expected_keepers.go
Additional context used
GitHub Check: lint
proto/exocore/reward/types.proto
[failure] 23-23:
Field name "Validator" should be lower_snake_case, such as "validator".proto/exocore/feedistribute/params.proto
[failure] 6-6:
Import "cosmos/base/v1beta1/coin.proto" is unused.
Additional comments not posted (18)
proto/exocore/reward/types.proto (3)
21-21: LGTM!The addition of the
earnings_addrfield is appropriate and follows the naming conventions.
23-23: LGTM!The closing brace of the
Rewardmessage is correctly placed.Tools
GitHub Check: lint
[failure] 23-23:
Field name "Validator" should be lower_snake_case, such as "validator".
22-22: Ensure consistency in field naming.The field
Validatorshould follow consistent naming conventions. Consider renaming it tovalidatorto match the naming style of other fields.- string Validator = 4; + string validator = 4;Likely invalid or redundant comment.
proto/exocore/feedistribute/params.proto (1)
12-23: Address symbol conflicts forParamsandcommunity_tax.Static analysis tools indicate symbol conflicts for
Paramsandcommunity_taxalready defined indistribution.proto.Ensure that the
Paramsmessage and its fields do not conflict with existing definitions indistribution.proto.x/reward/types/types.go (3)
22-26: LGTM!The code correctly retrieves the validator address and checks for duplicates.
31-33: LGTM!The code correctly verifies the address format.
Line range hint
34-40:
LGTM!The code correctly validates the rewards.
go.mod (10)
12-12: Dependency Addition Approved:github.com/bufbuild/buf v1.7.0The addition of
github.com/bufbuild/buf v1.7.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
42-42: Dependency Addition Approved:golang.org/x/tools v0.19.0The addition of
golang.org/x/tools v0.19.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
46-46: Dependency Addition Approved:google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0The addition of
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
47-47: Dependency Addition Approved:google.golang.org/protobuf v1.33.0The addition of
google.golang.org/protobuf v1.33.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
80-80: Dependency Addition Approved:github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161The addition of
github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
82-82: Dependency Addition Approved:github.com/Microsoft/go-winio v0.6.1The addition of
github.com/Microsoft/go-winio v0.6.1is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
90-90: Dependency Addition Approved:github.com/bufbuild/connect-go v1.0.0The addition of
github.com/bufbuild/connect-go v1.0.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
91-91: Dependency Addition Approved:github.com/bufbuild/protocompile v0.4.0The addition of
github.com/bufbuild/protocompile v0.4.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
102-102: Dependency Addition Approved:github.com/containerd/containerd v1.6.8The addition of
github.com/containerd/containerd v1.6.8is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.
258-258: Dependency Addition Approved:golang.org/x/mod v0.16.0The addition of
golang.org/x/mod v0.16.0is approved. Ensure it aligns with the project's requirements and does not introduce any security vulnerabilities.app/app.go (1)
598-598: Parameter Addition Approved:app.AVSManagerKeeperThe addition of the
app.AVSManagerKeeperparameter in theNewExocoreAppfunction is approved. Ensure that all usages of this parameter are correctly integrated and do not introduce any issues.Verification successful
Parameter Addition Approved:
app.AVSManagerKeeperThe addition of the
app.AVSManagerKeeperparameter in theNewExocoreAppfunction is approved. The parameter is correctly integrated and used consistently across the codebase, including in test files and the main application logic.
x/avs/keeper/avs_test.goprecompiles/avs/setup_test.goprecompiles/avsTask/setup_test.goprecompiles/avsTask/task_test.goapp/app.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `app.AVSManagerKeeper` in the codebase. # Test: Search for the usage of the `app.AVSManagerKeeper` parameter. Expect: Correct integration across the codebase. rg --type go -A 5 $'AVSManagerKeeper'Length of output: 7991
Comments failed to post (2)
proto/exocore/feedistribute/params.proto
6-6: Remove unused import.
The import
cosmos/base/v1beta1/coin.protois unused and should be removed to clean up the code.- import "cosmos/base/v1beta1/coin.proto";Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Tools
GitHub Check: lint
[failure] 6-6:
Import "cosmos/base/v1beta1/coin.proto" is unused.x/reward/types/types.go
27-30: Handle errors from
GetExocoreAddressFromBech32gracefully.The error handling for
utils.GetExocoreAddressFromBech32should include more context about the error.- if err != nil { - return err + if err != nil { + return fmt.Errorf("error converting validator address: %w", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.validator, err := utils.GetExocoreAddressFromBech32(reward.Validator) if err != nil { return fmt.Errorf("error converting validator address: %w", err) }
There was a problem hiding this comment.
Actionable comments posted: 20
Outside diff range, codebase verification and nitpick comments (5)
x/reward/types/keys.go (1)
23-29: Review new key prefix variable declarations.The new variable
KeyPrefixRewardInfois declared using the constantrewardInfo, which is defined as an integer due toiota. This can lead to confusion and potential bugs. IfrewardInfois intended to be a string identifier, it should be explicitly declared as a string.
rewardInfoshould be explicitly declared as a string if it is intended to be used as a string identifier.Analysis chain
Review new key prefix variable declarations.
The new variable
KeyPrefixRewardInfois correctly declared to match the new constantrewardInfo. However, ensure that the type ofrewardInfo(currently an integer due toiota) is appropriate for its use as a byte slice inKeyPrefixRewardInfo. IfrewardInfois intended to be a string identifier, it should be explicitly declared as a string to avoid confusion and potential bugs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the type and usage of `rewardInfo` in the codebase. # Test: Search for the declaration and usage of `rewardInfo`. Expect: Correct type and usage. rg --type go 'rewardInfo'Length of output: 125
Script:
#!/bin/bash # Description: Extract lines around the declaration of `rewardInfo` in `x/reward/types/keys.go`. # Test: Locate the declaration of `rewardInfo` and extract surrounding lines for context. rg --type go -A 10 -B 10 'rewardInfo' x/reward/types/keys.goLength of output: 376
x/feedistribution/types/genesis_test.go (1)
10-41: Incomplete test cases inTestGenesisState_Validate.The test structure is well-formed, but the presence of placeholder comments indicates that additional test cases and fields are needed to ensure comprehensive coverage. Please add the necessary test cases and fields to fully validate the genesis state.
- Add test cases for different scenarios as indicated by the placeholders.
- Ensure that all relevant fields are included in the test cases.
Analysis chain
LGTM! Verify the completeness of test cases.
The test structure is well-formed and follows best practices. Ensure that all edge cases and relevant scenarios are covered in the validation tests.
Ensure completeness of test cases by checking for scaffolded or additional test cases.
The current test structure is sound, but we need to verify if there are any scaffolded or additional test cases to ensure comprehensive coverage. Let's search for the placeholder comments and any other test cases in the file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness of test cases in the genesis state validation test file. # Test: Search for test functions in the genesis state validation test file. Expect: Only occurances of the test functions. rg --type go -A 5 'func Test' x/feedistribution/types/genesis_test.goLength of output: 208
Script:
#!/bin/bash # Description: Search for scaffolded or additional test cases in the genesis state validation test file. # Search for placeholder comments indicating scaffolded test cases. rg 'this line is used by starport scaffolding' x/feedistribution/types/genesis_test.go # Search for any other test functions in the file. rg --type go -A 5 'func Test' x/feedistribution/types/genesis_test.goLength of output: 445
x/reward/keeper/reward_record.go (3)
37-59: No tests found forAddRewardmethod.The
AddRewardmethod in therewardRecordstruct does not appear to have any associated tests. It is crucial to add comprehensive tests for this method to ensure its reliability, especially for edge cases like zero amounts and invalid addresses.
- File:
x/reward/keeper/reward_record.go- Lines: 37-59
Please add tests to cover the functionality of the
AddRewardmethod.Analysis chain
Review and test
AddRewardmethod for reliability.The
AddRewardmethod is critical for the functionality of the reward management system. Ensure thorough testing, especially for edge cases like zero amounts and invalid addresses. Additionally, ensure that all error paths are handled gracefully to maintain system stability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure thorough testing of the `AddReward` method. # Test: Search for the tests of `AddReward` method. Expect: Comprehensive test coverage for edge cases. rg --type go -A 5 'func TestAddReward'Length of output: 40
Script:
#!/bin/bash # Description: Ensure thorough testing of the `AddReward` method. # Search for test files related to rewardRecord fd -e go 'reward_record' # Search for test functions that might cover the AddReward method rg --type go -A 5 'rewardRecord'Length of output: 3173
99-108: Add direct tests for theClearRewardsmethod.The
ClearRewardsmethod is not directly tested in the codebase. To ensure comprehensive test coverage, especially for edge cases and error paths, please add direct tests for this method.
x/reward/keeper/reward_record.go:ClearRewardsmethodAnalysis chain
Review and test
ClearRewardsmethod for reliability.The
ClearRewardsmethod is critical for the functionality of the reward management system. Ensure thorough testing, especially for edge cases like invalid addresses. Additionally, ensure that all error paths are handled gracefully to maintain system stability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure thorough testing of the `ClearRewards` method. # Test: Search for the tests of `ClearRewards` method. Expect: Comprehensive test coverage for edge cases. rg --type go -A 5 'func TestClearRewards'Length of output: 43
Script:
#!/bin/bash # Description: Search for references to the `ClearRewards` method to verify its test coverage. # Find all Go test files and search for references to `ClearRewards`. rg --type go 'ClearRewards' --glob '*_test.go'Length of output: 46
Script:
#!/bin/bash # Description: Search for references to the `ClearRewards` method across the entire codebase to verify its usage and potential indirect testing. # Find all references to `ClearRewards` method in the codebase. rg 'ClearRewards'Length of output: 182
61-97: Review and testReleaseRewardsmethod for reliability.The
ReleaseRewardsmethod is critical for the functionality of the reward management system. Currently, there are no direct tests found for this method. Ensure thorough testing, especially for edge cases like invalid addresses and minting errors. Additionally, ensure that all error paths are handled gracefully to maintain system stability.
- File:
x/reward/keeper/reward_record.go- Lines: 61-97
Analysis chain
Review and test
ReleaseRewardsmethod for reliability.The
ReleaseRewardsmethod is critical for the functionality of the reward management system. Ensure thorough testing, especially for edge cases like invalid addresses and minting errors. Additionally, ensure that all error paths are handled gracefully to maintain system stability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure thorough testing of the `ReleaseRewards` method. # Test: Search for the tests of `ReleaseRewards` method. Expect: Comprehensive test coverage for edge cases. rg --type go -A 5 'func TestReleaseRewards'Length of output: 45
Script:
#!/bin/bash # Description: Search for references to `ReleaseRewards` in the test files to locate any relevant tests. # Find all test files and search for references to `ReleaseRewards` method. rg --type go 'ReleaseRewards'Length of output: 131
Script:
#!/bin/bash # Description: Find all test files in the `reward` package and inspect them for references to `ReleaseRewards`. # List all test files in the `reward` package. fd --type f --extension go --exec rg 'ReleaseRewards' {}Length of output: 127
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (24)
go.sumis excluded by!**/*.sumx/assets/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/avs/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/avs/types/tx.pb.gois excluded by!**/*.pb.gox/avstask/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/delegation/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/deposit/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/dogfood/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/epochs/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/exomint/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/exomint/types/tx.pb.gw.gois excluded by!**/*.pb.gw.gox/feedistribution/types/distribution.pb.gois excluded by!**/*.pb.gox/feedistribution/types/genesis.pb.gois excluded by!**/*.pb.gox/feedistribution/types/inflation.pb.gois excluded by!**/*.pb.gox/feedistribution/types/params.pb.gois excluded by!**/*.pb.gox/feedistribution/types/query.pb.gois excluded by!**/*.pb.gox/feedistribution/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/feedistribution/types/tx.pb.gois excluded by!**/*.pb.gox/operator/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/reward/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/reward/types/types.pb.gois excluded by!**/*.pb.gox/slash/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (50)
- app/app.go (1 hunks)
- go.mod (11 hunks)
- proto/buf.gen.pulsar.yaml (1 hunks)
- proto/buf.gen.sta.yaml (1 hunks)
- proto/buf.gen.ts.yaml (1 hunks)
- proto/exocore/avs/tx.proto (1 hunks)
- proto/exocore/feedistribute/distribution.proto (1 hunks)
- proto/exocore/feedistribute/genesis.proto (1 hunks)
- proto/exocore/feedistribute/module/module.proto (1 hunks)
- proto/exocore/feedistribute/params.proto (1 hunks)
- proto/exocore/feedistribute/query.proto (1 hunks)
- proto/exocore/feedistribute/tx.proto (1 hunks)
- proto/exocore/reward/types.proto (1 hunks)
- testutil/keeper/feedistribute.go (1 hunks)
- tools/tools.go (1 hunks)
- x/feedistribution/client/cli/query.go (1 hunks)
- x/feedistribution/client/cli/tx.go (1 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/epoch_info.go (1 hunks)
- x/feedistribution/keeper/genesis.go (1 hunks)
- x/feedistribution/keeper/hooks.go (1 hunks)
- x/feedistribution/keeper/keeper.go (1 hunks)
- x/feedistribution/keeper/msg_server.go (1 hunks)
- x/feedistribution/keeper/msg_server_test.go (1 hunks)
- x/feedistribution/keeper/msg_update_params.go (1 hunks)
- x/feedistribution/keeper/msg_update_params_test.go (1 hunks)
- x/feedistribution/keeper/params.go (1 hunks)
- x/feedistribution/keeper/params_test.go (1 hunks)
- x/feedistribution/keeper/query.go (1 hunks)
- x/feedistribution/keeper/query_params.go (1 hunks)
- x/feedistribution/keeper/query_params_test.go (1 hunks)
- x/feedistribution/module/genesis.go (1 hunks)
- x/feedistribution/module/genesis_test.go (1 hunks)
- x/feedistribution/module/module.go (1 hunks)
- x/feedistribution/types/codec.go (1 hunks)
- x/feedistribution/types/errors.go (1 hunks)
- x/feedistribution/types/expected_keepers.go (1 hunks)
- x/feedistribution/types/genesis.go (1 hunks)
- x/feedistribution/types/genesis_test.go (1 hunks)
- x/feedistribution/types/keys.go (1 hunks)
- x/feedistribution/types/msg_update_params.go (1 hunks)
- x/feedistribution/types/params.go (1 hunks)
- x/feedistribution/types/types.go (1 hunks)
- x/reward/keeper/epoch_info.go (1 hunks)
- x/reward/keeper/hooks.go (1 hunks)
- x/reward/keeper/keeper.go (1 hunks)
- x/reward/keeper/reward_record.go (1 hunks)
- x/reward/types/expected_keepers.go (2 hunks)
- x/reward/types/keys.go (1 hunks)
- x/reward/types/types.go (2 hunks)
Files skipped from review due to trivial changes (4)
- proto/buf.gen.pulsar.yaml
- proto/buf.gen.sta.yaml
- proto/buf.gen.ts.yaml
- x/feedistribution/types/types.go
Files skipped from review as they are similar to previous changes (9)
- app/app.go
- go.mod
- proto/exocore/feedistribute/distribution.proto
- proto/exocore/feedistribute/params.proto
- proto/exocore/reward/types.proto
- x/feedistribution/keeper/keeper.go
- x/feedistribution/keeper/params.go
- x/feedistribution/types/expected_keepers.go
- x/reward/types/types.go
Additional context used
GitHub Check: test-unit-cover
x/feedistribution/types/msg_update_params.go
[failure] 8-8:
cannot use &MsgUpdateParams{} (value of type *MsgUpdateParams) as "github.com/cosmos/cosmos-sdk/types".Msg value in variable declaration: *MsgUpdateParams does not implement "github.com/cosmos/cosmos-sdk/types".Msg (missing method GetSigners)x/feedistribution/types/keys.go
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte
GitHub Check: Run golangci-lint
x/feedistribution/types/msg_update_params.go
[failure] 8-8:
cannot use &MsgUpdateParams{} (value of type *MsgUpdateParams) as "github.com/cosmos/cosmos-sdk/types".Msg value in variable declaration: *MsgUpdateParams does not implement "github.com/cosmos/cosmos-sdk/types".Msg (missing method GetSigners)
[failure] 8-8:
cannot use &MsgUpdateParams{} (value of type *MsgUpdateParams) as "github.com/cosmos/cosmos-sdk/types".Msg value in variable declaration: *MsgUpdateParams does not implement "github.com/cosmos/cosmos-sdk/types".Msg (missing method GetSigners)
[failure] 8-8:
cannot use &MsgUpdateParams{} (value of type *MsgUpdateParams) as "github.com/cosmos/cosmos-sdk/types".Msg value in variable declaration: *MsgUpdateParams does not implement "github.com/cosmos/cosmos-sdk/types".Msg (missing method GetSigners)x/feedistribution/keeper/hooks.go
[failure] 16-16:
not enough arguments in call to k.AllocateTokensx/feedistribution/keeper/msg_update_params.go
[failure] 18-18:
k.SetParams(ctx, req.Params) (no value) used as value (typecheck)x/feedistribution/types/codec.go
[failure] 1-1:
: # github.com/ExocoreNetwork/exocore/x/feedistribution/typesx/feedistribution/types/keys.go
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte (typecheck)
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte) (typecheck)testutil/keeper/feedistribute.go
[failure] 5-5:
could not import github.com/ExocoreNetwork/exocore/x/feedistribution/types (-: # github.com/ExocoreNetwork/exocore/x/feedistribution/typesx/feedistribution/keeper/allocation.go
[failure] 6-6:
could not import github.com/ExocoreNetwork/exocore/x/feedistribution/types (-: # github.com/ExocoreNetwork/exocore/x/feedistribution/types
Additional comments not posted (78)
x/feedistribution/keeper/query.go (2)
3-5: Imports look good.The import statement correctly imports the necessary package.
7-7: Implement theQueryServermethods.The
Keepertype is declared to implement theQueryServerinterface, but the methods are not yet implemented. Ensure that all required methods are implemented.x/feedistribution/types/errors.go (2)
5-7: Imports look good.The import statement correctly imports the necessary package for error handling.
9-13: Error definitions look good.The error definitions are clear and follow the convention for sentinel errors.
x/feedistribution/keeper/msg_server.go (4)
3-5: Imports look good.The import statement correctly imports the necessary package.
7-9: Type declaration looks good.The
msgServertype embeds theKeepertype, which is a common pattern for message server implementations.
11-15: Function implementation looks good.The
NewMsgServerImplfunction correctly returns a newmsgServerinstance.
17-17: Implement theMsgServermethods.The
msgServertype is declared to implement theMsgServerinterface, but the methods are not yet implemented. Ensure that all required methods are implemented.proto/exocore/feedistribute/module/module.proto (3)
1-2: LGTM!The syntax and package declaration are correct.
4-4: Verify the necessity of the imported module.Ensure that the imported module
cosmos/app/v1alpha1/module.protois necessary for the functionality.
6-14: LGTM!The message definition for
Moduleis correct and follows the expected structure.x/feedistribution/keeper/params_test.go (3)
1-2: LGTM!The package declaration is correct.
3-10: LGTM!The import statements are correct and necessary for the test.
12-18: LGTM!The test function
TestGetParamsis well-structured and covers the necessary scenarios.x/feedistribution/types/msg_update_params.go (3)
1-2: LGTM!The package declaration is correct.
3-6: LGTM!The import statements are correct and necessary for the implementation.
10-20: LGTM!The
ValidateBasicmethod is well-implemented and performs necessary sanity checks.proto/exocore/feedistribute/genesis.proto (1)
1-17: LGTM! The genesis state definition is well-structured.The file correctly defines the genesis state for the
feedistributemodule using Protocol Buffers. The imports and options are appropriate, and theGenesisStatemessage is well-defined.x/feedistribution/keeper/query_params.go (1)
1-20: LGTM! The Params function is well-structured.The function correctly handles a gRPC request to query the module's parameters. The request validation and error handling are appropriate.
tools/tools.go (5)
6-6: LGTM!The import statement for
github.com/bufbuild/buf/cmd/bufis appropriate for managing protocol buffers.
7-7: LGTM!The import statement for
github.com/cosmos/cosmos-proto/cmd/protoc-gen-go-pulsaris appropriate for generating Go code from protobuf definitions.
13-13: LGTM!The import statement for
golang.org/x/tools/cmd/goimportsis appropriate for formatting Go code.
14-14: LGTM!The import statement for
google.golang.org/grpc/cmd/protoc-gen-go-grpcis appropriate for generating gRPC code from protobuf definitions.
15-15: LGTM!The import statement for
google.golang.org/protobuf/cmd/protoc-gen-gois appropriate for generating Go code from protobuf definitions.x/feedistribution/keeper/query_params_test.go (1)
12-20: LGTM!The test function
TestParamsQuerycorrectly tests theParamsquery of thefeedistributionmodule by setting default parameters and querying them to ensure they are correctly set and retrieved.x/feedistribution/client/cli/tx.go (1)
10-19: LGTM!The function
GetTxCmdcorrectly creates and returns a newcobra.Commandwith appropriate settings for thefeedistributionmodule.x/feedistribution/keeper/hooks.go (1)
5-7: No-op function acknowledged.The
BeforeEpochStartfunction is a no-op and does not perform any operations. This is acceptable if no actions are needed at the start of the epoch.x/feedistribution/keeper/epoch_info.go (2)
8-17: LGTM!The
GetEpochIdentifierfunction correctly retrieves the epoch identifier from the store.
19-23: LGTM!The
SetEpochIdentifierfunction correctly stores the epoch identifier in the store.x/reward/keeper/epoch_info.go (2)
8-17: LGTM!The
GetEpochIdentifierfunction correctly retrieves the epoch identifier from the store.
19-23: LGTM!The
SetEpochIdentifierfunction correctly stores the epoch identifier in the store.x/feedistribution/keeper/msg_server_test.go (2)
14-17: LGTM!The
setupMsgServerfunction correctly initializes the keeper and context for testing.
19-24: Add more assertions to the test.The test
TestMsgServercurrently only checks for non-null values. Consider adding more detailed assertions to verify the behavior and state of the message server beyond just its existence.+ require.Equal(t, expectedValue, actualValue, "Mismatch in expected and actual values")x/feedistribution/types/genesis.go (2)
10-15: LGTM!The
DefaultGenesisfunction correctly initializes the default genesis state with default parameters.
18-24: Add more thorough validation to theValidatefunction.Ensure that the
Validatefunction covers all necessary aspects of the genesis state, not just theParamsfield.+ if len(gs.SomeOtherField) == 0 { + return fmt.Errorf("SomeOtherField cannot be empty") + }x/feedistribution/types/codec.go (2)
14-21: LGTM!The
RegisterInterfacesfunction is correctly implemented and registers the necessary interfaces and message service descriptions.
23-26: LGTM!The variable declarations for
AminoandModuleCdcare correct and follow best practices.x/feedistribution/types/params.go (4)
14-16: LGTM!The
ParamKeyTablefunction correctly registers the parameter set for the module.
18-23: LGTM! But verify parameter initialization.The
NewParamsfunction correctly creates a newParamsinstance. Verify if any parameters need to be initialized.
25-29: LGTM! But verify default parameter values.The
DefaultParamsfunction correctly returns a default set of parameters. Verify if any default parameters need to be set.
31-35: LGTM! But verify parameter pairs.The
ParamSetPairsfunction correctly returns an empty set of parameter pairs. Verify if any parameter pairs need to be returned.x/feedistribution/module/genesis.go (1)
18-26: LGTM!The
ExportGenesisfunction correctly returns the module's exported genesis state.x/feedistribution/module/genesis_test.go (1)
13-29: LGTM! Verify the completeness of test cases.The test structure is well-formed and covers the basic scenarios for initializing and exporting the genesis state. Ensure that all edge cases and relevant scenarios are covered.
Verification successful
LGTM! Verify the completeness of test cases.
The test structure is well-formed and covers the basic scenarios for initializing and exporting the genesis state. Additionally, the presence of
TestGenesisState_Validateinx/feedistribution/types/genesis_test.goindicates that validation scenarios are also covered.
x/feedistribution/module/genesis_test.go:TestGenesisx/feedistribution/types/genesis_test.go:TestGenesisState_ValidateScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness of test cases in the genesis test file. # Test: Search for test functions in the genesis test file. Expect: Only occurances of the test functions. rg --type go -A 5 'func Test' x/feedistribution/module/genesis_test.goLength of output: 247
Script:
#!/bin/bash # Description: Search for other test functions related to genesis in the repository. # Test: Search for test functions in files related to genesis. Expect: Occurrences of test functions in relevant files. rg --type go -A 5 'func Test' | grep -i genesisLength of output: 4344
x/feedistribution/keeper/genesis.go (2)
21-26: LGTM! Verify the correctness of parameter export.The function structure is well-formed and follows best practices. Ensure that the parameters are correctly exported from the current context.
Verification successful
Verified: The parameters are correctly exported in the
ExportGenesisfunction.The function assigns the parameters to
genesis.Paramsusingk.GetParams(ctx), ensuring that the parameters are correctly exported from the current context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of parameter export in the ExportGenesis function. # Test: Search for parameter export in the ExportGenesis function. Expect: Only occurances of the parameter export. rg --type go -A 5 'genesis.Params = k.GetParams' x/feedistribution/keeper/genesis.goLength of output: 139
9-19: LGTM! Verify the correctness of error handling.The function structure is well-formed and follows best practices. Ensure that the error handling for missing epoch info is appropriate and that the parameters are correctly set.
proto/exocore/feedistribute/query.proto (6)
1-3: LGTM!The syntax declaration and package definition are correct.
4-8: LGTM!The import statements are correctly specified and necessary for the functionality.
9-10: LGTM!The Go package option is correctly specified.
11-17: LGTM!The
Queryservice and theParamsRPC method are correctly defined, following gRPC conventions.
19-21: LGTM!The
QueryParamsRequestmessage is correctly defined.
22-29: LGTM!The
QueryParamsResponsemessage is correctly defined, and theParamsfield includes necessary options.x/reward/types/expected_keepers.go (2)
6-6: LGTM!The import statement for
stakingtypesis necessary and correctly added.
24-26: LGTM!The
Distributorinterface and theAllocateTokensToValidatormethod are correctly defined.x/feedistribution/client/cli/query.go (1)
1-10: LGTM!The package declaration and import statements are correct.
proto/exocore/feedistribute/tx.proto (4)
1-2: LGTM!The syntax and package declaration are correct.
4-8: LGTM!The import statements are correct and necessary.
10-19: LGTM!The Go package option and service definition are correct.
21-40: LGTM!The message definitions are correct and follow the expected structure.
x/feedistribution/keeper/msg_update_params_test.go (3)
1-10: LGTM!The package and import statements are correct.
12-50: Comprehensive tests forMsgUpdateParamsfunctionality.The tests are well-structured and cover a variety of expected scenarios, ensuring robust error handling and functionality verification.
52-64: LGTM!The test case execution is correctly implemented.
x/feedistribution/types/keys.go (2)
1-6: LGTM!The package and import statements are correct.
8-51: LGTM!The constant definitions are correct and necessary for the module.
testutil/keeper/feedistribute.go (4)
25-27: Add error handling for store initialization.The
MountStoreWithDBfunction can return an error, which should be handled appropriately.- stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db) + if err := stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db); err != nil { + t.Fatalf("failed to mount store: %v", err) + }
36-38: Remove redundant code.The
blockedAddressesmap is initialized but not used anywhere in the function.- blockedAddresses := map[string]bool{ - accountkeeper.AccountKeeper{}.GetAuthority(): false, - }
57-57: LGTM!The return statement is correct and follows the expected pattern for setting up a keeper for testing.
5-5: Verify the import path fortypes.The static analysis tool flagged an issue with importing
github.com/ExocoreNetwork/exocore/x/feedistribution/types. Ensure the path is correct and the package is available.Verification successful
The import path for
typesis correct.The directory
x/feedistribution/typesexists, confirming the validity of the import pathgithub.com/ExocoreNetwork/exocore/x/feedistribution/types.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path for `types`. # Test: Check if the package exists at the specified path. fd --type d 'types' | grep 'x/feedistribution/types'Length of output: 75
Tools
GitHub Check: Run golangci-lint
[failure] 5-5:
could not import github.com/ExocoreNetwork/exocore/x/feedistribution/types (-: # github.com/ExocoreNetwork/exocore/x/feedistribution/typesx/reward/keeper/hooks.go (3)
3-10: LGTM!The imports are appropriate and necessary. The
BeforeEpochStartfunction correctly does nothing as indicated by the comment.
12-76: Consider refactoring theAfterEpochEndfunction to reduce complexity.The nested loops and multiple error checks make this function quite complex. Consider refactoring to improve readability and maintainability.
78-83: LGTM!The
ForEachfunction correctly applies a given function to each element in a slice.x/reward/keeper/keeper.go (4)
Line range hint
3-16:
LGTM!The imports are appropriate and necessary for the functionality provided in the file.
20-48: LGTM!The enhancements to the
Keeperstructure and theNewKeeperfunction are appropriate and follow the expected pattern for initializing a keeper with additional dependencies.
54-75: LGTM!The
setPoolandgetPoolsmethods are correct and follow the expected pattern for managing pools in a prefixed store.
77-86: LGTM!The
getPoolmethod is correct and follows the expected pattern for retrieving a specific pool by name or returning a new reward record if the pool does not exist.x/feedistribution/keeper/allocation.go (1)
21-21: Remove unused variablefeePool.The variable
feePoolis defined but not used anywhere in the function. This might be an oversight or could be a leftover from a previous version of the code.- feePool := k.FeePoolLikely invalid or redundant comment.
x/feedistribution/module/module.go (3)
22-25: LGTM!The variable declarations ensure that the module implements the necessary interfaces.
27-102: LGTM!The
AppModuleBasicimplementation is comprehensive and follows the standard structure for a Cosmos SDK module. The methods are correctly implemented and necessary for the module's functionality.
104-170: LGTM!The
AppModuleimplementation is comprehensive and follows the standard structure for a Cosmos SDK module. The methods are correctly implemented and necessary for the module's functionality.
x/feedistribution/module/module.go
Outdated
| @@ -0,0 +1,170 @@ | |||
| package feedistribute | |||
There was a problem hiding this comment.
Correct package name.
The package name feedistribute seems to be a typo or abbreviation. It might be more readable and consistent to use a full name like feedistribution.
- package feedistribute
+ package feedistributionCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package feedistribute | |
| package feedistribution |
x/feedistribution/keeper/hooks.go
Outdated
| // AfterEpochEnd mints and allocates coins at the end of each epoch end | ||
| func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) { | ||
| expEpochID := k.GetEpochIdentifier(ctx) | ||
| if epochIdentifier != expEpochID { | ||
| return | ||
| } | ||
| // the minted coins generated by minting module will do the token allocation distribution here | ||
| k.AllocateTokens() |
There was a problem hiding this comment.
Fix the AllocateTokens call.
The AllocateTokens function call is missing required arguments. Ensure that the correct number of arguments are passed.
- k.AllocateTokens()
+ k.AllocateTokens(ctx, epochIdentifier, epochNumber)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // AfterEpochEnd mints and allocates coins at the end of each epoch end | |
| func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) { | |
| expEpochID := k.GetEpochIdentifier(ctx) | |
| if epochIdentifier != expEpochID { | |
| return | |
| } | |
| // the minted coins generated by minting module will do the token allocation distribution here | |
| k.AllocateTokens() | |
| // AfterEpochEnd mints and allocates coins at the end of each epoch end | |
| func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) { | |
| expEpochID := k.GetEpochIdentifier(ctx) | |
| if epochIdentifier != expEpochID { | |
| return | |
| } | |
| // the minted coins generated by minting module will do the token allocation distribution here | |
| k.AllocateTokens(ctx, epochIdentifier, epochNumber) |
Tools
GitHub Check: Run golangci-lint
[failure] 16-16:
not enough arguments in call to k.AllocateTokens
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | ||
| if k.GetAuthority() != req.Authority { | ||
| return nil, errorsmod.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority) | ||
| } | ||
|
|
||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| if err := k.SetParams(ctx, req.Params); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &types.MsgUpdateParamsResponse{}, nil | ||
| } |
There was a problem hiding this comment.
Handle potential null values in req and fix the typecheck error.
Ensure that the function handles null values in req to prevent potential runtime errors. Also, fix the typecheck error related to the SetParams function.
+ if req == nil {
+ return nil, errorsmod.New("request cannot be nil")
+ }
if k.GetAuthority() != req.Authority {
return nil, errorsmod.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority)
}
ctx := sdk.UnwrapSDKContext(goCtx)
- if err := k.SetParams(ctx, req.Params); err != nil {
+ if err := k.SetParams(ctx, req.Params); err != nil {
return nil, err
}
return &types.MsgUpdateParamsResponse{}, nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | |
| if k.GetAuthority() != req.Authority { | |
| return nil, errorsmod.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority) | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| if err := k.SetParams(ctx, req.Params); err != nil { | |
| return nil, err | |
| } | |
| return &types.MsgUpdateParamsResponse{}, nil | |
| } | |
| func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | |
| if req == nil { | |
| return nil, errorsmod.New("request cannot be nil") | |
| } | |
| if k.GetAuthority() != req.Authority { | |
| return nil, errorsmod.Wrapf(types.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.GetAuthority(), req.Authority) | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| if err := k.SetParams(ctx, req.Params); err != nil { | |
| return nil, err | |
| } | |
| return &types.MsgUpdateParamsResponse{}, nil | |
| } |
Tools
GitHub Check: Run golangci-lint
[failure] 18-18:
k.SetParams(ctx, req.Params) (no value) used as value (typecheck)
x/feedistribution/types/keys.go
Outdated
| func KeyPrefix(p uint64) []byte { | ||
| return []byte(p) | ||
| } |
There was a problem hiding this comment.
Fix the KeyPrefix function implementation.
The KeyPrefix function is intended to return a byte slice from a uint64, but it directly casts the integer to a byte slice, which is incorrect.
- return []byte(p)
+ return []byte(strconv.FormatUint(p, 10))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func KeyPrefix(p uint64) []byte { | |
| return []byte(p) | |
| } | |
| func KeyPrefix(p uint64) []byte { | |
| return []byte(strconv.FormatUint(p, 10)) | |
| } |
Tools
GitHub Check: test-unit-cover
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte
GitHub Check: Run golangci-lint
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte (typecheck)
[failure] 54-54:
cannot convert p (variable of type uint64) to type []byte) (typecheck)
| func RegisterCodec(*codec.LegacyAmino) { | ||
| } |
There was a problem hiding this comment.
Implement or remove the RegisterCodec function.
The RegisterCodec function is currently defined but not implemented. If it's not needed, consider removing it. Otherwise, provide the necessary implementation.
| remaining := feesCollected | ||
| communityTax, err := k.GetCommunityTax(ctx) |
There was a problem hiding this comment.
Remove redundant variable declarations remaining and communityTax.
Both remaining and communityTax are declared but never used in any meaningful way within the function. This could be a result of incomplete refactoring.
- remaining := feesCollected
- communityTax, err := k.GetCommunityTax(ctx)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remaining := feesCollected | |
| communityTax, err := k.GetCommunityTax(ctx) |
| if currentCommission, ok := k.ValidatorsAccumulatedCommission[valBz]; ok { | ||
| currentCommission.Commission = currentCommission.Commission.Add(commission...) | ||
| k.ValidatorsAccumulatedCommission[valBz] = currentCommission | ||
| } else { | ||
| fmt.Sprintf("currentCommission %s didn't exist", currentCommission) | ||
| // No need to return here | ||
| } |
There was a problem hiding this comment.
Remove redundant code block.
The code block that checks for the existence of currentCommission and logs a message if it doesn't exist is redundant and can be removed.
- } else {
- fmt.Sprintf("currentCommission %s didn't exist", currentCommission)
- // No need to return hereCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if currentCommission, ok := k.ValidatorsAccumulatedCommission[valBz]; ok { | |
| currentCommission.Commission = currentCommission.Commission.Add(commission...) | |
| k.ValidatorsAccumulatedCommission[valBz] = currentCommission | |
| } else { | |
| fmt.Sprintf("currentCommission %s didn't exist", currentCommission) | |
| // No need to return here | |
| } | |
| if currentCommission, ok := k.ValidatorsAccumulatedCommission[valBz]; ok { | |
| currentCommission.Commission = currentCommission.Commission.Add(commission...) | |
| k.ValidatorsAccumulatedCommission[valBz] = currentCommission | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumx/feedistribution/types/distribution.pb.gois excluded by!**/*.pb.gox/feedistribution/types/params.pb.gois excluded by!**/*.pb.gox/feedistribution/types/query.pb.gois excluded by!**/*.pb.gox/reward/types/types.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- app/app.go (1 hunks)
- go.mod (11 hunks)
- proto/exocore/feedistribute/distribution.proto (1 hunks)
- proto/exocore/feedistribute/params.proto (1 hunks)
- proto/exocore/feedistribute/query.proto (1 hunks)
- proto/exocore/reward/types.proto (1 hunks)
- testutil/keeper/feedistribute.go (1 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/hooks.go (1 hunks)
- x/feedistribution/keeper/keeper.go (1 hunks)
- x/feedistribution/keeper/msg_update_params.go (1 hunks)
- x/feedistribution/keeper/params.go (1 hunks)
- x/feedistribution/module/genesis.go (1 hunks)
- x/feedistribution/types/codec.go (1 hunks)
- x/feedistribution/types/expected_keepers.go (1 hunks)
- x/feedistribution/types/keys.go (1 hunks)
- x/feedistribution/types/msg_update_params.go (1 hunks)
- x/reward/types/types.go (2 hunks)
Files skipped from review due to trivial changes (2)
- x/feedistribution/keeper/hooks.go
- x/feedistribution/types/expected_keepers.go
Files skipped from review as they are similar to previous changes (16)
- app/app.go
- go.mod
- proto/exocore/feedistribute/distribution.proto
- proto/exocore/feedistribute/params.proto
- proto/exocore/feedistribute/query.proto
- proto/exocore/reward/types.proto
- testutil/keeper/feedistribute.go
- x/feedistribution/keeper/allocation.go
- x/feedistribution/keeper/keeper.go
- x/feedistribution/keeper/msg_update_params.go
- x/feedistribution/keeper/params.go
- x/feedistribution/module/genesis.go
- x/feedistribution/types/codec.go
- x/feedistribution/types/keys.go
- x/feedistribution/types/msg_update_params.go
- x/reward/types/types.go
mikebraver
force-pushed
the
distribution
branch
2 times, most recently
from
0b9a407 to
52ecbf5
Compare
No other recommendations were received for the x/dogfood module.
* feat(assets): validate total supply before edit Now that the TVL limits are imposed on the client chain instead of Exocore, this PR renames the variable `tvlLimit` to `totalSupply`. In addition, editing the `totalSupply` to a value lower than the currently deposited tokens is not permitted. * fix(doc): update IAssets comment * feat: introduce `getTotalSupply` precompile This can be used to fetch the total supply, according to Exocore, for an asset. * fix: remove staking asset total supply The `totalSupply` variable was created to guard against black swan events wherein a malicious transaction could try to deposit more than the quantity of tokens that exist. The benefits of this feature are negligible, and the costs to maintain the constant updates between the "real" total supply on the client chain and the "recorded" total supply on Exocore are too high. Hence, it has been removed in its entirety. Instead, TVL limits will be imposed by the client chain for an operational risk management perspective. * fix the lint error caused by the lower curl version --------- Co-authored-by: TimmyExogenous <chuang@exogenous.io>
mikebraver
force-pushed
the
distribution
branch
from
2ec3964 to
6e9d9b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
x/dogfood/keeper/keeper.go (1)
138-140: Consider renaming the return variables for clarity.The
OperatorInfomethod returns variables namedinfoanderr. Consider renaming these tooperatorInfoanderrorrespectively for improved clarity and consistency with Go naming conventions.Apply this diff to rename the return variables:
-func (k *Keeper) OperatorInfo(ctx sdk.Context, addr string) (info *operatortypes.OperatorInfo, err error) { +func (k *Keeper) OperatorInfo(ctx sdk.Context, addr string) (operatorInfo *operatortypes.OperatorInfo, error error) {x/feedistribution/keeper/allocation.go (3)
14-71: LGTM! Enhance error handling and track parallelization optimization.The
AllocateTokensfunction correctly distributes fees to validators based on their voting power and manages the community pool allocations. A few suggestions:
- Instead of just logging errors and continuing, consider propagating the errors to the caller. This allows the caller to handle the errors appropriately and ensures that the function doesn't continue with an inconsistent state.
Apply this diff to propagate errors:
- ctx.Logger().Error("Failed to deserialize public key; skipping", "error", err, "i", i) - continue + return err - ctx.Logger().Error("Operator address not found; skipping", "consAddress", sdk.GetConsAddress(pk), "i", i) - continue + return err
- The comment about considering parallelization is valid. Parallelizing the loop can improve performance, especially with a large number of validators.
Do you want me to open a GitHub issue to track this optimization task?
75-112: Review ofAllocateTokensToValidatorFunction:
- Commission Calculation: The calculation of commission using
MulDecis correctly implemented. This ensures that the validator's commission rate is properly applied to the tokens.- Event Emissions: The function emits events for both commission and rewards, which is crucial for transparency and auditability in blockchain applications.
- Error Handling: There is no explicit error handling within this function. Consider adding error checks, especially after external calls or critical operations.
Consider enhancing the function with better error handling to improve robustness and maintainability.
114-163: LGTM! Enhance error handling and address potential non-determinism.The
AllocateTokensToStakersfunction correctly retrieves the stakers and their power, and allocates rewards proportionally. The sorting of stakers ensures that rewards are distributed fairly, and the handling of remaining rewards by adding them to the community pool is a sensible default.A few suggestions:
- Instead of just logging errors and continuing, consider propagating the errors to the caller. This allows the caller to handle the errors appropriately and ensures that the function doesn't continue with an inconsistent state.
Apply this diff to propagate errors:
- logger.Debug("avs address lists not found; skipping") - return + return err - logger.Debug("avs address lists not found; skipping") - continue + return err - logger.Debug("staker lists not found; skipping") - continue + return err - logger.Error("curStakerPower error", err) + return err
- The iteration over
stakersPowerMapmay lead to non-deterministic behavior. Consider using a deterministic approach if order matters.Apply this diff to address potential non-determinism:
- for staker, stakerPower := range stakersPowerMap { + // Consider sorting keys or using a deterministic iteration methodTools
GitHub Check: CodeQL
[warning] 130-145: Iteration over map
Iteration over map may be a possible source of non-determinism
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/app.go (12 hunks)
- local_node.sh (1 hunks)
- testutil/utils.go (2 hunks)
- x/dogfood/keeper/keeper.go (3 hunks)
- x/dogfood/types/expected_keepers.go (3 hunks)
- x/feedistribution/keeper/allocation.go (1 hunks)
- x/feedistribution/keeper/hooks_test.go (1 hunks)
- x/operator/keeper/usd_value_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- local_node.sh
- x/dogfood/types/expected_keepers.go
- x/feedistribution/keeper/hooks_test.go
Additional context used
GitHub Check: test-unit-cover
x/operator/keeper/usd_value_test.go
[failure] 35-35:
undefined: assetstype.MaxAssetTotalSupply
[failure] 37-37:
undefined: assetstype.MaxAssetTotalSupply
[failure] 40-40:
undefined: assetstype.MaxDecForTotalSupply
[failure] 46-46:
undefined: assetstype.MaxDecForTotalSupply
[failure] 78-78:
unknown field TotalSupply in struct literal of type "github.com/ExocoreNetwork/exocore/x/assets/types".AssetInfo
GitHub Check: CodeQL
x/feedistribution/keeper/allocation.go
[warning] 130-145: Iteration over map
Iteration over map may be a possible source of non-determinism
Additional comments not posted (15)
x/dogfood/keeper/keeper.go (1)
43-43: Verify thedelegationKeepertype change in the codebase.The
NewKeeperfunction signature has been modified to accept a more specific type for thedelegationKeeperparameter. Ensure that all calls to this function have been updated to pass the correct type.Run the following script to verify the
NewKeeperfunction usage:x/operator/keeper/usd_value_test.go (2)
35-35: LGTM!The changes replace the usage of local constants
MaxAssetTotalSupplyandMaxDecimalwith references to theassetstypepackage constantsassetstype.MaxAssetTotalSupplyandassetstype.MaxDecimal. This improves clarity and maintainability by sourcing the values from a dedicated package.Also applies to: 37-37
Tools
GitHub Check: test-unit-cover
[failure] 35-35:
undefined: assetstype.MaxAssetTotalSupply
40-40: LGTM!The expected values calculated using
sdkmath.LegacyNewDecFromBigInthave been updated to reflect the new references toassetstype.MaxDecForTotalSupply. This adjustment maintains the integrity of the tests while aligning them with the new constant references.Also applies to: 46-46
Tools
GitHub Check: test-unit-cover
[failure] 40-40:
undefined: assetstype.MaxDecForTotalSupplyx/feedistribution/keeper/allocation.go (1)
165-171: LGTM!The
AllocateTokensToSingleStakerfunction correctly updates the staker's rewards by adding the allocated reward. The function is straightforward and does not require any changes.testutil/utils.go (2)
23-24: LGTM!The import statement for the
distributiontypespackage is correctly added.
267-270: Verify the distribution module's genesis state.The changes introduce the creation of a new genesis state for the distribution module using the
distributiontypes.NewGenesisStatefunction with default parameters. This is then serialized and added to thegenesisStatemap.The implementation looks correct. However, please ensure that:
- The default parameters are appropriate for your use case.
- The distribution module's genesis state is correctly handled during the chain's initialization.
- The addition of this new genesis state does not introduce any unintended side effects or conflicts with other modules.
To verify the correctness and impact of the distribution module's genesis state, consider the following:
app/app.go (9)
13-16: LGTM!The new import statements for the fee distribution module, keeper, and types are correctly added.
281-281: LGTM!The
distr.AppModuleBasic{}is correctly added to theModuleBasicsmanager.
296-296: LGTM!The fee distribution module account permissions are correctly defined in the
maccPermsmap.
367-367: LGTM!The
DistrKeeperfield is correctly added to theExocoreAppstruct.
822-822: LGTM!The
DistrKeeper.EpochsHooks()is correctly added to theEpochsKeeper's hooks.
959-959: LGTM!The
distrtypes.ModuleNameis correctly added to theSetOrderBeginBlockersfunction in the appropriate position.
993-993: LGTM!The
distrtypes.ModuleNameis correctly added to theSetOrderEndBlockersfunction in the appropriate position.
1035-1035: LGTM!The
distrtypes.ModuleNameis correctly added to theSetOrderInitGenesisfunction in the appropriate position.
701-713: Verify the dependencies and initialization order.The
DistrKeeperis correctly initialized with the required dependencies. However, please ensure that the initialization order of the keepers is correct, considering the dependencies between them.Run the following script to verify the keeper initialization order:
| func (k *Keeper) GetStakersByOperator(ctx sdk.Context, operator, assetID string) (delegationtype.StakerList, error) { | ||
| return k.delegationKeeper.GetStakersByOperator(ctx, operator, assetID) | ||
| } |
There was a problem hiding this comment.
Consider error handling for GetStakersByOperator.
The method returns an error, but it is not being handled in the current implementation. Consider adding error handling logic or documenting why the error is being ignored.
| func (k Keeper) GetAVSSupportedAssets(ctx sdk.Context, avsAddr string) (map[string]interface{}, error) { | ||
| return k.avsKeeper.GetAVSSupportedAssets(ctx, avsAddr) | ||
| } |
There was a problem hiding this comment.
Consider error handling for GetAVSSupportedAssets.
Similar to the previous comment, the returned error is not being handled. Add error handling or document the reason for ignoring the error.
| func (k Keeper) GetOptedInAVSForOperator(ctx sdk.Context, operatorAddr string) ([]string, error) { | ||
| return k.operatorKeeper.GetOptedInAVSForOperator(ctx, operatorAddr) | ||
| } |
There was a problem hiding this comment.
Consider error handling for GetOptedInAVSForOperator.
The method returns an error, but it is not being handled. Add error handling or provide a comment explaining why the error is being ignored.
| func (k Keeper) CalculateUSDValueForStaker(ctx sdk.Context, stakerID, avsAddr string, operator sdk.AccAddress) (sdkmath.LegacyDec, error) { | ||
| return k.operatorKeeper.CalculateUSDValueForStaker(ctx, stakerID, avsAddr, operator) | ||
| } |
There was a problem hiding this comment.
Consider error handling for CalculateUSDValueForStaker.
The method returns an error, but it is not being handled in the current implementation. Add error handling logic or document the reason for ignoring the error.
x/operator/keeper/usd_value_test.go
Outdated
| @@ -75,6 +75,7 @@ | |||
| Symbol: "USDC", | |||
| Address: usdcAddr.String(), | |||
| Decimals: 6, | |||
| TotalSupply: sdkmath.NewInt(1e18), | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Remove the TotalSupply field from the AssetInfo initialization
The verification process has confirmed that the TotalSupply field is indeed not part of the assetstype.AssetInfo struct. This field should be removed from the struct initialization in the test file to fix the compilation error.
Please make the following change:
usdcClientChainAsset := assetstype.AssetInfo{
Name: "USD coin",
Symbol: "USDC",
Address: usdcAddr.String(),
Decimals: 6,
- TotalSupply: sdkmath.NewInt(1e18),
LayerZeroChainID: 101,
MetaInfo: "USDC",
}This change will ensure that the struct initialization matches the actual definition of AssetInfo, resolving the compilation error.
Analysis chain
Fix the struct field error.
The TotalSupply field is not defined in the assetstype.AssetInfo struct. This will cause a compilation error.
To verify the struct definition, run:
If the TotalSupply field is indeed missing from the struct definition, please remove it from the struct initialization to fix the compilation error:
usdcClientChainAsset := assetstype.AssetInfo{
Name: "USD coin",
Symbol: "USDC",
Address: usdcAddr.String(),
Decimals: 6,
- TotalSupply: sdkmath.NewInt(1e18),
LayerZeroChainID: 101,
MetaInfo: "USDC",
}If the TotalSupply field is required, please add it to the assetstype.AssetInfo struct definition first.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the `TotalSupply` field is not defined in the `assetstype.AssetInfo` struct.
# Test: Search for the struct definition. Expect: No `TotalSupply` field.
ast-grep --lang go --pattern $'type AssetInfo struct {
$$$
}' | rg -v 'TotalSupply'
Length of output: 1682
Tools
GitHub Check: test-unit-cover
[failure] 78-78:
unknown field TotalSupply in struct literal of type "github.com/ExocoreNetwork/exocore/x/assets/types".AssetInfo
| // Validate validates the set of params | ||
| func (p Params) Validate() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Agree with this one
|
|
||
| // Params defines the parameters for the module. | ||
| message Params { | ||
| option (amino.name) = "exocore/x/feedistribution/Params"; |
There was a problem hiding this comment.
Not a blocker, but what does this do? I haven't seen it before.
| if totalPreviousPower == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| if totalPreviousPower == 0 { | |
| return nil | |
| } |
This can be removed since we already checked this outside the loop. Or did you mean to check val.Power == 0 ?
| if err != nil { | ||
| ctx.Logger().Error("avs address lists not found; skipping") | ||
| } | ||
| stakersPowerMap, curTotalStakersPowers := make(map[string]math.LegacyDec), math.LegacyNewDec(0) |
There was a problem hiding this comment.
Recheck this @coderabbitai
| // x/feedistribution module sentinel errors | ||
| var ( | ||
| ErrEpochNotFound = sdkerrors.Register( | ||
| ModuleName, 1102, |
There was a problem hiding this comment.
Any reason to start with 1102 and not 2?
| func (p Params) Validate() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| func (p Params) Validate() error { | |
| return nil | |
| } | |
| func (p Params) Validate() error { | |
| if err := epochstypes.ValidateEpochIdentifierString(p.EpochIdentifier); err != nil { | |
| return fmt.Errorf("invalid epoch ID %s", err) | |
| } | |
| if p.CommunityTax.IsNil() || p.CommunityTax.IsNegative() || p.CommunityTax.GT(math.LegacyOneDec()) { | |
| return fmt.Errorf("invalid community tax %d", p.CommunityTax) | |
| } | |
| return nil | |
| } |
|
Minor changes suggested; looks good. |
Description
Add the distribution module logic and minting reward related.
The distribution mainly contains two parts:
And generally, these will be allocated on epoch-basis, say, one day. And upon the epoch end, all these two hooks will be triggered to distribute the reward.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes