fix(x/auth): facultative vesting as well in simulation #22721
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
📝 WalkthroughWalkthroughThe changes introduced in this pull request modify the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
|
@julienrbrt your pull request is missing a changelog! |
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/auth/simulation/genesis.go (2)
29-35: Consider adding a comment explaining the check order.The placement of the vesting module check before the validator bonding check is logical, but it would be helpful to document why this order is important.
+// First check if vesting module is enabled before proceeding with any vesting-related logic +// This ensures we don't attempt to create vesting accounts when the module is not available // check if vesting module is enabled // if not, just use base account if _, ok := simState.GenState["vesting"]; !ok {
29-34: Consider documenting simulation behavior changes.This change affects the distribution of account types in simulations. Consider:
- Adding a comment in the function documentation explaining the conditional vesting account creation
- Updating simulation documentation to reflect this behavior
- Adding test cases that verify simulation behavior both with and without the vesting module
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/auth/simulation/genesis.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/simulation/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
x/auth/simulation/genesis.go (1)
29-34: LGTM! Clean implementation of facultative vesting check.
The implementation correctly handles the case where the vesting module is not present by falling back to base accounts, which aligns with the PR objective of preventing panics in simulation.
Let's verify that this is the only place where vesting accounts are created during simulation:
✅ Verification successful
Verified: The vesting check implementation is correctly placed
After reviewing the complete file content, I can confirm that this is indeed the only place where vesting accounts are created during simulation. The RandomGenesisAccounts function is the designated location for creating different types of accounts during genesis simulation, and the new check properly gates the creation of vesting accounts based on module availability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential vesting account creation points in simulation
rg -g '**/*simulation*.go' "vestingtypes\.New(Continuous|Delayed|Period)VestingAccount"
Length of output: 89
Script:
#!/bin/bash
# Let's try a broader search for vesting-related code in simulation files
rg -g '**/*simulation*.go' -i "vesting" -A 3 -B 3
Length of output: 49
Script:
#!/bin/bash
# Let's search for the file content to understand the context better
cat x/auth/simulation/genesis.go
Length of output: 4647
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/auth/simulation/genesis.go (1)
29-34: Enhance module presence check with constants and documentationThe conditional check for the vesting module is correct and addresses the PR objective. However, consider these improvements:
- Define the module name as a constant to avoid string literals
- Add a more detailed comment explaining why this check prevents simulation panics
+// ModuleName for vesting +const VestingModuleName = "vesting" // check if vesting module is enabled -// if not, just use base account +// Skip vesting account creation if the vesting module is not enabled in simulation +// to prevent panics when simulating chains that don't use vesting -if _, ok := simState.GenState["vesting"]; !ok { +if _, ok := simState.GenState[VestingModuleName]; !ok {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/auth/simulation/genesis.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/simulation/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
x/auth/simulation/genesis.go (2)
31-34: LGTM: Clean integration with existing vesting logic
The placement and integration of the vesting module check is well-structured:
- Early exit prevents unnecessary processing
- Maintains existing functionality when vesting is enabled
- Preserves simulation behavior for supported configurations
Also applies to: 35-77
29-34: Verify the fix prevents simulation panics
Let's confirm this resolves the simulation panic issue by checking for similar patterns or related code.
✅ Verification successful
Fix is properly implemented and prevents simulation panics
The code changes effectively prevent simulation panics by:
- Correctly checking for vesting module availability before attempting to create vesting accounts
- Maintaining proper account initialization when vesting is disabled
- Preserving the existing simulation logic for cases where vesting is enabled
The implementation is verified through:
- The presence of proper error handling in the
RandomGenesisAccountsfunction - Consistent usage across simulation tests
- No other locations in the codebase requiring similar vesting-related simulation fixes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential vesting-related simulation code that might need similar checks
rg -g '*.go' -l 'simState.*GenState.*vesting' --type go
# Look for any other direct vesting module references in simulation code
rg -g '*.go' -l 'vestingtypes.*simulation' --type go
# Check for any related test files that might need updates
fd -g '*_test.go' -x grep -l 'RandomGenesisAccounts'
Length of output: 688
Script:
#!/bin/bash
# Let's check the implementation of RandomGenesisAccounts in genesis.go and its test
cat x/auth/simulation/genesis.go x/auth/simulation/genesis_test.go
# Check if there are any simulation-related test failures or panics being handled
rg -g '*.go' -A 5 'simulation.*panic' --type go
# Look for any vesting-related simulation tests
rg -g '*_test.go' -A 5 'vesting.*simulation' --type go
Length of output: 8470
(cherry picked from commit 4d1adcf)
…) (#22726) Co-authored-by: Julien Robert <[email protected]>
Description
Noticed when migrating a chain to use
simsx(ignite/cli#4432).Now that the vesting module is not so mandatory, we don't recommend adding it to new chains and use the lockup account.
This fixes the fact that it would still create vesting account in simulation while no vesting module was on the chain. Causing sims to panic.
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.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit