fix: initialize staking at genesis #290
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/ibc-hooks/move-hooks/common_test.go (1)
312-312: Consider adding test cases with custom bond denominations.To ensure robust testing of the staking initialization, consider adding test cases that use different bond denominations beyond the default one.
Example test setup:
func TestStakingInitializationWithCustomBondDenom(t *testing.T) { customBondDenom := "custom" ctx, keepers := createTestInput(t, false, true) // Verify initialization with custom bond denom // Add assertions here }x/slashing/keeper/common_test.go (1)
392-392: Consider adding specific test cases for genesis staking initializationWhile the initialization change looks good, consider adding test cases that specifically verify the genesis staking initialization behavior with different bond denominations to ensure the fix works as intended.
Would you like me to help create test cases that verify the genesis staking initialization with different bond denominations?
x/move/keeper/staking_test.go (1)
250-270: Enhance error handling verification.The error checks could be more thorough:
- Verify specific error messages for failure cases
- Consider using
require.ErrorContainsfor more precise error checking- Consider combining multiple error checks using a helper function
Example helper function:
func requireNoErrorf(t *testing.T, err error, format string, args ...interface{}) { t.Helper() if err != nil { t.Errorf(format+": %v", append(args, err)...) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- x/bank/keeper/common_test.go (1 hunks)
- x/distribution/keeper/common_test.go (1 hunks)
- x/gov/keeper/common_test.go (1 hunks)
- x/ibc-hooks/keeper/common_test.go (1 hunks)
- x/ibc-hooks/move-hooks/common_test.go (1 hunks)
- x/move/keeper/common_test.go (1 hunks)
- x/move/keeper/genesis.go (3 hunks)
- x/move/keeper/staking_test.go (1 hunks)
- x/mstaking/keeper/common_test.go (1 hunks)
- x/reward/keeper/common_test.go (1 hunks)
- x/slashing/keeper/common_test.go (1 hunks)
🔇 Additional comments (15)
x/move/keeper/genesis.go (4)
18-18: LGTM: Parameter addition aligns with PR objective.The addition of
baseDenomparameter is appropriate for initializing staking at genesis.
61-71: Verify initialization order dependency.The code initializes move bank coin before move staking. Please verify that this order is required and documented.
Consider:
- Adding a comment explaining the required initialization order
- Documenting this dependency in the module's README or design docs
93-95: Verify genesis parameter handling.The initialization is only performed when no modules exist (
len(genState.GetModules()) == 0). Please verify:
- This is the intended behavior for staking initialization
- The
params.BaseDenomis properly set in the genesis file#!/bin/bash # Search for genesis.json examples or tests that set BaseDenom rg -g "*.json" -A 5 "BaseDenom"
57-59: Review duplicate error handling.There are two calls to
handleExecuteResponse- one at line 57 and another at line 73. The second call could potentially overwrite the error state from the staking initialization block. Consider consolidating the error handling.Also applies to: 73-73
x/reward/keeper/common_test.go (1)
378-379: LGTM: Properly integrated bondDenom parameterThe addition of bondDenom parameter to moveKeeper.Initialize is well integrated with the test setup. The parameter is consistent with moveParams.BaseDenom and properly initialized for staking at genesis.
Let's verify the integration:
✅ Verification successful
Let me generate additional scripts to verify the BaseDenom consistency, as the previous search didn't capture the full context.
Let me try one more time with corrected paths and patterns:
Based on the search results, I can now generate the final response:
✅ Verified: bondDenom parameter is consistently integrated
The verification shows that:
- The
bondDenomparameter is consistently used across all test setups in different modules- In all cases,
moveParams.BaseDenomis properly set tobondDenombefore callingInitialize- The initialization sequence is consistent:
- Set
moveParams.BaseDenom = bondDenom- Set raw params via
SetRawParams- Load stdlib module bytes
- Call
Initializewith correct parameters includingbondDenom🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that bondDenom is consistently used across the codebase # Test 1: Check if bondDenom is properly used in Initialize calls rg -A 2 "Initialize.*bondDenom" # Test 2: Check if BaseDenom is consistently set to bondDenom before Initialize rg -B 5 "Initialize.*bondDenom" | rg "BaseDenom.*bondDenom"Length of output: 2009
Script:
#!/bin/bash # Let's check the full context around Initialize calls to verify BaseDenom setup # Using more context lines and a broader search pattern # Check the context of Initialize calls in test files rg -B 10 "Initialize.*bondDenom" common_test.go # Specifically look for moveParams setup before Initialize rg -B 10 "moveParams.*BaseDenom.*bondDenom" common_test.go # Check the Initialize method signature to understand the expected parameters ast-grep --pattern 'func $_Initialize($$$) { $$$ }'Length of output: 276
Script:
#!/bin/bash # Search for Initialize implementation ast-grep --pattern 'func ($_ *Keeper) Initialize($$$) { $$$ }' # Check the context of Initialize calls in test files, searching all directories rg -B 10 "Initialize.*bondDenom" --type go # Look for moveParams.BaseDenom setup rg -B 5 "moveParams\.BaseDenom.*=.*bondDenom" --type goLength of output: 9872
x/ibc-hooks/move-hooks/common_test.go (1)
312-312: LGTM! Verify test coverage for different bond denominations.The addition of bondDenom parameter to moveKeeper.Initialize is correct and aligns with the PR objective of fixing staking initialization at genesis.
Let's verify the test coverage:
x/ibc-hooks/keeper/common_test.go (1)
399-399: LGTM: Consistent bond denomination initializationThe addition of
bondDenomparameter tomoveKeeper.Initializeis consistent with the staking configuration across other keepers and properly uses the defined constant.x/bank/keeper/common_test.go (1)
397-397: LGTM: Proper initialization of moveKeeper with bondDenomThe addition of
bondDenomparameter tomoveKeeper.Initializeis correct and consistent with:
- The PR's objective to fix staking initialization at genesis
- The test setup where
bondDenomis properly defined as a constant- Other related initialization like
stakingParamsandrewardParamswhich also use the samebondDenomx/mstaking/keeper/common_test.go (1)
423-424: LGTM! Verify test coverage for staking initialization.The addition of
bondDenomparameter tomoveKeeper.Initializealigns with the PR objective of fixing staking initialization at genesis. Since this is a common test setup, it ensures all staking-related tests use the correct bond denomination.Let's verify the test coverage for staking initialization:
✅ Verification successful
Let me gather more information to verify the test coverage comprehensively.
Based on the search results, I can now provide a comprehensive verification of the test coverage.
Test coverage for
moveKeeper.InitializewithbondDenomis comprehensiveThe search results show that:
The
Initializemethod withbondDenomparameter is properly tested across multiple test suites:
- x/mstaking/keeper/common_test.go
- x/reward/keeper/common_test.go
- x/slashing/keeper/common_test.go
- x/distribution/keeper/common_test.go
- x/gov/keeper/common_test.go
- x/bank/keeper/common_test.go
- x/ibc-hooks/keeper/common_test.go
The initialization flow is also tested through:
- Genesis initialization tests
- Staking-specific initialization via
InitializeStakingmethod- Integration with other modules' test setups
The change to add
bondDenomparameter is well-covered across the test suite, ensuring the staking initialization works correctly with the proper bond denomination at genesis.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for staking initialization # Look for test cases that verify staking initialization with bond denomination # Search for test functions related to staking initialization rg -t go "func.*Test.*Init.*Staking" # Search for test assertions related to bond denomination rg -t go "require.*bond.*denom"Length of output: 525
Script:
#!/bin/bash # Search for Initialize method usage in test files rg -t go "Initialize.*\(" -A 2 # Search for test functions that might be testing initialization rg -t go "func.*Test.*Initialize" -A 5 # Look for genesis-related test functions since initialization happens at genesis rg -t go "func.*Test.*Genesis" -A 5 # Search for moveKeeper initialization patterns ast-grep --pattern 'moveKeeper.Initialize($$$)'Length of output: 11605
x/gov/keeper/common_test.go (1)
409-409: LGTM: Consistent bond denomination initializationThe addition of
bondDenomparameter tomoveKeeper.Initializealigns with the PR's objective of fixing staking initialization at genesis and maintains consistency with the bond denomination handling throughout the test setup.x/slashing/keeper/common_test.go (1)
392-392: LGTM: Proper integration of bondDenom parameterThe initialization change correctly integrates the
bondDenomparameter, maintaining consistency with other test files and properly handling errors.x/distribution/keeper/common_test.go (3)
381-382: LGTM: Bond denomination properly initialized in move keeper.The addition of
bondDenomparameter tomoveKeeper.Initializealigns with the PR's objective of fixing staking initialization at genesis. The initialization is properly handled with error checking.
381-382: Verify test coverage for move keeper initialization.The initialization of the move keeper with bond denomination is a critical change. Let's ensure there are adequate test cases covering this initialization.
#!/bin/bash # Description: Check for test coverage of move keeper initialization # Expected: Should find test cases specifically testing initialization with bond denomination # Search for test cases related to move keeper initialization rg -A 5 "func Test.*Initialize" ast-grep --pattern 'func Test$_($_) { $$$ moveKeeper.Initialize($$$) $$$ }'Also applies to: 171-175
381-382: Verify bond denomination consistency across the test setup.The bond denomination is defined as a constant and used consistently in the initialization. Let's verify its usage across the codebase to ensure consistent initialization.
Also applies to: 67-68
✅ Verification successful
Let me gather more specific information about the bond denomination usage.
This will help us verify the consistency of bond denomination usage in the test setup and across the codebase.
Let me generate the final response based on the gathered information.
Bond denomination is consistently defined and used across test files
The verification shows that:
- The
bondDenomconstant inx/distribution/keeper/common_test.gois correctly defined usinginitiaapp.BondDenom- The bond denomination is consistently used in the
Initializecall- The same bond denomination constant is used consistently across various test files in different modules (distribution, gov, bank, ibc-hooks, etc.)
- The bond denomination is properly set in staking parameters across all test setups
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of bond denomination in test files # Expected: All test files should use the same bond denomination constant # Search for bond denomination definitions and usage in test files rg -l "bondDenom.*=.*initiaapp\.BondDenom" rg -l "\.Initialize\(.*bondDenom\)"Length of output: 505
Script:
#!/bin/bash # Search for bond denomination definition and usage in test files rg -A 3 "bondDenom.*:?=.*" x/distribution/keeper/common_test.go rg -A 3 "Initialize.*bondDenom" x/distribution/keeper/common_test.go # Search for other test files that might be using Initialize with bond denomination rg -A 3 "Initialize.*bondDenom" "**/*_test.go" # Look for bond denomination constant definitions rg -A 1 "BondDenom.*=.*"Length of output: 8504
x/move/keeper/common_test.go (1)
422-422: LGTM! Verify integration with staking parameters.The addition of
bondDenomparameter tomoveKeeper.Initializealigns with the PR objective of fixing staking initialization at genesis. The parameter is properly sourced from the constant and matches the staking parameters set earlier in the test setup.✅ Verification successful
The shell script results show that:
- The
bondDenomparameter is consistently used across all test files for staking parameters configuration viastakingParams.BondDenoms = []string{bondDenom}- The
moveKeeper.Initializemethod is called with thebondDenomparameter consistently across all test files in various modules (bank, gov, distribution, ibc-hooks, move, mstaking, slashing, reward)This verifies that the integration is consistent across the codebase, and the change aligns with the staking initialization patterns used in other modules.
Integration verified - bondDenom parameter consistently used
The addition of
bondDenomparameter tomoveKeeper.Initializeis properly integrated across all test files. The parameter is consistently sourced from the same constant and aligns with the staking parameters configuration, ensuring uniform bond denomination handling across modules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the bondDenom is consistently used across staking initialization # and parameters # Check staking parameters configuration rg -A 5 "stakingParams.*BondDenoms.*bondDenom" # Check move keeper initialization with bondDenom rg -A 5 "moveKeeper.*Initialize.*bondDenom"Length of output: 7405
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
==========================================
- Coverage 40.83% 40.82% -0.02%
==========================================
Files 270 270
Lines 25779 25789 +10
==========================================
+ Hits 10528 10529 +1
- Misses 13626 13632 +6
- Partials 1625 1628 +3
|
* prepare upgrade initiation-2 * fetching code bytes with precompile readlib func * delete existing upgrade handler * dont delete if it is module * add cosmos move * add get&set checksums, genesis import&export module checksums * change checksum func * initialize staking at genesis (#290) * movevm loaderv2 integration (#284) * connect movevm with loaderv2 and introduce module & script cache * use cache context before query * disable unstable * update movevm to latest * add toml description --------- Co-authored-by: beer-1 <[email protected]> * wip * delete keys after iteration * clone key value * fix * reverse iterate vmstore & set keys * final touch * update swagger docs --------- Co-authored-by: beer-1 <[email protected]> Co-authored-by: beer-1 <[email protected]>
Description
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 changeReviewers 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...