fix: Sync with upstream #7
Conversation
WalkthroughThe changes introduce version v0.50.10 of the Cosmos SDK, released on September 20, 2024. Key updates include new CLI commands for querying module hashes and bulk adding genesis accounts, enhancements to event attributes and error messages, and several bug fixes across various modules. Additionally, the Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
client/v2/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
Files selected for processing (18)
- CHANGELOG.md (1 hunks)
- RELEASE_NOTES.md (2 hunks)
- client/v2/CHANGELOG.md (1 hunks)
- client/v2/autocli/query.go (1 hunks)
- client/v2/go.mod (1 hunks)
- go.mod (2 hunks)
- runtime/builder.go (1 hunks)
- simapp/app_config.go (1 hunks)
- simapp/app_di.go (5 hunks)
- simapp/go.mod (2 hunks)
- tests/go.mod (3 hunks)
- testutil/integration/router.go (2 hunks)
- types/collections.go (2 hunks)
- x/circuit/autocli.go (3 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/json_marshal.go (4 hunks)
- x/tx/signing/aminojson/json_marshal_test.go (1 hunks)
- x/tx/signing/aminojson/options.go (1 hunks)
Additional context used
LanguageTool
x/tx/CHANGELOG.md
[uncategorized] ~40-~40: You might be missing the article “the” here.
Context: ...712) AddAminoNameAsTypeURLoption to Amino JSON encoder. ## [v0.13.4](https://git...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
Additional comments not posted (37)
RELEASE_NOTES.md (3)
1-1: LGTM!The version number has been correctly updated to v0.50.10, which aligns with the PR objective of synchronizing with upstream changes.
11-13: Significant improvements!The changes in this segment bring notable enhancements:
The new
module-hash-by-heightcommand enables querying and retrieving module hashes at specific heights, which is a valuable addition for developers and users.Fixing the emission of
PreBlockevents, particularly for thex/upgrademodule, ensures correct behavior and aligns with expected functionality.Addressing the issue with runtime baseapp option ordering when other modules have options is a crucial fix that prevents potential problems.
These improvements contribute to a more robust and reliable SDK.
17-17: Accurate changelog reference!The changelog link has been properly updated to point to the v0.50.10 changelog. This ensures that users and developers have access to the correct and up-to-date information about the changes in this release.
runtime/builder.go (1)
32-41: LGTM!The changes in this code segment are well-structured and improve the initialization process of the
BaseAppinstance. By setting the message service and gRPC query routers as part of thebaseAppOptions, the code ensures that the routers are configured before theBaseAppis created. This is a cleaner approach compared to the previous implementation where the routers were set directly after theBaseAppinstantiation.The removal of the previous direct calls to set the routers is consistent with the new approach and improves code organization. The changes do not introduce any breaking changes or compatibility issues.
Overall, the code segment follows the existing coding style and conventions, and the changes enhance the readability and maintainability of the codebase.
x/circuit/autocli.go (3)
46-46: LGTM!The change to add leading slashes to the
limit_type_urlsJSON array elements in theauthorizecommand example is consistent with the modifications made to the other command examples. The change looks good.
56-56: LGTM!The change to add leading slashes to the message type URLs in the
disablecommand example is consistent with the modifications made to the other command examples. The change looks good.
65-65: LGTM!The change to add leading slashes to the message type URLs in the
resetcommand example is consistent with the modifications made to the other command examples. The change looks good.x/tx/CHANGELOG.md (1)
36-40: LGTM!The changelog entry for v0.13.5 is correctly formatted and accurately reflects the improvement made in #21712. The categorization under "Improvements" is appropriate, and the pull request reference is correctly linked.
Note: The static analysis hint suggesting to add the article "the" before "Amino JSON encoder" is a false positive and can be ignored, as the sentence reads correctly without it.
Tools
LanguageTool
[uncategorized] ~40-~40: You might be missing the article “the” here.
Context: ...712) AddAminoNameAsTypeURLoption to Amino JSON encoder. ## [v0.13.4](https://git...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
client/v2/CHANGELOG.md (1)
39-43: LGTM!The changelog entry for version
v2.0.0-beta.5is well-formatted and follows the guiding principles. The improvement is clearly described with enough context, and the pull request reference is correctly linked.x/tx/signing/aminojson/options.go (2)
35-36: LGTM!The changes to
getMessageAminoNameAnyimprove the consistency and usability of the function by always returning a string, either the amino name or the message URL, instead of a boolean indicating failure.
Line range hint
38-51: LGTM!The new
getMessageTypeURLfunction provides a consistent way to retrieve the message URL, using a series of fallbacks to handle various types of messages. The logic is sound and the fallbacks are reasonable.client/v2/autocli/query.go (1)
121-126: Verify the impact of theAminoNameAsTypeURLfield on message serialization.The addition of the
AminoNameAsTypeURLfield to theencoderOptionsstruct is a valid change that enhances the configuration options for the encoder. However, it's important to ensure that this change is intentional and aligns with the desired encoding behavior.To verify the impact of the
AminoNameAsTypeURLfield on message serialization, consider the following:
Review the documentation or specifications related to the
AminoNameAsTypeURLfield to understand its purpose and expected behavior.Analyze how the
AminoNameAsTypeURLfield is used within the encoding process. Trace its usage and identify any potential side effects or changes in the serialized output.Run the following script to search for relevant test cases that cover the serialization logic affected by the
AminoNameAsTypeURLfield:Execute the identified test cases and carefully review the test results to ensure that the serialization behavior aligns with the expected outcomes.
If necessary, consider adding new test cases to comprehensively validate the impact of the
AminoNameAsTypeURLfield on message serialization across different scenarios.By thoroughly verifying the impact of this change, you can ensure that the addition of the
AminoNameAsTypeURLfield does not introduce any unintended consequences and maintains the desired encoding behavior.Verification successful
These scripts will help us gather more comprehensive information about the
AminoNameAsTypeURLfield, its usage, and potential impact on serialization. Once we have this information, we can make a more informed decision about the verification process.
Verified:
AminoNameAsTypeURLfield impacts Amino JSON encoding as intendedThe
AminoNameAsTypeURLfield is correctly used in theBuildQueryMethodCommandfunction. Its purpose, as documented inx/tx/signing/aminojson/json_marshal.go, is to "use the amino name as the type URL in the JSON output" when set to true. This aligns with the change inquery.go.Impact on message serialization:
- When set to true, it modifies how type URLs are represented in the JSON output of Amino-encoded messages.
- This change affects the structure of the serialized JSON, potentially impacting how clients parse and interpret the output.
Recommendations:
- Review the existing test case
TestAminoNameAsTypeURLinjson_marshal_test.goto ensure it adequately covers this functionality in the context of query commands.- Add a comment in
client/v2/autocli/query.goexplaining whyAminoNameAsTypeURLis set to true for query commands, to improve code clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases related to message serialization using the `AminoNameAsTypeURL` field. # Test: Search for test functions that invoke the `BuildQueryMethodCommand` function. # Expect: Relevant test cases that cover the serialization logic. rg --type go -A 10 $'func Test.*BuildQueryMethodCommand'Length of output: 57
Script:
#!/bin/bash # Description: Search for usage and information about AminoNameAsTypeURL # Search for usage of AminoNameAsTypeURL across the codebase echo "Usage of AminoNameAsTypeURL:" rg "AminoNameAsTypeURL" --type go # Look for documentation or comments mentioning AminoNameAsTypeURL echo -e "\nDocumentation or comments for AminoNameAsTypeURL:" rg "AminoNameAsTypeURL" --type go -C 3 | rg "//|/\*|\*/" # Search for encoder options or serialization-related tests echo -e "\nEncoder options or serialization-related tests:" rg "func Test.*Encoder|func Test.*Serializ|func Test.*Marshal" --type go # Find the definition of the struct or interface containing AminoNameAsTypeURL echo -e "\nStruct or interface definition containing AminoNameAsTypeURL:" ast-grep --lang go --pattern $'(struct|interface) $_ { $$$ AminoNameAsTypeURL $_ $$$ }'Length of output: 4801
client/v2/go.mod (1)
10-10: Dependency version update looks good.Updating the
cosmossdk.io/x/txmodule tov0.13.5is a reasonable change to get the latest features and fixes.Please ensure that:
- The codebase is compatible with the new version of the module.
- All unit and integration tests are passing after updating the dependency.
You can run the following commands to verify:
types/collections.go (3)
35-37: LGTM!The variable declaration follows the established pattern and is consistent with the rest of the code.
45-48: LGTM!The constant declaration is valid and consistent with the usage of the
math.LegacyDectype.
176-210: LGTM!The
legacyDecValueCodectype correctly implements thecollcodec.ValueCodecinterface for themath.LegacyDectype. The implementation is consistent with the existing code patterns and follows the expected behavior for encoding, decoding, and stringifying the values.go.mod (2)
13-14: LGTM!The minor version updates for
cosmossdk.io/storeandcosmossdk.io/x/txmodules are consistent with semantic versioning and should be backwards compatible.
84-84: LGTM!The minor version update for the indirect dependency
github.com/cosmos/iavlis consistent with semantic versioning and should be backwards compatible.tests/go.mod (4)
13-13: LGTM!The minor version update to
cosmossdk.io/storefromv1.1.0tov1.1.1looks good.
17-17: LGTM!The patch version update to
cosmossdk.io/x/txfromv0.13.4tov0.13.5looks good.
39-39: Verify the compatibility and stability of the pre-release version update.The version of
cosmossdk.io/client/v2has been updated to a pre-release versionv2.0.0-beta.4.0.20240918122632-1879050ca719, which includes a specific commit hash.Please ensure that this pre-release version is compatible with the current project and has been thoroughly tested for stability and potential breaking changes.
68-68: LGTM!The minor version update to
github.com/cosmos/iavlfromv1.1.2tov1.2.0looks good.simapp/go.mod (4)
7-7: Ensure the specific commit has been properly validated.The version update to include a specific commit hash can help with reproducibility and stability. However, please ensure this commit (
1879050ca719) has been thoroughly tested and validated before merging.
13-13: Review the changelog for any notable changes.The minor version update to
cosmossdk.io/storesuggests there may be new features or bug fixes. Please review the changelog for this version and ensure there are no breaking changes or issues that could affect the current codebase.
19-19: Quickly scan the changelog for relevant fixes or changes.The patch version update to
cosmossdk.io/x/txlikely contains bug fixes or minor improvements. While patch updates are generally safe, it's still good to quickly review the changelog for any changes that are particularly relevant to the codebase.
68-68: Carefully review the changelog and thoroughly test the new version.The update to
github.com/cosmos/iavlfromv1.1.2tov1.2.0is a significant version bump that likely includes substantial changes or new features. Please do the following:
Carefully review the entire changelog for this version, noting any breaking changes or updates that could impact our codebase.
Thoroughly test the integration of this new version to ensure compatibility and catch any issues early.
Consider noting any major changes from the changelog in the PR description.
x/tx/signing/aminojson/json_marshal_test.go (1)
359-416: LGTM!The
TestAminoNameAsTypeURLfunction provides good test coverage for theAminoNameAsTypeURLoption. It verifies that the marshaled output includes the type URL and the expected fields, with default values omitted. The test also checks for any errors during the marshaling process.Adding this test helps ensure the correctness of the marshaling behavior when the
AminoNameAsTypeURLoption is enabled.x/tx/signing/aminojson/json_marshal.go (4)
35-38: LGTM!The new
AminoNameAsTypeURLfield in theEncoderOptionsstruct is a valid addition. It provides more flexibility to the encoder by allowing it to use the type URL instead of the amino name in the JSON output. The field is well-documented with clear comments explaining its purpose and usage.
57-57: LGTM!The new
aminoNameAsTypeURLfield in theEncoderstruct is a valid addition. It corresponds to theAminoNameAsTypeURLfield in theEncoderOptionsstruct and is used to store the value of theAminoNameAsTypeURLoption in theEncoderinstance.
88-93: LGTM!The changes to the
NewEncoderfunction are valid. TheaminoNameAsTypeURLfield in theEncoderstruct is correctly initialized with the value from theAminoNameAsTypeURLfield in theEncoderOptionsstruct. This ensures that theEncoderinstance is properly configured with the provided options.
196-206: LGTM!The changes to the
beginMarshalmethod are valid. The method now conditionally uses the type URL instead of the amino name when marshaling messages, based on theaminoNameAsTypeURLflag. The logic correctly handles the conditional usage of the type URL for both cases where the message is of typeAnyand when it is not. The changes ensure that the encoder uses the type URL when specified, while still maintaining the correct behavior for thenamedvariable. The changes are well-structured and easy to understand.CHANGELOG.md (2)
41-42: LGTM!The new version and release date look good. The format is consistent with the existing changelog entries.
47-53: Looks great!The changelog entries are well categorized and the descriptions are clear and concise. Referencing the PR numbers is very helpful.
simapp/app_di.go (4)
32-32: Import added for custom ante handler implementationThe addition of
github.com/cosmos/cosmos-sdk/x/auth/anteis necessary for implementing the custom ante handler in thesetAnteHandlermethod.
85-85: Renaming toCircuitKeeperimproves clarityRenaming
CircuitBreakerKeepertoCircuitKeeperin theSimAppstruct enhances readability and aligns with naming conventions.
186-186: InjectingCircuitKeeperinto dependency injectionIncluding
&app.CircuitKeeperin the dependency injection ensures that theCircuitKeeperis properly initialized and available throughout the application.
252-254: Setting custom ante handler during initializationCalling
app.setAnteHandler(app.txConfig)withinNewSimAppcorrectly initializes the custom ante handler during application setup.
| Name: "tx", | ||
| Config: appconfig.WrapAny(&txconfigv1.Config{ | ||
| SkipAnteHandler: true, // Enable this to skip the default antehandlers and set custom ante handlers. | ||
| }), |
There was a problem hiding this comment.
Skipping default ante handlers and using custom ones
The change allows the application to skip the default ante handlers and set custom ones by configuring SkipAnteHandler: true in the "tx" module. This provides flexibility to implement custom transaction processing logic.
However, it's crucial to ensure that the custom ante handlers perform the necessary checks and validations to maintain the integrity and security of transactions. Please verify that the custom ante handlers are correctly implemented and tested.
Also, consider the impact of this change on other parts of the application that may rely on the default ante handler behavior. Ensure that the custom ante handlers are properly integrated with the rest of the application.
Lastly, document the custom ante handler logic thoroughly, including the checks performed and any assumptions made.
| // setAnteHandler sets custom ante handlers. | ||
| // "x/auth/tx" pre-defined ante handler have been disabled in app_config. | ||
| func (app *SimApp) setAnteHandler(txConfig client.TxConfig) { | ||
| anteHandler, err := NewAnteHandler( | ||
| HandlerOptions{ | ||
| ante.HandlerOptions{ | ||
| AccountKeeper: app.AccountKeeper, | ||
| BankKeeper: app.BankKeeper, | ||
| SignModeHandler: txConfig.SignModeHandler(), | ||
| FeegrantKeeper: app.FeeGrantKeeper, | ||
| SigGasConsumer: ante.DefaultSigVerificationGasConsumer, | ||
| }, | ||
| &app.CircuitKeeper, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Set the AnteHandler for the app | ||
| app.SetAnteHandler(anteHandler) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure HandlerOptions struct includes CircuitKeeper
In the setAnteHandler method, the HandlerOptions struct is used to pass options to NewAnteHandler. Ensure that HandlerOptions is properly defined to include both ante.HandlerOptions and the CircuitKeeper.
If HandlerOptions is not already defined, you may need to define it as follows:
type HandlerOptions struct {
ante.HandlerOptions
CircuitKeeper *circuitkeeper.Keeper
}This ensures that NewAnteHandler receives all necessary options for the custom ante handler.
| @@ -56,7 +57,7 @@ func NewIntegrationApp( | |||
| basicModuleManager.RegisterInterfaces(interfaceRegistry) | |||
|
|
|||
| txConfig := authtx.NewTxConfig(codec.NewProtoCodec(interfaceRegistry), authtx.DefaultSignModes) | |||
| bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), baseapp.SetChainID(appName)) | |||
| bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), append(baseAppOptions, baseapp.SetChainID(appName))...) | |||
There was a problem hiding this comment.
Order of Options May Override User-Provided Chain ID
In the call to NewBaseApp, you're appending baseAppOptions before baseapp.SetChainID(appName). This means that baseapp.SetChainID(appName) will override any SetChainID options provided in baseAppOptions. If you want to allow baseAppOptions to override the default chain ID, consider placing baseapp.SetChainID(appName) before baseAppOptions.
Apply this diff to adjust the order of options:
-bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), append(baseAppOptions, baseapp.SetChainID(appName))...)
+bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), append([]func(*baseapp.BaseApp){baseapp.SetChainID(appName)}, baseAppOptions...)...)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.
| bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), append(baseAppOptions, baseapp.SetChainID(appName))...) | |
| bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), append([]func(*baseapp.BaseApp){baseapp.SetChainID(appName)}, baseAppOptions...)...) |
| @@ -47,6 +47,7 @@ | |||
| keys map[string]*storetypes.KVStoreKey, | |||
| appCodec codec.Codec, | |||
| modules map[string]appmodule.AppModule, | |||
| baseAppOptions ...func(*baseapp.BaseApp), | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Based on the analysis of the NewIntegrationApp function calls across various test files, we can conclude that there is an issue that needs to be addressed.
Update Required: NewIntegrationApp Function Calls
The NewIntegrationApp function has been updated to include a new parameter baseAppOptions ...func(*baseapp.BaseApp), but this change has not been reflected in the existing function calls throughout the test files. This inconsistency needs to be addressed to ensure proper functionality and prevent potential compilation errors. The following files contain calls to NewIntegrationApp that need to be updated:
- testutil/integration/example_test.go
- tests/integration/slashing/keeper/keeper_test.go
- tests/integration/staking/keeper/determinstic_test.go
- tests/integration/staking/keeper/common_test.go
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/gov/keeper/keeper_test.go
- tests/integration/evidence/keeper/infraction_test.go
- tests/integration/distribution/keeper/msg_server_test.go
Please update all calls to NewIntegrationApp in these files to include the new baseAppOptions parameter. If no specific options are needed, you can pass an empty variadic parameter (e.g., NewIntegrationApp(..., nil)).
Analysis chain
Potential Breaking Change: Update Function Calls to Match New Signature
The function NewIntegrationApp now includes a variadic parameter baseAppOptions ...func(*baseapp.BaseApp) in its signature. Since this function is exported, any existing code that calls NewIntegrationApp without the new parameter will fail. Please ensure that all calls to this function have been updated to match the new signature.
Run the following script to identify calls to NewIntegrationApp that may need to be updated:
To gather more comprehensive information about the NewIntegrationApp function calls, we need to modify our search approach. Let's use a script that provides more context for each instance.
This script will show us 10 lines of context before and after each NewIntegrationApp call, which should be sufficient to see the entire function call in most cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of NewIntegrationApp to verify they match the new signature.
# Test: Search for calls to NewIntegrationApp. Expect: All calls include the new parameter.
rg --type go 'NewIntegrationApp\(' --glob='!testutil/integration/router.go' -A 2
Length of output: 2833
Script:
#!/bin/bash
# Description: Find all usages of NewIntegrationApp with extended context to verify they match the new signature.
# Test: Search for calls to NewIntegrationApp with 10 lines of context before and after. Expect: Full function calls visible.
rg --type go 'NewIntegrationApp\(' --glob='!testutil/integration/router.go' -C 10
Length of output: 17601
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...
!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
New Features
module-hash-by-heightfor querying module hashes andbulk-add-genesis-accountfor adding multiple genesis accounts.LegacyDeccollection value for enhanced data handling.Improvements
MsgMultiSendevent with aSenderattribute.NewIntegrationAppto accept additional baseapp options.Bug Fixes
GetSignersfunction.Documentation