Upgrade to Cosmos SDK 0.47 #560
Conversation
WalkthroughThis update involves upgrading the Go version across several workflows, extensive changes in import statements reflecting a shift from Tendermint to CometBFT libraries, and modifications in the handling of modules and configurations within the application. The changes aim to enhance performance, improve compatibility with updated dependencies, and streamline the codebase for better maintainability and functionality. Changes
This table groups similar changes together, providing a concise overview of the modifications spread across the project. Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (23)
Files selected for processing (48)
Files not reviewed due to errors (7)
Files skipped from review due to trivial changes (11)
Additional Context UsedShellCheck (13)
Additional comments not posted (74)
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 8
Out of diff range and nitpick comments (2)
integration_tests/setup_test.go (1)
415-415: Binding gRPC server to all interfaces (0.0.0.0:9090). If this configuration is intended for testing environments only, consider adding a comment to clarify this to avoid potential security misconfigurations.orchestrator/gravity_proto/src/prost/gravity.v1.rs (1)
582-584: Consider adding documentation for the newly introduced fieldsethereum_event_vote_windowandconfirmed_outgoing_tx_windowin theParamsstruct to maintain code clarity and help other developers understand their purpose quickly.
| @@ -77,7 +77,7 @@ func TestSignerSetTxCreationUponUnbonding(t *testing.T) { | |||
| ) | |||
| require.NoError(t, err) | |||
|
|
|||
| staking.EndBlocker(input.Context, input.StakingKeeper) | |||
| staking.EndBlocker(input.Context, &input.StakingKeeper) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The output from the latest script provides the definition of the EndBlocker function within the module/x/gravity/abci.go file. The function signature is func EndBlocker(ctx sdk.Context, k keeper.Keeper), and it includes calls to various internal functions such as outgoingTxSlashing, eventVoteRecordTally, and updateObservedEthereumHeight. However, there is no explicit mention or handling of unbonding validators within the provided lines of the function definition.
Given the absence of specific handling for unbonding validators in the EndBlocker function as shown in the script output, and considering that the function signature does not explicitly include parameters or logic related to SDK 0.47 changes concerning unbonding validators, it appears that the staking.EndBlocker function may not be fully aligned with the new SDK changes as expected.
Conclusion:
- The
EndBlockerfunction is defined in the codebase, but there is no evidence from the provided output that it has been updated to handle specifics of unbonding validators in the context of SDK 0.47. - Further review or updates may be necessary to ensure that the function aligns with the latest SDK changes, particularly concerning the handling of unbonding validators.
Analysis chain
Confirm that the staking.EndBlocker function is correctly handling the new SDK changes, especially in the context of unbonding validators.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `staking.EndBlocker` is updated for SDK 0.47 in the context of unbonding validators.
# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type go $'staking.EndBlocker'
Length of output: 390
Script:
#!/bin/bash
# Description: Check the definition of `staking.EndBlocker` to ensure it matches the expected changes in SDK 0.47.
# Search for the definition of `staking.EndBlocker` in Go files and display the function definition.
rg --type go -A 10 "func EndBlocker" | grep -A 10 "staking.EndBlocker"
Length of output: 70
Script:
#!/bin/bash
# Description: Broadly search for the definition of `EndBlocker` functions to ensure the correct implementation in the context of SDK 0.47.
# Search for the definition of any `EndBlocker` function in Go files to ensure we capture all relevant definitions.
rg --type go -A 10 "func EndBlocker"
Length of output: 711
|
|
||
| # move proto files to the right places | ||
| cp -r github.com/peggyjv/gravity-bridge/module/* ./ | ||
| xpath=$(head -n 1 go.mod | sed 's/^module //') | ||
| cp -r $xpath/* ./ |
There was a problem hiding this comment.
Double quote the variable $xpath to prevent globbing and word splitting.
- cp -r $xpath/* ./
+ cp -r "$xpath"/* ./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.
| cp -r $xpath/* ./ | |
| cp -r "$xpath"/* ./ |
| gravity init --chain-id=testing local | ||
|
|
||
| # Create delegate keys | ||
| CONFIG=$(echo "$(pwd)/gorc_config.toml") |
There was a problem hiding this comment.
Instead of using echo to pass a value to a command, directly use the value.
- CONFIG=$(echo "$(pwd)/gorc_config.toml")
+ CONFIG="$(pwd)/gorc_config.toml"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.
| CONFIG=$(echo "$(pwd)/gorc_config.toml") | |
| CONFIG="$(pwd)/gorc_config.toml" |
| gorc keys eth add signer | ||
| gorc keys cosmos add orchestrator | ||
|
|
||
| SIGNER=$(gorc --config $CONFIG keys eth show signer) |
There was a problem hiding this comment.
Double quote the variable $CONFIG to prevent globbing and word splitting.
- SIGNER=$(gorc --config $CONFIG keys eth show signer)
+ SIGNER=$(gorc --config "$CONFIG" keys eth show signer)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.
| SIGNER=$(gorc --config $CONFIG keys eth show signer) | |
| SIGNER=$(gorc --config "$CONFIG" keys eth show signer) |
| SIGNER=$(gorc --config $CONFIG keys eth show signer) | ||
| ORCHESTRATOR=$(gorc --config $CONFIG keys cosmos show orchestrator) | ||
| # remove prefix from output | ||
| ORCHESTRATOR=$(echo $ORCHESTRATOR | sed s/"orchestrator "//) |
There was a problem hiding this comment.
Use parameter expansion for string manipulation to improve readability and performance.
- ORCHESTRATOR=$(echo $ORCHESTRATOR | sed s/"orchestrator "//)
+ ORCHESTRATOR=${ORCHESTRATOR/"orchestrator "/}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.
| ORCHESTRATOR=$(echo $ORCHESTRATOR | sed s/"orchestrator "//) | |
| ORCHESTRATOR=${ORCHESTRATOR/"orchestrator "/} |
|
|
||
|
|
||
| e2e_validator_out: e2e_clean_slate | ||
| integration_tests/integration-tests.test -test.failfast -test.v -test.run IntegrationTestSuite -testify.m TestValidatorOut || make -s fail | ||
| integration_tests/integration_tests.test -test.failfast -test.v -test.run IntegrationTestSuite -testify.m TestValidatorOut || make -s fail |
There was a problem hiding this comment.
Duplicate target e2e_validator_out found. This could lead to confusion or errors in the build process. Consider removing or renaming one of the duplicates.
| tmconfig "github.com/cometbft/cometbft/config" | ||
| tmos "github.com/cometbft/cometbft/libs/os" | ||
| tmrand "github.com/cometbft/cometbft/libs/rand" | ||
| "github.com/cometbft/cometbft/types" | ||
| tmtime "github.com/cometbft/cometbft/types/time" |
There was a problem hiding this comment.
Tip
Codebase Verification
The imports in testnet.go have been successfully updated from tendermint to cometbft, and there are no remaining tendermint imports in the actual code. However, there is a comment that still references tendermint, which might cause confusion. It would be beneficial to update this comment to reflect the current usage of cometbft.
- Update the comment in
testnet.goto replace the mention oftendermintwithcometbft.
Analysis chain
Confirm that the updated imports from cometbft are correctly replacing the equivalent tendermint functionalities.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the CometBFT imports in `testnet.go` correctly replace Tendermint functionalities.
# Test: Compare the CometBFT and Tendermint functionalities used in the file. Expect: Equivalent functionalities are replaced.
rg --type go 'cometbft|tendermint' module/cmd/gravity/cmd/testnet.go
Length of output: 374
Script:
#!/bin/bash
# Description: Fetch the original Tendermint imports and functionalities from `testnet.go` to compare with CometBFT replacements.
# Test: Search for any remaining references to Tendermint in the file and list the functionalities.
rg --type go 'tendermint' module/cmd/gravity/cmd/testnet.go
Length of output: 133
| /// InterfaceDescriptor describes an interface type to be used with | ||
| /// accepts_interface and implements_interface and declared by declare_interface. | ||
| #[derive(Clone, PartialEq, ::prost::Message)] | ||
| pub struct InterfaceDescriptor { | ||
| /// name is the name of the interface. It should be a short-name (without | ||
| /// a period) such that the fully qualified name of the interface will be | ||
| /// package.name, ex. for the package a.b and interface named C, the | ||
| /// fully-qualified name will be a.b.C. | ||
| #[prost(string, tag = "1")] | ||
| pub name: ::prost::alloc::string::String, | ||
| /// description is a human-readable description of the interface and its | ||
| /// purpose. | ||
| #[prost(string, tag = "2")] | ||
| pub description: ::prost::alloc::string::String, | ||
| } | ||
| /// ScalarDescriptor describes an scalar type to be used with | ||
| /// the scalar field option and declared by declare_scalar. | ||
| /// Scalars extend simple protobuf built-in types with additional | ||
| /// syntax and semantics, for instance to represent big integers. | ||
| /// Scalars should ideally define an encoding such that there is only one | ||
| /// valid syntactical representation for a given semantic meaning, | ||
| /// i.e. the encoding should be deterministic. | ||
| #[derive(Clone, PartialEq, ::prost::Message)] | ||
| pub struct ScalarDescriptor { | ||
| /// name is the name of the scalar. It should be a short-name (without | ||
| /// a period) such that the fully qualified name of the scalar will be | ||
| /// package.name, ex. for the package a.b and scalar named C, the | ||
| /// fully-qualified name will be a.b.C. | ||
| #[prost(string, tag = "1")] | ||
| pub name: ::prost::alloc::string::String, | ||
| /// description is a human-readable description of the scalar and its | ||
| /// encoding format. For instance a big integer or decimal scalar should | ||
| /// specify precisely the expected encoding format. | ||
| #[prost(string, tag = "2")] | ||
| pub description: ::prost::alloc::string::String, | ||
| /// field_type is the type of field with which this scalar can be used. | ||
| /// Scalars can be used with one and only one type of field so that | ||
| /// encoding standards and simple and clear. Currently only string and | ||
| /// bytes fields are supported for scalars. | ||
| #[prost(enumeration = "ScalarType", repeated, tag = "3")] | ||
| pub field_type: ::prost::alloc::vec::Vec<i32>, | ||
| } | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] | ||
| #[repr(i32)] | ||
| pub enum ScalarType { | ||
| Unspecified = 0, | ||
| String = 1, | ||
| Bytes = 2, | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Several files in the codebase still do not use the new protobuf definitions InterfaceDescriptor and ScalarDescriptor.
- The definitions are present in
orchestrator/gravity_proto/src/prost/cosmos_proto.rsbut are not referenced or used in any other part of the module.
Analysis chain
Ensure that the new protobuf definitions InterfaceDescriptor and ScalarDescriptor are used appropriately throughout the module.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new protobuf definitions are used appropriately.
# Test: Search for the usage of `InterfaceDescriptor` and `ScalarDescriptor`. Expect: Proper usage throughout the module.
rg --type rust $'InterfaceDescriptor|ScalarDescriptor'
Length of output: 460
Script:
#!/bin/bash
# Description: Search for references to `InterfaceDescriptor` and `ScalarDescriptor` throughout the entire codebase to verify their usage.
# Test: Search for the usage of `InterfaceDescriptor` and `ScalarDescriptor` in all Rust files, not just their definitions.
rg --type rust --files-with-matches $'InterfaceDescriptor|ScalarDescriptor' | xargs rg 'InterfaceDescriptor|ScalarDescriptor'
Length of output: 319
Summary by CodeRabbit
New Features
Bug Fixes
happy_path_test.goto improve reliability of integration tests.Refactor
Chores
Documentation