chore: bring our cosmos-sdk up to date with upstream #195
Conversation
WalkthroughThe changes encompass multiple updates across the Cosmos SDK, including the introduction of a Linux-only backend for the keyring, enhancements to CLI commands, bug fixes in simulation tests, and improvements in governance proposal handling. Documentation has also been updated to reflect these changes, alongside modifications in error messaging and command handling in the client modules. Overall, the updates aim to enhance functionality and ensure smoother operation within the SDK. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
client/v2/autocli/flag/enum.go (1)
61-66: Approve changes with suggestions for further improvementThe enhancement to the error message is a great improvement, providing users with more informative feedback. This change aligns well with best practices for error handling and will significantly improve the user experience.
However, there are a couple of potential optimizations to consider:
Pre-compute the list of valid values during initialization to avoid recreating it on each error. This would be more efficient, especially if errors occur frequently.
Sort the valid values for consistent output. This makes the error message more predictable and easier to read.
Here's a suggested implementation incorporating these improvements:
type enumValue struct { enum protoreflect.EnumDescriptor value protoreflect.EnumNumber valMap map[string]protoreflect.EnumValueDescriptor validValues []string // New field } // In the NewValue function or wherever enumValue is initialized func (b enumType) NewValue(*context.Context, *Builder) Value { val := &enumValue{ enum: b.enum, valMap: map[string]protoreflect.EnumValueDescriptor{}, } n := b.enum.Values().Len() for i := 0; i < n; i++ { valDesc := b.enum.Values().Get(i) name := enumValueName(b.enum, valDesc) val.valMap[name] = valDesc val.validValues = append(val.validValues, name) } sort.Strings(val.validValues) // Sort for consistent output return val } // In the Set method func (e *enumValue) Set(s string) error { valDesc, ok := e.valMap[s] if !ok { return fmt.Errorf("%s is not a valid value for enum %s. Valid values are: %s", s, e.enum.FullName(), strings.Join(e.validValues, ", ")) } e.value = valDesc.Number() return nil }This approach maintains the improved error message while optimizing for performance and consistency.
client/v2/autocli/msg.go (1)
52-54: Approved with suggestions for improvementThe addition of the
EnhanceCustomCommandcheck improves the flexibility of sub-command handling. However, I have a few suggestions to enhance clarity and maintainability:
Consider adding a comment explaining the purpose of this condition, e.g.:
// Skip adding the sub-command if EnhanceCustomCommand is true, // allowing for custom handling of this command elsewhereIt might be helpful to log when a sub-command is skipped, especially for debugging purposes:
if !subCmdDescriptor.EnhanceCustomCommand { cmd.AddCommand(subCmd) } else { // Consider adding a debug log here // log.Debug(fmt.Sprintf("Skipped adding sub-command %s due to EnhanceCustomCommand flag", cmdName)) }Could you provide more context on how
EnhanceCustomCommandis set and where it comes from? This information would be valuable for understanding the full impact of this change.docs/docs/build/building-apps/05-app-testnet.md (3)
Line range hint
19-98: LGTM: Staking setup for testnet looks good.The changes in the Staking section correctly set up a new validator for the testnet and remove existing validators. This simplification of the validator set is appropriate for testnet purposes.
Consider wrapping the panic calls with a custom error type to provide more context. For example:
if err != nil { panic(fmt.Errorf("failed to set validator by consensus address: %w", err)) }This will make debugging easier if an error occurs during testnet setup.
Line range hint
110-198: LGTM: Slashing, Bank, and Upgrade sections set up correctly.The Slashing, Bank, and Upgrade sections are implemented correctly for setting up a testnet environment. Each section follows Cosmos SDK conventions and uses appropriate keeper methods.
In the Bank section, consider making the account creation more flexible:
- Instead of hardcoding the accounts, consider accepting a list of accounts as a parameter or reading from a configuration file.
- Allow customization of the default coin amount, possibly as a parameter to the function.
This would make the testnet setup more adaptable to different testing scenarios. For example:
func createTestAccounts(ctx sdk.Context, app *SimApp, accounts []string, defaultAmount int64) error { defaultCoins := sdk.NewCoins(sdk.NewInt64Coin("ustake", defaultAmount)) for _, account := range accounts { addr, err := sdk.AccAddressFromBech32(account) if err != nil { return err } err = app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, defaultCoins) if err != nil { return err } err = app.BankKeeper.SendCoinsFromModuleToAccount(ctx, minttypes.ModuleName, addr, defaultCoins) if err != nil { return err } } return nil }This function could then be called with a list of accounts and a default amount, allowing for more flexible testnet configurations.
Line range hint
198-233: LGTM: Testnet setup and running implemented correctly.The changes in the Running the Testnet section correctly implement the necessary modifications to create and run a testnet. The new command and helper function are well-integrated into the existing codebase.
While using
panicfor error handling in a testnet setup function is acceptable, consider using a more graceful error handling approach:
- Modify the
newTestnetAppfunction to return an error:func newTestnetApp(logger log.Logger, db cometbftdb.DB, traceStore io.Writer, appOpts servertypes.AppOptions) (servertypes.Application, error) { // ... existing code ... newValAddr, ok := appOpts.Get(server.KeyNewValAddr).(bytes.HexBytes) if !ok { return nil, fmt.Errorf("newValAddr is not of type bytes.HexBytes") } // ... similar changes for other type assertions ... return simapp.InitSimAppForTestnet(simApp, newValAddr, newValPubKey, newOperatorAddress, upgradeToTrigger) }
- Update the
AddTestnetCreatorCommandcall to handle the error:server.AddTestnetCreatorCommand(rootCmd, simapp.DefaultNodeHome, func(logger log.Logger, db cometbftdb.DB, traceStore io.Writer, appOpts servertypes.AppOptions) servertypes.Application { app, err := newTestnetApp(logger, db, traceStore, appOpts) if err != nil { fmt.Fprintf(os.Stderr, "Failed to create testnet app: %v\n", err) os.Exit(1) } return app }, addModuleInitFlags)This approach provides more informative error messages and allows for better error handling in the calling code.
x/simulation/simulate.go (2)
108-111: LGTM! Consider adding a debug log.The addition of this check for an empty validator set is a good safeguard. It prevents the simulation from running in an invalid state where there are no validators.
Consider adding a debug log before skipping to provide more context in the logs:
if len(nextValidators) == 0 { + fmt.Fprintf(w, "Debug: Skipping simulation due to empty validator set in genesis\n") tb.Skip("skipping: empty validator set in genesis") return true, params, nil }
250-253: LGTM! Consider consistent messaging.This check for an empty validator set during the simulation is a good addition. It ensures that the simulation doesn't continue if all validators have been removed.
For consistency with the earlier check, consider updating the skip message:
if len(nextValidators) == 0 { - tb.Skip("skipping: empty validator set") + tb.Skip("skipping: empty validator set after validator updates") return true, params, nil }Also, consider adding a debug log here as well, similar to the suggestion for the earlier check.
server/start.go (2)
753-762: Improved handling of addrbook.json for testnet creationThe added code segment improves the testnet creation process by regenerating the
addrbook.jsonfile. This is a good practice as it prevents potential issues with peers from the old network. Here's a breakdown of the changes:
- The existing
addrbook.jsonis removed.- A new empty
addrbook.jsonis created.This approach ensures a clean slate for the testnet's peer discovery mechanism.
Consider adding error logging for the file removal operation:
if err := os.Remove(addrBookPath); err != nil && !os.IsNotExist(err) { + ctx.Logger.Error("failed to remove existing addrbook.json", "error", err) return nil, fmt.Errorf("failed to remove existing addrbook.json: %w", err) }This will provide more detailed information in case of unexpected errors during the removal process.
753-762: Consider additional testnet isolation measuresThe addition of the
addrbook.jsonregeneration is a valuable improvement to the testnet creation process. It helps ensure that the new testnet is properly isolated from the previous network configuration.To further enhance the testnet isolation and configuration, consider the following suggestions:
- Reset or clear any persistent peers configuration that might be present in the node's config.toml file.
- Generate and set a new node ID to ensure uniqueness in the testnet environment.
- Clear or reset any stored peer information in the node's database, if applicable.
Would you like assistance in implementing these additional isolation measures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- client/v2/CHANGELOG.md (1 hunks)
- client/v2/autocli/flag/enum.go (1 hunks)
- client/v2/autocli/msg.go (1 hunks)
- client/v2/autocli/query.go (1 hunks)
- docs/docs/build/building-apps/05-app-testnet.md (5 hunks)
- server/start.go (2 hunks)
- x/auth/autocli.go (1 hunks)
- x/simulation/simulate.go (2 hunks)
🔇 Additional comments (10)
client/v2/CHANGELOG.md (2)
39-45: LGTM: New entries in the "Unreleased" section are well-formatted and informative.The new entries in the "Unreleased" section follow the correct changelog format and provide clear, concise descriptions of the changes. The inclusion of GitHub issue numbers is good for traceability.
Line range hint
1-46: Excellent maintenance of the CHANGELOG.md file.The changelog is well-structured and follows best practices:
- Changes are grouped by type (Improvements, Bug Fixes, etc.).
- Each entry includes a GitHub issue reference for traceability.
- Versions are clearly marked and dated.
- The new entries are consistent with the existing format.
This changelog provides valuable information for users and developers about the evolution of the client/v2 module.
client/v2/autocli/query.go (1)
55-57: Approve with suggestions: Enhanced subcommand controlThe addition of the
EnhanceCustomCommandcheck provides more granular control over subcommand addition, which is a positive enhancement. However, there are a few points to consider:
- The current implementation might lead to command omission if
EnhanceCustomCommandis true. Is this intentional?- Consider adding a comment explaining the purpose and implications of this condition for future maintainers.
To ensure this change doesn't unintentionally omit commands, please run the following verification:
If the second test doesn't yield results, consider whether an else clause is needed to handle cases where
EnhanceCustomCommandis true.docs/docs/build/building-apps/05-app-testnet.md (1)
Line range hint
100-108: LGTM: Distribution records initialized correctly.The Distribution section properly initializes the necessary records for the new validator. This includes setting up historical rewards, current rewards, accumulated commission, and outstanding rewards. The code follows Cosmos SDK conventions and uses the appropriate keeper methods.
x/simulation/simulate.go (1)
Line range hint
1-453: Overall assessment: Improved simulation robustnessThe changes in this file enhance the simulation's ability to handle edge cases, specifically when the validator set becomes empty. This is a valuable addition that prevents the simulation from running or continuing in an invalid state.
Key improvements:
- Check for empty validator set at the start of the simulation.
- Check for empty validator set after each validator update during the simulation.
These checks ensure that the simulation will not proceed without a valid set of validators, which is crucial for maintaining the integrity of the simulated blockchain environment.
server/start.go (1)
Line range hint
1-1000: Overall assessment: Beneficial improvement to testnet creationThe changes made to the
testnetifyfunction inserver/start.gorepresent a positive improvement to the testnet creation process. By regenerating theaddrbook.jsonfile, the code ensures better isolation between the main network and the newly created testnet.Key points:
- The modification addresses a specific issue without introducing unnecessary complexity.
- The changes are well-integrated into the existing function structure.
- The approach taken (removing and recreating the file) is straightforward and effective.
While the current changes are approved, consider implementing the additional isolation measures suggested earlier to further enhance the testnet creation process.
CHANGELOG.md (4)
Line range hint
1-24: Comprehensive changelog for a major release with significant changesThis changelog for Cosmos SDK v0.50.x indicates a major release with extensive changes across various modules and components. Some notable changes include:
- The introduction of a new Linux-only backend for the keyring that supports the Linux kernel's
keyctl.- Significant updates to the server module, including regeneration of
addrbook.jsonfor in-place testnets.- Multiple enhancements to CLI commands and queries.
- Important bug fixes in simulation tests and various modules.
Developers should pay close attention to the breaking changes sections, as they may require updates to existing applications built on the Cosmos SDK.
Line range hint
26-205: Significant API and client breaking changes require careful migrationThis section outlines numerous API breaking changes that will require substantial updates for developers:
- Many modules (e.g., x/upgrade, x/feegrant, x/nft) have been extracted into separate go.mod files, indicating a shift towards a more modular architecture.
- Significant changes to method signatures, especially in keeper methods, now often taking
context.Contextinstead ofsdk.Contextand returning errors.- Removal of deprecated functions and types, such as
StdTxtransaction and signing APIs.- Updates to interfaces like
sdk.MsgandTxConfig.For client-side changes:
- Modification of event handling in ABCI responses, including the addition of
msg_indexto all event attributes.- Changes in gRPC-Web configuration, now using the same port for gRPC-Web and the API server.
Developers should carefully review these changes and plan for a comprehensive migration of their codebase when upgrading to this version.
Line range hint
207-270: CLI restructuring and critical bug fixesCLI Breaking Changes:
- Migration to AutoCLI has led to changes in command output format and pagination flag names.
- Some commands have been split or renamed for clarity, e.g., in the bank module.
- Removal of certain commands, such as
<appd> q accountin favor of<appd> q auth account.These changes aim to improve consistency and usability but will require updates to scripts and documentation using the CLI.
Notable Bug Fixes:
- Addressing potential security issues, such as panic prevention in x/distribution and x/auth/vesting modules.
- Fixing consensus-related issues in baseapp, including vote extension handling and transaction processing.
- Correcting behavior in various modules like x/bank, x/gov, and x/slashing.
These fixes enhance the overall stability and security of the SDK. Developers should review these changes to understand how they might affect existing applications and consider testing thoroughly after upgrading.
Line range hint
272-323: Continued bug fixes and important deprecationsAdditional Bug Fixes:
- Corrections in x/staking module, including proper handling of unbonding entries.
- Improvements in types and crypto modules, such as better coin validation and secure key file permissions.
- Fixes in baseapp to ensure correct block height context and proper handling of malformed transactions.
These continued fixes further enhance the reliability and correctness of the SDK across various modules.
Deprecations:
IntProtoandDecProtoare deprecated in favor ofmath.Intandmath.LegacyDec.- The
delegator_addressfield ofMsgCreateValidatorin x/staking is deprecated.Developers should take note of these deprecations and plan to update their code in future releases to avoid using deprecated features.
Overall, this changelog represents a significant update to the Cosmos SDK with wide-ranging changes. Developers are advised to carefully review all sections and plan for a comprehensive update process when migrating to this version.
| for k := range e.valMap { | ||
| validValues = append(validValues, k) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
992121f
into
MANTRA-Chain:release/v0.50.x
Description
This PR brings our sdk v0.50.x branch up to date with upstream.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates enhance functionality, improve user experience, and ensure smoother operations across the Cosmos SDK.