-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
feat(x/mint)!: Replace InflationCalculationFn with MintFn + simple epoch minting #20363
base: main
Are you sure you want to change the base?
Conversation
Warning Rate limit exceeded@facundomedica has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 41 minutes and 24 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce new minting functionalities, including a Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
event.NewAttribute(types.AttributeKeyAnnualProvisions, minter.AnnualProvisions.String()), | ||
event.NewAttribute(sdk.AttributeKeyAmount, mintedCoin.Amount.String()), | ||
) | ||
return k.Minter.Set(ctx, minter) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
@@ -156,7 +157,7 @@ | |||
|
|||
// BeginBlock returns the begin blocker for the mint module. | |||
func (am AppModule) BeginBlock(ctx context.Context) error { | |||
return am.keeper.BeginBlocker(ctx, am.inflationCalculator) | |||
return am.keeper.BeginBlocker(ctx, am.mintFn) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
for k, v := range *x.m { | ||
mapKey := (protoreflect.MapKey)(protoreflect.ValueOfString(k)) | ||
mapValue := protoreflect.ValueOfString(v) | ||
if !f(mapKey, mapValue) { | ||
break | ||
} | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
for k, v := range x.ArbitraryParams { | ||
SiZeMaP(k, v) | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
for k := range x.ArbitraryParams { | ||
v := x.ArbitraryParams[k] | ||
out, err := MaRsHaLmAp(k, v) | ||
if err != nil { | ||
return out, err | ||
} | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
for k := range m.ArbitraryParams { | ||
v := m.ArbitraryParams[k] | ||
baseI := i | ||
i -= len(v) | ||
copy(dAtA[i:], v) | ||
i = encodeVarintMint(dAtA, i, uint64(len(v))) | ||
i-- | ||
dAtA[i] = 0x12 | ||
i -= len(k) | ||
copy(dAtA[i:], k) | ||
i = encodeVarintMint(dAtA, i, uint64(len(k))) | ||
i-- | ||
dAtA[i] = 0xa | ||
i = encodeVarintMint(dAtA, i, uint64(baseI-i)) | ||
i-- | ||
dAtA[i] = 0x1a | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
for k, v := range m.ArbitraryParams { | ||
_ = k | ||
_ = v | ||
mapEntrySize := 1 + len(k) + sovMint(uint64(len(k))) + 1 + len(v) + sovMint(uint64(len(v))) | ||
n += mapEntrySize + 1 + sovMint(uint64(mapEntrySize)) | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
@facundomedica your pull request is missing a changelog! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
x/epochs/keeper/keeper_test.go (1)
Line range hint
52-70
: Update toSetup
function to return a pointer and initialize the genesis state is well-implemented. Consider adding a test to verify the correct initialization of the genesis state.Would you like me to help by adding a test case for genesis state initialization?
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (7)
simapp/go.mod
is excluded by!**/*.mod
tests/go.mod
is excluded by!**/*.mod
x/group/go.mod
is excluded by!**/*.mod
x/group/go.sum
is excluded by!**/*.sum
x/mint/go.mod
is excluded by!**/*.mod
x/mint/go.sum
is excluded by!**/*.sum
x/mint/types/mint.pb.go
is excluded by!**/*.pb.go
Files selected for processing (22)
- api/cosmos/mint/v1beta1/mint.pulsar.go (17 hunks)
- simapp/app.go (1 hunks)
- simapp/app_di.go (3 hunks)
- simapp/mint_fn.go (1 hunks)
- tests/e2e/bank/grpc.go (4 hunks)
- tests/e2e/distribution/grpc_query_suite.go (3 hunks)
- x/epochs/depinject.go (2 hunks)
- x/epochs/keeper/epoch_test.go (1 hunks)
- x/epochs/keeper/genesis_test.go (1 hunks)
- x/epochs/keeper/grpc_query.go (1 hunks)
- x/epochs/keeper/grpc_query_test.go (1 hunks)
- x/epochs/keeper/keeper.go (2 hunks)
- x/epochs/keeper/keeper_test.go (3 hunks)
- x/epochs/module.go (1 hunks)
- x/epochs/types/genesis.go (1 hunks)
- x/mint/depinject.go (4 hunks)
- x/mint/epoch_hooks.go (1 hunks)
- x/mint/keeper/abci.go (2 hunks)
- x/mint/keeper/keeper.go (2 hunks)
- x/mint/module.go (2 hunks)
- x/mint/proto/cosmos/mint/v1beta1/mint.proto (1 hunks)
- x/mint/types/genesis.go (1 hunks)
Files not summarized due to errors (1)
- api/cosmos/mint/v1beta1/mint.pulsar.go: Error: Message exceeds token limit
Additional Context Used
GitHub Check Runs (1)
CodeQL success (7)
api/cosmos/mint/v1beta1/mint.pulsar.go: [warning] 36-42: Iteration over map
Iteration over map may be a possible source of non-determinism
api/cosmos/mint/v1beta1/mint.pulsar.go: [warning] 441-443: Iteration over map
Iteration over map may be a possible source of non-determinism
api/cosmos/mint/v1beta1/mint.pulsar.go: [warning] 509-515: Iteration over map
Iteration over map may be a possible source of non-determinism
x/mint/keeper/abci.go: [warning] 28-28: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
x/mint/module.go: [warning] 161-161: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
Path-based Instructions (21)
x/epochs/keeper/grpc_query_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/mint/keeper/abci.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/epoch_hooks.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/epochs/keeper/keeper.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/types/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/epochs/keeper/grpc_query.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/epochs/depinject.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/epochs/types/genesis.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/depinject.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/epochs/keeper/keeper_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/epochs/keeper/genesis_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/epochs/keeper/epoch_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/epochs/module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/mint_fn.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/keeper.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/bank/grpc.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/distribution/grpc_query_suite.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"simapp/app_di.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/mint/v1beta1/mint.pulsar.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Learnings (1)
x/epochs/keeper/genesis_test.go (1)
User: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction. - The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module. - The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
Additional comments not posted (24)
x/mint/keeper/abci.go (1)
28-28
: Ensure that theSet
method inMinter
handles all possible error scenarios to prevent panics.x/mint/epoch_hooks.go (1)
16-29
: Ensure that themintFn
used inBeforeEpochStart
is robust against all potential errors, especially since it interacts with external state.x/epochs/keeper/keeper.go (1)
Line range hint
22-35
: Ensure that thepanic
inNewKeeper
is handled or logged appropriately to aid in debugging.x/mint/types/genesis.go (1)
10-10
: The newMintFn
type provides the necessary flexibility for custom minting operations as described in the PR. Ensure it is well-documented.x/epochs/keeper/grpc_query.go (1)
18-18
: The changes to theQuerier
struct and theNewQuerier
function are correct and improve the handling of the keeper instance.Also applies to: 22-22
x/epochs/depinject.go (1)
42-42
: Change from value to pointer type forEpochKeeper
looks good. Ensure all usages correctly handle the pointer semantics.Verification successful
The usages of
EpochKeeper
correctly handle pointer semantics.
ProvideModule
assignsEpochKeeper
the result ofkeeper.NewKeeper
, which returns a pointer.InvokeSetHooks
acceptsEpochKeeper
as a pointer argument.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `EpochKeeper` handle pointer semantics. # Test: Search for usages of `EpochKeeper`. Expect: Proper handling of pointers. rg --type go 'EpochKeeper' --context 5Length of output: 851
x/epochs/types/genesis.go (1)
20-20
: Addition of the "minute" epoch aligns with the PR's objectives. Ensure it is integrated correctly in the system.Verification successful
The "minute" epoch is correctly integrated into the system, as evidenced by its usage in multiple relevant files.
x/tx/signing/textual/duration.go
: Formatting of durations includes "minute".x/epochs/types/genesis.go
: Added to the genesis configuration.x/epochs/keeper/epoch_test.go
: Included in the epoch tests.simapp/mint_fn.go
: Used in the minting function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the "minute" epoch. # Test: Search for usages of the "minute" epoch. Expect: Correct integration. rg --type go '"minute"' --context 5Length of output: 3333
x/mint/depinject.go (2)
9-9
: Addition ofepochstypes
import is necessary for the new integration with the epochs module.
46-46
: Integration ofEpochHooks
and handling ofMintFn
inProvideModule
are well-implemented. Verify the integration and fallback mechanisms.Also applies to: 76-76, 79-79
Verification successful
The integration of
EpochHooks
andMintFn
is well-implemented across the codebase. The fallback mechanism forMintFn
is correctly handled.
MintFn
is used inx/mint/module.go
,x/mint/depinject.go
,x/mint/keeper/keeper.go
, andsimapp/mint_fn.go
.EpochHooks
is used inx/mint/epoch_hooks.go
,x/mint/depinject.go
,x/epochs/types/hooks.go
, andx/epochs/keeper/keeper.go
.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `EpochHooks` and `MintFn`. # Test: Search for usages of `EpochHooks` and `MintFn`. Expect: Correct integration and fallback. rg --type go 'EpochHooks|MintFn' --context 5Length of output: 16300
x/epochs/keeper/keeper_test.go (1)
30-30
: Transition to pointer type forEpochsKeeper
in tests is consistent with main code changes. Ensure all test cases correctly handle the pointer type.Verification successful
The previous script failed because the specified directory or file does not exist. Let's locate the correct directory or file containing the test cases for
EpochsKeeper
and verify the handling ofEpochsKeeper
pointers.
The script successfully located the test files and found occurrences of
EpochsKeeper
in several test files, includingx/epochs/keeper/keeper_test.go
. Let's review the context around these occurrences to ensure proper handling of the pointer type.
The script successfully located the test files and found occurrences of
EpochsKeeper
in several test files, includingx/epochs/keeper/keeper_test.go
. The context around these occurrences shows that theEpochsKeeper
pointer type is being handled correctly in the test cases.Transition to pointer type for
EpochsKeeper
in tests is consistent with main code changes. Ensure all test cases correctly handle the pointer type.
x/epochs/keeper/keeper_test.go
x/epochs/keeper/epoch_test.go
x/epochs/keeper/abci_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all test cases handle the `EpochsKeeper` pointer type correctly. # Test: Search for test cases using `EpochsKeeper`. Expect: Correct handling of pointers. rg --type go 'EpochsKeeper' 'x/epochs/keeper/*_test.go' --context 5Length of output: 133
Script:
#!/bin/bash # Description: Locate the correct directory or file containing the test cases for `EpochsKeeper` and verify the handling of `EpochsKeeper` pointers. # Step 1: Find all Go test files in the repository. test_files=$(fd --type f --extension go --glob '*_test.go') # Step 2: Search for occurrences of `EpochsKeeper` in the test files. for file in $test_files; do echo "Checking file: $file" rg --type go 'EpochsKeeper' "$file" --context 5 doneLength of output: 178958
Script:
#!/bin/bash # Description: Review the context around the occurrences of `EpochsKeeper` in the test files to ensure proper handling of the pointer type. # Step 1: Find all Go test files in the repository. test_files=$(fd --type f --extension go --glob '*_test.go') # Step 2: Search for occurrences of `EpochsKeeper` in the test files and display the context. for file in $test_files; do echo "Checking file: $file" rg --type go 'EpochsKeeper' "$file" --context 10 doneLength of output: 180975
x/epochs/keeper/genesis_test.go (1)
21-21
: Update to expect four epochs in the genesis test is correct. Verify that the genesis state is correctly initialized elsewhere in the system.x/epochs/keeper/epoch_test.go (1)
96-101
: Updated assertions correctly reflect the addition of the "minute" epoch and its order among other epochs.x/epochs/module.go (1)
39-43
: Transitioningkeeper
from a value to a pointer in theAppModule
struct and its constructor enhances performance and maintainability.simapp/mint_fn.go (1)
19-123
: TheProvideExampleMintFn
function is well-implemented, with comprehensive error handling and condition checks for minting based on the epoch ID.x/mint/keeper/keeper.go (1)
107-180
: TheDefaultMintFn
function encapsulates the minting logic effectively, with appropriate checks for conditions like epoch ID and max supply, and includes comprehensive error handling.x/mint/module.go (1)
47-69
: Adding amintFn
field to theAppModule
struct and handling it in theNewAppModule
constructor enhances the flexibility of the mint module by allowing for custom minting logic.tests/e2e/bank/grpc.go (1)
Line range hint
38-75
: Updated expected amounts in GRPC handler test cases correctly reflect the new staking tokens configuration.tests/e2e/distribution/grpc_query_suite.go (3)
286-286
: Updated delegator rewards value to "60.27stake" appears correct given the PR context.
173-173
: Updated commission value to "60.27stake" appears correct given the PR context.Verification successful
The search results show that the value "60.27stake" appears in two places within the same file: once for commission and once for rewards. To ensure the correctness of the updated commission value, we need to verify if this value is consistent with the actual commission calculation logic in the codebase.
To verify the correctness of the updated commission value, we need to inspect the implementation of the
SetInitialCommission
andbuildCommissionRates
functions, as they seem directly related to commission settings.
To fully understand the commission calculation, we need the complete implementation of both the
SetInitialCommission
andbuildCommissionRates
functions.
To fully verify the commission calculation, we still need the complete implementation of the
SetInitialCommission
function.
The command to extract the remaining part of the
SetInitialCommission
function did not produce any output. To ensure we capture the full implementation of this function, we need to use a broader search pattern.
The updated commission value in the test is consistent with the commission calculation logic in the codebase.
- The
SetInitialCommission
function validates and sets the commission for the validator.- The
buildCommissionRates
function converts string inputs to decimal values for commission rates.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the updated commission value in the distribution logic. # Test: Search for the commission calculation logic. Expect: Consistency with the new test value. rg --type go $'60.27stake'Length of output: 212
Script:
#!/bin/bash # Description: Search for the commission calculation logic in the codebase to verify consistency with the test value. # Search for functions or methods related to commission calculation. rg --type go 'commission' | grep -i 'func\|method'Length of output: 1247
Script:
#!/bin/bash # Description: Extract and review the implementation of the SetInitialCommission and buildCommissionRates functions to verify commission calculation logic. # Extract the implementation of the SetInitialCommission function. rg --type go --context 10 'func (v Validator) SetInitialCommission' x/staking/types/validator.go # Extract the implementation of the buildCommissionRates function. rg --type go --context 10 'func buildCommissionRates' x/staking/client/cli/utils.goLength of output: 857
Script:
#!/bin/bash # Description: Extract the full implementation of the SetInitialCommission and buildCommissionRates functions to verify commission calculation logic. # Extract the full implementation of the SetInitialCommission function. rg --type go --context 20 'func (v Validator) SetInitialCommission' x/staking/types/validator.go # Extract the remaining part of the buildCommissionRates function. rg --type go --context 20 'func buildCommissionRates' x/staking/client/cli/utils.goLength of output: 1436
Script:
#!/bin/bash # Description: Extract the remaining part of the SetInitialCommission function to verify commission calculation logic. # Extract the remaining part of the SetInitialCommission function. rg --type go --context 30 'func (v Validator) SetInitialCommission' x/staking/types/validator.goLength of output: 98
Script:
#!/bin/bash # Description: Use a broader search pattern to locate the full implementation of the SetInitialCommission function. # Search for the full implementation of the SetInitialCommission function. rg --type go --context 50 'SetInitialCommission' x/staking/types/validator.goLength of output: 3735
119-119
: Updated reward value to "120.54stake" appears correct given the PR context.simapp/app_di.go (2)
96-96
: Transition ofEpochsKeeper
to pointer type aligns with the PR's objectives to enhance performance and manageability.
115-115
: Addition ofProvideExampleMintFn
to the dependency injection setup is consistent with the PR's objectives to introduce new minting logic.simapp/app.go (1)
174-174
: Ensure thatEpochsKeeper
is properly instantiated before use to avoid nil pointer dereferences.api/cosmos/mint/v1beta1/mint.pulsar.go (1)
441-443
: Verify deterministic behavior in map iteration during size calculation.The iteration over
x.ArbitraryParams
in theSize
method offastReflection_Minter
might lead to non-deterministic size calculations. If the order of map entries affects the size calculation, consider implementing a sorting mechanism before iteration.Verification successful
The
Size
function infastReflection_Minter
already includes a mechanism to ensure deterministic behavior when iterating overx.ArbitraryParams
. Whenoptions.Deterministic
is true, the keys are sorted before iteration, ensuring consistent size calculations.
- Lines 427-443 in
api/cosmos/mint/v1beta1/mint.pulsar.go
handle this correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the map order affects the size calculation logic. grep -rn 'Size' api/cosmos/mint/v1beta1/mint.pulsar.goLength of output: 944
Script:
#!/bin/bash # Extract the relevant portion of the Size function to analyze the iteration over x.ArbitraryParams sed -n '400,460p' api/cosmos/mint/v1beta1/mint.pulsar.goLength of output: 1835
@@ -49,7 +49,7 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { | |||
return ModuleOutputs{EpochKeeper: k, Module: m} | |||
} | |||
|
|||
func InvokeSetHooks(keeper keeper.Keeper, hooks map[string]types.EpochHooksWrapper) error { | |||
func InvokeSetHooks(keeper *keeper.Keeper, hooks map[string]types.EpochHooksWrapper) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to accept a pointer in InvokeSetHooks
is consistent with other changes. Consider adding a test to verify behavior when hooks
is nil.
Would you like me to help by adding a test case for this scenario?
for k, v := range *x.m { | ||
mapKey := (protoreflect.MapKey)(protoreflect.ValueOfString(k)) | ||
mapValue := protoreflect.ValueOfString(v) | ||
if !f(mapKey, mapValue) { | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address potential non-determinism in map iteration.
The iteration over the map x.m
in the Range
method could lead to non-deterministic behavior, which might be problematic depending on how the results are used. Consider sorting the keys before iteration if the order impacts the application logic.
ModuleKey depinject.OwnModuleKey | ||
Config *modulev1.Module | ||
Environment appmodule.Environment | ||
Cdc codec.Codec | ||
MintFn types.MintFn `optional:"true"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduction of MintFn
in ModuleInputs
enhances flexibility. Consider adding documentation to explain its optional nature.
+ // MintFn is an optional field that allows custom minting logic.
Committable suggestion was skipped due low confidence.
@@ -21,6 +21,8 @@ message Minter { | |||
(gogoproto.customtype) = "cosmossdk.io/math.LegacyDec", | |||
(gogoproto.nullable) = false | |||
]; | |||
|
|||
map<string, string> arbitrary_params = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addition of arbitrary_params
to the Minter
message enhances customization. Consider documenting its intended use cases.
+ // arbitrary_params can be used to pass custom parameters for minting operations.
Committable suggestion was skipped due low confidence.
@@ -35,7 +35,7 @@ func (s *E2ETestSuite) TestTotalSupplyGRPCHandler() { | |||
&types.QueryTotalSupplyResponse{ | |||
Supply: sdk.NewCoins( | |||
sdk.NewCoin(fmt.Sprintf("%stoken", val.GetMoniker()), s.cfg.AccountTokens), | |||
sdk.NewCoin(s.cfg.BondDenom, s.cfg.StakingTokens.Add(math.NewInt(10))), | |||
sdk.NewCoin(s.cfg.BondDenom, s.cfg.StakingTokens.Add(math.NewInt(123))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the increase ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because now it mints the equivalent of a minute in the first block (the beginning of the epoch)
@@ -39,7 +39,7 @@ type ModuleInputs struct { | |||
type ModuleOutputs struct { | |||
depinject.Out | |||
|
|||
EpochKeeper keeper.Keeper | |||
EpochKeeper *keeper.Keeper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it need to be a pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug with x/epochs, in which hooks weren't being set. The issue was that we were doing (k Keeper) SetHooks(...
instead of (k *Keeper) SetHooks(...
. I copied what we do in x/gov to achieve the same
x/epochs/keeper/grpc_query.go
Outdated
@@ -15,11 +15,11 @@ var _ types.QueryServer = Querier{} | |||
// Querier defines a wrapper around the x/epochs keeper providing gRPC method | |||
// handlers. | |||
type Querier struct { | |||
Keeper | |||
*Keeper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be able to change this one back, as it's just the querier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a question but overall it looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; only nit is that I'm not sure if i like the exception of blcok minting vs epoch minting.
Maybe two separate functions, IDK might be too verbose!
Or maybe being more explicit on the mint fn when it's epoch vs block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
x/mint/keeper/keeper_test.go (1)
Line range hint
1-72
: Ensure proper cleanup of resources after tests.Consider adding a
TearDownTest
method to clean up resources, such as closing thegomock.Controller
and any other necessary cleanup to prevent resource leakage during testing.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- x/mint/keeper/keeper.go (2 hunks)
- x/mint/keeper/keeper_test.go (1 hunks)
- x/mint/module_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/mint/keeper/keeper.go
Additional Context Used
Path-based Instructions (2)
x/mint/module_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/mint/keeper/keeper_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
x/mint/module_test.go (1)
80-91
: Improve test coverage for error scenarios.
[REFACTOR_SUGGESTion]
Add negative test cases toTestEpochHooks
to ensure robust error handling. For example, test the behavior whenMintCoins
orSendCoinsFromModuleToModule
fails.x/mint/keeper/keeper_test.go (1)
137-155
: Verify state consistency inTestBeginBlocker
.Add assertions to verify that the state of the minter before and after the
BeginBlocker
call is consistent with the expected changes. This will help ensure that theBeginBlocker
is modifying the state as intended.
So the mint fn gets to decide when to be executed, so we still need to do the epoch id check. What we don't have is an epoch that is executed on every block, that's why I added the "block" epoch ID. We could remove the EndBlock call, but it would be too much breakage imo. I think we should deprecate it, not sure how tho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/mint/module_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/mint/module_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start!
I am bit worried about atomic behaviour in the mint function but I don't have the full picture yet.
|
||
// ProvideExampleMintFn returns the function used in x/mint's endblocker to mint new tokens. | ||
// Note that this function can not have the mint keeper as a parameter because it would create a cyclic dependency. | ||
func ProvideExampleMintFn(bankKeeper bankkeeper.Keeper) minttypes.MintFn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personal preference: an interface would be great to decouple this instead of a keeper
@@ -0,0 +1,123 @@ | |||
package simapp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in simapp? I would expect this code to be in the mint module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is the example that developers can use to develop their own mint function. The idea here is that anyone creates their own minting function if they want to, and it works without requiring direct usage of keepers as it uses the query routers.
// ProvideExampleMintFn returns the function used in x/mint's endblocker to mint new tokens. | ||
// Note that this function can not have the mint keeper as a parameter because it would create a cyclic dependency. | ||
func ProvideExampleMintFn(bankKeeper bankkeeper.Keeper) minttypes.MintFn { | ||
return func(ctx context.Context, env appmodule.Environment, minter *minttypes.Minter, epochId string, epochNumber int64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be epochID
with uppercase D
func ProvideExampleMintFn(bankKeeper bankkeeper.Keeper) minttypes.MintFn { | ||
return func(ctx context.Context, env appmodule.Environment, minter *minttypes.Minter, epochId string, epochNumber int64) error { | ||
// in this example we ignore epochNumber as we don't care what epoch we are in, we just assume we are being called every minute. | ||
if epochId != "minute" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stumbled upon this is the unit coming from the epoch serviceminute
as ID. This looks very ambiguous.
minter.AnnualProvisions = minter.NextAnnualProvisions(mintParams.Params, stakingTokenSupply.Amount) | ||
|
||
// because we are minting every minute, we need to divide the annual provisions by minutes in a year (525600) | ||
provisionAmt := minter.AnnualProvisions.QuoInt64(525600) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the mint amount still correct if the chain halts? Time would pass although no blocks were produced on the halt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current design it works like this. But yes, if the developer would like to have a a more accurate minting, we should keep track of the last epoch and mint accordingly.
@@ -112,6 +112,7 @@ func init() { | |||
func AppConfig() depinject.Config { | |||
return depinject.Configs( | |||
appConfig, // Alternatively use appconfig.LoadYAML(AppConfigYAML) | |||
depinject.Provide(ProvideExampleMintFn), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very familiar with the DI framework and not sure what happens here. Does this overwrite the default mint function? If yes, this can add some risk and unexpected behaviour. In any case, some doc would be great.
App v1 seems to have the default mint function set
return am.Name() | ||
} | ||
|
||
// BeforeEpochStart is a noop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment still correct?
} | ||
|
||
// mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(mintParams.Params.MintDenom)).LT(maxSupply) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when total + minted == max supply, then neither L67 or this condition fits and no coins would be minted although a sub amount would be possible. I think L67 should handle this
} | ||
|
||
// mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).LT(maxSupply) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue when total+minted == max that is ignored
@@ -101,3 +103,83 @@ func (k Keeper) MintCoins(ctx context.Context, newCoins sdk.Coins) error { | |||
func (k Keeper) AddCollectedFees(ctx context.Context, fees sdk.Coins) error { | |||
return k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, fees) | |||
} | |||
|
|||
func (k Keeper) DefaultMintFn(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personal preference: it is ok to have this defined at the keeper but an independent method like ProvideExampleMintFn
would be nicer to compare and test.
params, err := k.Params.Get(ctx) | ||
// we pass -1 as epoch number to indicate that this is not an epoch minting, | ||
// but a regular block minting. Same with epoch id "block". | ||
err = mintFn(ctx, k.Environment, &minter, "block", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is atomic behaviour ensured or will this halt the chain? The branch service is not used afaik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/mint/types/minter_test.go (2 hunks)
- x/mint/types/params.go (6 hunks)
Additional context used
Path-based instructions (2)
x/mint/types/params.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/types/minter_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (3)
x/mint/types/params.go (1)
72-72
: The refactoring of validation functions to use specific types instead ofinterface{}
enhances type safety and clarity. This change aligns well with best practices in Go for strong typing and error handling.Also applies to: 83-83, 97-97, 111-111, 125-125, 139-139, 147-147
x/mint/types/minter_test.go (2)
58-60
: The testTestNextInflation
correctly calculates annual provisions and uses assertions to validate the expected outcomes. This ensures that the minting logic adheres to the specified rules.
94-111
: The new testTestValidateMinter
effectively checks theMinter
struct's validation logic under various scenarios. This addition enhances the test coverage and helps ensure the robustness of the minter's validation logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- x/epochs/keeper/keeper_test.go (4 hunks)
- x/epochs/module.go (2 hunks)
- x/mint/types/genesis_test.go (1 hunks)
- x/mint/types/params_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/epochs/keeper/keeper_test.go
- x/epochs/module.go
Additional context used
Path-based instructions (2)
x/mint/types/genesis_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/mint/types/params_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
x/mint/types/genesis_test.go (1)
9-22
: The test functionTestGenesis
effectively validates the default genesis state and checks its consistency with a manually created genesis state. Good coverage for genesis state validation.x/mint/types/params_test.go (1)
10-111
: The test functionTestValidate
provides comprehensive coverage for parameter validation, testing various edge cases and ensuring that incorrect values are appropriately handled. Excellent work on ensuring robustness in parameter validation.
Description
MintFn is a breaking replacement for InflationCalculationFn.
Until now, developers could only modify minting by passing an inflation calculation function that could take in some x/mint parameters and the bonded ratio from x/staking. The rest of the minting process was being handled by the module, so minting to custom accounts was not possible without having to create a separate module for this.
Now
MintFn
...[WIP], will remove minter and params?
I'm missing docs but would like to get a pre-approval before starting to write them
Before & after
Before only the "Calculate Inflation" step was user/dev defined.
Now the entire MintFn is user defined, so the user can make it work like it used to work:
Or it can get creative and do other stuff:
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests