fix init-from-state cmd #264
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Poem
Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/bitsongd/cmd/init_from_state.go (2)
Line range hint
169-175: Address TODO comment about genesis validation.The commented-out genesis validation code should be fixed and uncommented to ensure the generated genesis file is valid.
Would you like me to help implement the genesis validation logic or create a GitHub issue to track this task?
55-55: Add command usage example as code comment.The command usage example is currently in an unusual location. Consider moving it to the command's description or help text.
-// bitsongd init-from-state v021 export-v021.json v021 --old-moniker Cosmostation --old-account-addr bitsong1wf3q0a3uzechxvf27reuqts8nqm45sn29ykncv --increase-coin-amount 10000000000000000 -o + cmd.Example = ` + $ bitsongd init-from-state v021 export-v021.json v021 \ + --old-moniker Cosmostation \ + --old-account-addr bitsong1wf3q0a3uzechxvf27reuqts8nqm45sn29ykncv \ + --increase-coin-amount 10000000000000000 -o +`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/bitsongd/cmd/init_from_state.go(4 hunks)
🔇 Additional comments (1)
cmd/bitsongd/cmd/init_from_state.go (1)
117-117: Verify the impact of changing keyring backend from "test" to "os".The change from "test" to "os" backend improves security but requires user interaction for key operations. This might affect automation scripts or CI/CD pipelines that previously relied on the non-interactive "test" backend.
Run this script to check for potential automation dependencies:
✅ Verification successful
✓ Keyring backend change to "os" is appropriate
The change to use the "os" backend is safe as this command is designed for manual operations and isn't used in any automated processes or tests. The improved security from using the OS's credential store is beneficial for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for automation scripts that might be affected by the keyring backend change # Look for CI/CD files that might use this command rg -g '*.{yml,yaml}' -A 5 'init-from-state' # Look for shell scripts that might use this command rg -g '*.sh' -A 5 'init-from-state'Length of output: 79
Script:
#!/bin/bash # Search for any usage of init-from-state command rg -l "InitFromState" # Search for keyring initialization patterns rg -A 3 'keyring\.New\(' # Look for test files that might use keyring fd -e test.go --exec grep -l "keyring"Length of output: 501
| // Update LastValidatorPowers | ||
| stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{ | ||
| { | ||
| Address: operatorAddr, | ||
| Power: updatedValidators[0].Power, | ||
| }, | ||
| } | ||
| stakingGenState.LastTotalPower = stakingGenState.LastTotalPower.Add(sdk.NewInt(params.IncreaseCoinAmount / 1000000)) | ||
|
|
There was a problem hiding this comment.
Fix potential panic in LastValidatorPowers update.
The code assumes updatedValidators has exactly one element and directly accesses index 0, which could panic if the slice is empty.
Apply this safer implementation:
- stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{
- {
- Address: operatorAddr,
- Power: updatedValidators[0].Power,
- },
- }
+ if len(updatedValidators) == 0 {
+ return nil, fmt.Errorf("no validator found with moniker %s", params.OldMoniker)
+ }
+ stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{{
+ Address: operatorAddr,
+ Power: updatedValidators[0].Power,
+ }}📝 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.
| // Update LastValidatorPowers | |
| stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{ | |
| { | |
| Address: operatorAddr, | |
| Power: updatedValidators[0].Power, | |
| }, | |
| } | |
| stakingGenState.LastTotalPower = stakingGenState.LastTotalPower.Add(sdk.NewInt(params.IncreaseCoinAmount / 1000000)) | |
| if len(updatedValidators) == 0 { | |
| return nil, fmt.Errorf("no validator found with moniker %s", params.OldMoniker) | |
| } | |
| stakingGenState.LastValidatorPowers = []stakingtypes.LastValidatorPower{{ | |
| Address: operatorAddr, | |
| Power: updatedValidators[0].Power, | |
| }} |
| // Filter the validators to keep only the one matching params.OldMoniker | ||
| var updatedValidators []tmtypes.GenesisValidator | ||
| for i := range genDoc.Validators { | ||
| if genDoc.Validators[i].Name == params.OldMoniker { | ||
| oldValidator = genDoc.Validators[i] | ||
| // Update the matching validator's data | ||
| validator := &genDoc.Validators[i] | ||
|
|
||
| // Replace validator data | ||
| validator.PubKey = params.TmPubKey | ||
| validator.Address = params.TmPubKey.Address() | ||
| validator.Power = validator.Power + (params.IncreaseCoinAmount / 1000000) | ||
| validator.Power = validator.Power + (params.IncreaseCoinAmount) | ||
|
|
||
| // Add the updated validator to the new slice | ||
| updatedValidators = append(updatedValidators, *validator) | ||
| break // Exit loop after finding and updating the matching validator | ||
| } | ||
| } | ||
|
|
||
| // Replace the original validators slice with the updated slice | ||
| genDoc.Validators = updatedValidators |
There was a problem hiding this comment.
Improve validator update logic to handle edge cases.
The current implementation has several potential issues:
- It assumes only one validator matches the moniker, breaking after the first match.
- Using a single-element slice in
updatedValidatorscould be problematic if multiple validators share the same moniker. - The power calculation no longer divides by 1,000,000, which might affect compatibility.
Consider this safer implementation:
- var updatedValidators []tmtypes.GenesisValidator
+ updatedValidators := make([]tmtypes.GenesisValidator, 0)
for i := range genDoc.Validators {
if genDoc.Validators[i].Name == params.OldMoniker {
oldValidator = genDoc.Validators[i]
- // Update the matching validator's data
validator := &genDoc.Validators[i]
validator.PubKey = params.TmPubKey
validator.Address = params.TmPubKey.Address()
- validator.Power = validator.Power + (params.IncreaseCoinAmount)
+ validator.Power = validator.Power + (params.IncreaseCoinAmount / 1_000_000)
- // Add the updated validator to the new slice
updatedValidators = append(updatedValidators, *validator)
- break // Exit loop after finding and updating the matching validator
+ if len(updatedValidators) > 1 {
+ return nil, fmt.Errorf("multiple validators found with moniker %s", params.OldMoniker)
+ }
}
}📝 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.
| // Filter the validators to keep only the one matching params.OldMoniker | |
| var updatedValidators []tmtypes.GenesisValidator | |
| for i := range genDoc.Validators { | |
| if genDoc.Validators[i].Name == params.OldMoniker { | |
| oldValidator = genDoc.Validators[i] | |
| // Update the matching validator's data | |
| validator := &genDoc.Validators[i] | |
| // Replace validator data | |
| validator.PubKey = params.TmPubKey | |
| validator.Address = params.TmPubKey.Address() | |
| validator.Power = validator.Power + (params.IncreaseCoinAmount / 1000000) | |
| validator.Power = validator.Power + (params.IncreaseCoinAmount) | |
| // Add the updated validator to the new slice | |
| updatedValidators = append(updatedValidators, *validator) | |
| break // Exit loop after finding and updating the matching validator | |
| } | |
| } | |
| // Replace the original validators slice with the updated slice | |
| genDoc.Validators = updatedValidators | |
| // Filter the validators to keep only the one matching params.OldMoniker | |
| updatedValidators := make([]tmtypes.GenesisValidator, 0) | |
| for i := range genDoc.Validators { | |
| if genDoc.Validators[i].Name == params.OldMoniker { | |
| oldValidator = genDoc.Validators[i] | |
| validator := &genDoc.Validators[i] | |
| validator.PubKey = params.TmPubKey | |
| validator.Address = params.TmPubKey.Address() | |
| validator.Power = validator.Power + (params.IncreaseCoinAmount / 1_000_000) | |
| updatedValidators = append(updatedValidators, *validator) | |
| if len(updatedValidators) > 1 { | |
| return nil, fmt.Errorf("multiple validators found with moniker %s", params.OldMoniker) | |
| } | |
| } | |
| } | |
| // Replace the original validators slice with the updated slice | |
| genDoc.Validators = updatedValidators |
|
merged in #265 , will be replaced for new function from sdk to generate-testnet from state from state |
Summary by CodeRabbit
New Features
Bug Fixes
These changes improve the node initialization and validator management process with more precise handling of configuration and power settings.