Upgrade prep #561
Upgrade prep #561
Conversation
WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Orchestrator
participant Ethereum
User->>Orchestrator: Send SendToEthereum message
Orchestrator->>Ethereum: Execute transaction
Ethereum-->>Orchestrator: Confirm transaction completion
Orchestrator->>User: Confirm transaction status
📜 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
integration_tests/chain.go (1)
244-246: Consider removing commented-out code and empty line
If the legacy Amino codec and interface registration are no longer needed, it's better to remove the commented-out code entirely rather than leaving it in the codebase. This improves code readability and maintenance.
The empty line (246) doesn't add any value and can be removed for better code organization.
Could you clarify why this code was commented out instead of removed? If it's kept for reference, consider adding a comment explaining why or move it to documentation if it's important information for developers.
Here's a suggested change:
- //simapp.ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino) - //simapp.ModuleBasics.RegisterInterfaces(encodingConfig.InterfaceRegistry) -If you need to keep this code for reference, please add a comment explaining why:
// TODO: Legacy Amino codec registration might be needed in the future // simapp.ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino) // simapp.ModuleBasics.RegisterInterfaces(encodingConfig.InterfaceRegistry)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
.github/workflows/rust.ymlis excluded by!**/*.ymlgo.work.sumis excluded by!**/*.sum,!**/*.sumintegration_tests/go.modis excluded by!**/*.modintegration_tests/go.sumis excluded by!**/*.sum,!**/*.summodule/go.modis excluded by!**/*.modmodule/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
- integration_tests/chain.go (1 hunks)
| //simapp.ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino) | ||
| //simapp.ModuleBasics.RegisterInterfaces(encodingConfig.InterfaceRegistry) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Potential Impact on Test Configuration
Removing RegisterLegacyAminoCodec and RegisterInterfaces in integration_tests/chain.go may disrupt the encoding configurations, as Amino and InterfaceRegistry are still utilized within the file. This could lead to issues in test execution.
🔗 Analysis chain
LGTM, but verify impact on the rest of the codebase
The changes look good as they don't introduce any issues and seem to be part of cleaning up deprecated code. However, it's important to ensure that removing these registrations doesn't have unintended consequences elsewhere in the codebase.
Please run the following script to verify that the removed registrations aren't used elsewhere in the codebase:
If these methods are still used elsewhere, we may need to reconsider this change or update those occurrences as well.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of RegisterLegacyAminoCodec and RegisterInterfaces
# Search for RegisterLegacyAminoCodec usage
echo "Searching for RegisterLegacyAminoCodec usage:"
rg --type go "RegisterLegacyAminoCodec"
# Search for RegisterInterfaces usage
echo "Searching for RegisterInterfaces usage:"
rg --type go "RegisterInterfaces"
Length of output: 2685
Script:
#!/bin/bash
# Description: Check for imports and usage of Amino and InterfaceRegistry in integration_tests/chain.go
# Search for Amino imports
echo "Searching for Amino imports in integration_tests/chain.go:"
rg --type go "Amino" integration_tests/chain.go
# Search for InterfaceRegistry imports or usage
echo "Searching for InterfaceRegistry imports or usage in integration_tests/chain.go:"
rg --type go "InterfaceRegistry" integration_tests/chain.go
# Additionally, check for usages of codec and registry in the file
echo "Searching for codec or registry usages in integration_tests/chain.go:"
rg --type go "(codec|registry)" integration_tests/chain.go
Length of output: 1244
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
Makefile (1)
Line range hint
1-61: Consider updating the PR title and description.The current PR title "Fixing dependencies" doesn't accurately reflect the nature of the changes made. The modifications are primarily updates to test binary paths in the Makefile, rather than dependency fixes.
Consider updating the PR title to something like "Update test binary paths in Makefile" and adding a brief description explaining the reason for these changes. This will help other developers understand the purpose of this PR more quickly and accurately.
integration_tests/transaction_stress_test.go (1)
123-123: Usetime.Secondfor better readability in sleep duration.For improved code readability, consider using
time.Secondto represent durations in seconds. This makes the code more intuitive.Apply this diff:
- time.Sleep(2000 * time.Millisecond) + time.Sleep(2 * time.Second)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
.github/workflows/integration-tests.ymlis excluded by!**/*.ymlgo.work.sumis excluded by!**/*.sum,!**/*.sumintegration_tests/go.modis excluded by!**/*.mod
📒 Files selected for processing (3)
- Makefile (1 hunks)
- integration_tests/transaction_stress_test.go (4 hunks)
- module/x/gravity/keeper/migrations.go (0 hunks)
💤 Files with no reviewable changes (1)
- module/x/gravity/keeper/migrations.go
🔇 Additional comments (4)
Makefile (3)
54-54: LGTM. Consistent change.The update to the test binary path is consistent with the previous target and the AI-generated summary.
61-61: LGTM. Consistent change.The update to the test binary path is consistent with the previous targets and the AI-generated summary.
51-51: LGTM. Verify the new test binary name.The update to the test binary path looks correct. It's consistent across all changed targets.
Let's verify the existence of the new test binary:
integration_tests/transaction_stress_test.go (1)
9-9: Importingsdkmathis appropriate.The addition of
sdkmath "cosmossdk.io/math"is necessary for the mathematical operations introduced in the code changes.
|
|
||
|
|
||
| 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.
LGTM. Consistent change, but duplicate target detected.
The update to the test binary path is consistent with the previous targets and the AI-generated summary.
However, there's a duplicate target definition for e2e_validator_out (lines 25-26). Consider removing the duplicate target to avoid confusion and potential conflicts.
| expectedBalance := initialBalances[validator.ethereumKey.address].Add(sdkmath.NewInt(eth_sent_amt * transactions_per_validator)) | ||
| if balance.LT(expectedBalance) { | ||
| s.T().Logf("funds not received yet, dest balance: %s, expected: %s", balance.String(), expectedBalance.String()) |
There was a problem hiding this comment.
Avoid mixing pointer and value types with sdkmath.Int.
In the calculation and comparison of balances, you're mixing pointers (*sdkmath.Int) and values (sdkmath.Int). The Add and LT methods are defined on sdkmath.Int values, not pointers, which can lead to unexpected behavior.
To fix this issue, ensure that all sdkmath.Int instances are values when performing operations:
-
Update the
initialBalancesmap to store values:- initialBalances := make(map[string]*sdkmath.Int) + initialBalances := make(map[string]sdkmath.Int)
-
Adjust the assignment when recording initial balances:
- initialBalances[validator.ethereumKey.address] = balance + initialBalances[validator.ethereumKey.address] = *balance
-
Ensure that
balanceis a value when comparing:- if balance.LT(expectedBalance) { + if (*balance).LT(expectedBalance) {
Or, better yet, dereference
balanceonce:balanceValue := *balance if balanceValue.LT(expectedBalance) { // ... }
This ensures consistency in your use of sdkmath.Int and prevents potential runtime errors due to type mismatches.
| initialBalances := make(map[string]*sdkmath.Int) | ||
|
|
||
| // Record initial balances for each validator | ||
| for _, validator := range s.chain.validators { | ||
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(validator.ethereumKey.address), testERC20contract) | ||
| s.Require().NoError(err, "error getting initial balance") | ||
| initialBalances[validator.ethereumKey.address] = balance | ||
| s.T().Logf("Initial balance for validator %s: %s", validator.ethereumKey.address, balance.String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider storing sdkmath.Int values instead of pointers to prevent potential nil pointer dereferences.
In the initialBalances map, you're currently storing pointers to sdkmath.Int (*sdkmath.Int). This could lead to potential nil pointer dereferences if any of the pointers become nil. Since sdkmath.Int is an immutable value type, it's safer and more efficient to store the values directly.
You can modify the map to store sdkmath.Int values:
- initialBalances := make(map[string]*sdkmath.Int)
+ initialBalances := make(map[string]sdkmath.Int)And update the assignment accordingly:
- initialBalances[validator.ethereumKey.address] = balance
+ initialBalances[validator.ethereumKey.address] = *balanceThis ensures that you're working with values, reducing the risk of nil pointer dereferences.
📝 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.
| initialBalances := make(map[string]*sdkmath.Int) | |
| // Record initial balances for each validator | |
| for _, validator := range s.chain.validators { | |
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(validator.ethereumKey.address), testERC20contract) | |
| s.Require().NoError(err, "error getting initial balance") | |
| initialBalances[validator.ethereumKey.address] = balance | |
| s.T().Logf("Initial balance for validator %s: %s", validator.ethereumKey.address, balance.String()) | |
| } | |
| initialBalances := make(map[string]sdkmath.Int) | |
| // Record initial balances for each validator | |
| for _, validator := range s.chain.validators { | |
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(validator.ethereumKey.address), testERC20contract) | |
| s.Require().NoError(err, "error getting initial balance") | |
| initialBalances[validator.ethereumKey.address] = *balance | |
| s.T().Logf("Initial balance for validator %s: %s", validator.ethereumKey.address, balance.String()) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (21)
module/x/gravity/keeper/signer_set.go (1)
41-41: Approve error handling changes in GetUnsignedSignerSetTxs and consider refactoring.The modifications to use
errors.Wrapfinstead ofsdkerrors.Wrapfare consistent with the import change and previous function modifications. The error handling logic remains intact, ensuring that the function's behavior is preserved.Consider refactoring the duplicate error handling code into a separate function to reduce code duplication. For example:
func panicOnInvalidSignerSet(otx types.OutgoingTx) { if _, ok := otx.(*types.SignerSetTx); !ok { panic(errors.Wrapf(types.ErrInvalid, "couldn't cast to signer set for %s", otx)) } }Then, you can use this function in both places:
panicOnInvalidSignerSet(cotx) // and panicOnInvalidSignerSet(otx)This refactoring would improve code maintainability and reduce the risk of inconsistencies in future updates.
Also applies to: 52-52
module/x/gravity/types/ethereum_signature.go (1)
89-89: Approve error handling update, consider refactoringThe change from
sdkerrors.Wraptoerrors.Wrapis consistent with the previous modifications. The error message and overall logic remain unchanged, maintaining consistency throughout the file.Consider refactoring the common error handling logic for invalid Ethereum signer addresses into a separate function to improve code reusability and maintainability. For example:
func validateEthereumSigner(signer string) error { if !common.IsHexAddress(signer) { return errors.Wrap(ErrInvalid, "ethereum signer must be address") } return nil }Then, you can use this function in all three Validate() methods:
if err := validateEthereumSigner(u.EthereumSigner); err != nil { return err }This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain.
module/x/gravity/keeper/contract_call.go (1)
Line range hint
1-79: Summary: Consistent error handling refactoring with potential for broader impact.The changes in this file represent a shift from using
sdkerrors.Wrapftoerrors.Wrapffor error handling. This modification is applied consistently throughout the file and doesn't alter the logic or behavior of the functions. However, it's crucial to ensure that:
- This change is part of a broader refactoring effort across the entire codebase.
- The new error handling approach is compatible with existing error checking and handling mechanisms in other parts of the system.
- Any error logging, monitoring, or handling systems are updated to work with the new error types if necessary.
Consider documenting this change in the PR description to provide context for reviewers and future maintainers.
module/x/gravity/types/ethereum_signer.go (3)
20-20: LGTM with a minor suggestion for improvementThe change from
sdkerrorstoerrorsfromcosmossdk.io/errorsis consistent with the overall modifications in the file. The functionality remains unchanged.Consider using a more specific error type instead of
ErrInvalidfor better error handling. For example:return nil, errors.Wrap(ErrInvalidPrivateKey, "private key is nil")This would require defining
ErrInvalidPrivateKeyin the appropriate error file.
82-82: LGTM with a suggestion for consistencyThe changes from
sdkerrorstoerrorsfromcosmossdk.io/errorsare consistent with the overall modifications in the file. The functionality remains unchanged.For consistency, consider using
errors.Wrapinstead oferrors.Wrapfwhen not using format specifiers. For example:return errors.Wrap(err, fmt.Sprintf("signature to public key sig %x hash %x", sigCopy, hash))This makes the error wrapping consistent throughout the function and improves readability.
Also applies to: 86-86
Incomplete Refactoring:
sdkerrorsStill in UseThe replacement of
sdkerrorswithcosmossdk.io/errorsis not yet consistent across the codebase. The following files still usesdkerrors:
module/x/gravity/handler.gomodule/x/gravity/types/msgs.gomodule/x/gravity/types/ethereum_event.gomodule/x/gravity/types/codec.gomodule/app/app.goPlease update these files to use
cosmossdk.io/errorsto maintain consistency with the Cosmos SDK conventions.🔗 Analysis chain
Line range hint
1-90: Overall change looks good, consider documenting the rationaleThe replacement of
sdkerrorswitherrorsfromcosmossdk.io/errorsis consistent throughout the file. This change doesn't affect the functionality of the code but improves consistency with the Cosmos SDK.To ensure this change is part of a larger refactoring effort, please run the following script:
This will help verify if the change is consistent across the project.
Consider adding a comment at the top of the file or in the PR description explaining the rationale behind this change, such as:
// This file has been updated to use cosmossdk.io/errors instead of sdkerrors // as part of a larger refactoring effort to align with the latest Cosmos SDK conventions.This will help future maintainers understand the context of these changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar changes in other files # Test: Search for files still using sdkerrors echo "Files still using sdkerrors:" rg --type go 'sdkerrors' # Test: Search for files using the new error package echo "Files using cosmossdk.io/errors:" rg --type go 'cosmossdk.io/errors'Length of output: 4984
module/x/gravity/types/types.go (1)
Line range hint
1-24: Summary of changes and potential impactThe changes in this file are part of a larger refactoring effort to update error handling:
- Updated import from
sdkerrorstoerrorsfromcosmossdk.io/errors.- Changed error wrapping in
ValidateBasicmethod fromsdkerrors.Wraptoerrors.Wrap.These changes maintain the same functionality while updating to a new error handling package. The limited scope of changes reduces the risk of unintended side effects. However, it's crucial to ensure these changes are applied consistently across the entire codebase to maintain uniform error handling.
Consider the following recommendations:
- Update all relevant documentation to reflect the new error handling approach.
- Ensure that any error logging or monitoring systems are updated to work with the new error types.
- Review and update any error handling middleware or global error handlers to be compatible with the new error package.
- Consider creating a migration guide for developers to understand how to work with the new error handling approach.
module/x/gravity/types/ethereum_event.go (3)
119-128: LGTM: Consistent error handling improvements.The changes in the
SendToCosmosEvent.Validate()method consistently apply the new error wrapping approach usingerrors.Wrap. This improves error context and aligns with the new import.Consider using
errors.Wrapffor the last error to include the invalid address in the error message:return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid cosmos receiver address: %s", stce.CosmosReceiver)This would provide more context in error scenarios.
155-155: LGTM: Consistent error handling improvement.The change in the
ERC20DeployedEvent.Validate()method consistently applies the new error wrapping approach usingerrors.Wrap. This improves error context and aligns with the changes made throughout the file.For consistency with other validation methods, consider wrapping the error returned by
sdk.ValidateDenom:if err := sdk.ValidateDenom(e20de.CosmosDenom); err != nil { return errors.Wrap(err, "invalid cosmos denom") }This would provide a consistent error wrapping approach throughout the file.
Line range hint
1-176: Suggestion: Apply consistent error handling across all Validate() methods.While the changes made improve error handling in several methods, there are a few methods that could benefit from the same treatment for consistency:
- In
ContractCallExecutedEvent.Validate():if ccee.EventNonce == 0 { return errors.Wrap(ErrInvalid, "event nonce cannot be 0") }
- In
SignerSetTxExecutedEvent.Validate():if sse.EventNonce == 0 { return errors.Wrap(ErrInvalid, "event nonce cannot be 0") } if sse.Members == nil { return errors.Wrap(ErrInvalid, "members cannot be nil") } for i, member := range sse.Members { if err := member.ValidateBasic(); err != nil { return errors.Wrapf(err, "ethereum signer %d error", i) } }These changes would ensure a consistent error handling approach across all
Validate()methods in the file.module/x/gravity/types/codec.go (1)
133-133: LGTM with a minor suggestion: Consistent error handling in UnpackConfirmation.The changes in the UnpackConfirmation function are consistent with the new error handling approach. However, there's a small inconsistency in the error message on line 138.
Consider updating the error message for consistency:
- return nil, errors.Wrapf(sdkerrors.ErrUnpackAny, "cannot unpack Any into EthereumSignature %T", any) + return nil, errors.Wrapf(sdkerrors.ErrUnpackAny, "cannot unpack Any into EthereumTxConfirmation %T", any)Also applies to: 138-138
module/x/gravity/keeper/pool.go (1)
98-103: LGTM: Consistent error handling updates.The changes from
sdkerrors.Wrapftoerrors.Wrapfandsdkerrors.Wraptoerrors.Wrapare consistent with the previous updates. They maintain the same functionality while using the new error handling mechanism from the Cosmos SDK.As a minor suggestion, consider using sentinel errors for specific error cases to improve error handling and make it easier for callers to check for specific error conditions.
For example, you could define sentinel errors in the
typespackage:var ( ErrIDNotFound = errors.Register(ModuleName, 1, "id not found in send to ethereum pool") ErrMintVouchers = errors.Register(ModuleName, 2, "failed to mint voucher coins") ErrSendCoins = errors.Register(ModuleName, 3, "failed to send coins from module account") )Then use them in the
cancelSendToEthereumfunction:if send == nil { return errors.Wrapf(types.ErrIDNotFound, "id %d", id) } // ... if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, coinsToRefund); err != nil { return errors.Wrapf(types.ErrMintVouchers, "coins: %s, error: %v", coinsToRefund, err) } if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, sender, coinsToRefund); err != nil { return errors.Wrapf(types.ErrSendCoins, "sender: %s, coins: %s, error: %v", sender, coinsToRefund, err) }This approach would make it easier for callers to check for specific error conditions and provide more context in the error messages.
module/x/gravity/keeper/genesis.go (1)
164-169: Consistent improvement across all confirmation typesThe restructuring of the ContractCallTxConfirmation packing completes the consistent application of this pattern across all confirmation types. This systematic approach to code improvement enhances overall readability and maintainability.
Given the similarity in structure across these confirmation types, consider if there's an opportunity for further refactoring. For example, a generic function for packing confirmations could potentially reduce code duplication.
Would you like assistance in exploring a more generic approach to packing these confirmations?
module/x/gravity/types/outgoing_tx.go (2)
213-213: Consider returning errors instead of using panicThe error handling in the
packCallfunction has been updated to use the newerrors.Wrapmethod, which is consistent with the import change. While this maintains the current functionality, consider refactoring this function to return errors instead of usingpanic. This would allow for more graceful error handling by the calling functions.Here's a suggested refactoring:
func packCall(abiString, method string, args []interface{}) ([]byte, error) { encodedCall, err := abi.JSON(strings.NewReader(abiString)) if err != nil { return nil, errors.Wrap(err, "bad ABI definition in code") } abiEncodedCall, err := encodedCall.Pack(method, args...) if err != nil { return nil, errors.Wrap(err, "packing checkpoint") } return crypto.Keccak256Hash(abiEncodedCall[4:]).Bytes(), nil }This change would require updating the calling functions to handle the returned error, but it would improve the overall error handling in the package.
Also applies to: 217-217
Line range hint
1-220: General code improvements and TODO resolutionWhile reviewing the entire file, I noticed a few areas for potential improvement:
The TODO comment on line 35 should be addressed:
// TODO: do we need a prefix byte for the different types?Consider implementing the prefix byte if it's necessary, or remove the comment if it's no longer relevant.
The
GetCheckpointmethods forSignerSetTx,BatchTx, andContractCallTxhave very similar structures. Consider refactoring these into a common helper function to reduce code duplication.The use of
panicin various methods (e.g.,GetCheckpoint) could be replaced with error returns for better error handling throughout the package.Consider adding more inline comments to explain the purpose and functionality of complex operations, especially in the
GetCheckpointmethods.Would you like assistance in implementing any of these suggestions?
module/x/gravity/keeper/ethereum_event_vote.go (1)
Line range hint
1-383: Summary: Error handling update completed successfully.The changes in this file are part of a larger effort to update the error handling approach, likely to align with recent updates in the Cosmos SDK. The modifications are minimal and focused:
- Updated import from
sdkerrorstocosmossdk.io/errors.- Updated error wrapping in the
recordEventVotefunction to use the new import.These changes should not affect the functionality of the code, but they do modernize the error handling approach. To ensure a smooth transition:
- Verify that all instances of
sdkerrorshave been replaced throughout the file.- Add or update unit tests to confirm that error handling behavior remains consistent.
- Keep an eye on the error handling in related files to ensure consistency across the codebase.
Consider creating a central document or guide that outlines the new error handling approach. This will help maintain consistency as you continue to update other parts of the codebase and onboard new developers to the project.
module/x/gravity/keeper/msg_server_test.go (5)
81-142: LGTM: Comprehensive error handling tests addedThe new test cases for
TestMsgServer_SubmitEthereumSignaturesignificantly improve the test coverage by handling various error scenarios. They check for invalid confirmation types, non-existent validators, invalid Ethereum signatures, and duplicate confirmations.Consider adding a test case for an expired nonce, if applicable to your system's design.
370-405: LGTM: Comprehensive error handling for SubmitEthereumEventThe new test cases for
TestMsgServer_SubmitEthereumEventsignificantly enhance the test coverage by checking for invalid signer addresses, non-existent signers, and non-contiguous event nonces. These scenarios are crucial for maintaining the integrity of the event submission process.Consider adding a test case for an event with an invalid structure or content, if applicable to your system's design.
453-527: LGTM: Extensive error handling tests for SetDelegateKeysThe new test cases for
TestMsgServer_SetDelegateKeysprovide comprehensive coverage of potential error scenarios. They check for invalid addresses, duplicate addresses, invalid signatures, and non-existent validators. This thorough testing helps ensure the robustness of the delegate key setting process.Consider adding a test case for setting delegate keys with mismatched nonce values in the signature and message, if applicable to your system's design.
578-583: LGTM: Minor updates to TestEthVerifyThe changes to
TestEthVerifyare minor and do not affect the test's functionality. The updated variable names improve readability.Consider removing or commenting out the
fmt.Printlnstatement on line 605 if it's not needed for regular test runs. If it's useful for debugging, consider wrapping it in a debug flag check.Also applies to: 589-590, 606-607
Line range hint
1-619: Great job on improving test coverage!The changes in this file significantly enhance the test suite for the Gravity module's message handling. The new error handling tests cover a wide range of scenarios, improving the robustness and reliability of the code. The additions are well-structured and consistent with good testing practices.
Consider creating helper functions for common test setup steps to reduce code duplication and improve maintainability. This could include functions for creating test messages with specific error conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- integration_tests/happy_path_test.go (2 hunks)
- integration_tests/setup_test.go (1 hunks)
- integration_tests/validator_out.go (1 hunks)
- module/x/gravity/handler.go (3 hunks)
- module/x/gravity/keeper/batch.go (5 hunks)
- module/x/gravity/keeper/contract_call.go (4 hunks)
- module/x/gravity/keeper/ethereum_event_handler.go (7 hunks)
- module/x/gravity/keeper/ethereum_event_vote.go (2 hunks)
- module/x/gravity/keeper/genesis.go (3 hunks)
- module/x/gravity/keeper/grpc_query.go (7 hunks)
- module/x/gravity/keeper/msg_server.go (10 hunks)
- module/x/gravity/keeper/msg_server_test.go (13 hunks)
- module/x/gravity/keeper/pool.go (3 hunks)
- module/x/gravity/keeper/signer_set.go (4 hunks)
- module/x/gravity/types/codec.go (6 hunks)
- module/x/gravity/types/errors.go (1 hunks)
- module/x/gravity/types/ethereum_event.go (4 hunks)
- module/x/gravity/types/ethereum_signature.go (4 hunks)
- module/x/gravity/types/ethereum_signer.go (2 hunks)
- module/x/gravity/types/genesis.go (3 hunks)
- module/x/gravity/types/msgs.go (7 hunks)
- module/x/gravity/types/outgoing_tx.go (2 hunks)
- module/x/gravity/types/types.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- module/x/gravity/keeper/batch.go
- module/x/gravity/keeper/msg_server.go
- module/x/gravity/types/errors.go
🔇 Additional comments (62)
module/x/gravity/handler.go (3)
57-57: Verify consistency in error handling.The change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the upgrade described in the AI summary and the previous change in this file. However,sdkerrors.ErrUnknownRequestis still being used.Please verify if
sdkerrors.ErrUnknownRequestshould also be updated to use the new error package for consistency. This is similar to the previous instance in theNewHandlerfunction. You can use the same verification script as before to check for usage patterns across the codebase.
46-46: Verify consistency in error handling.The change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the upgrade described in the AI summary. However,sdkerrors.ErrUnknownRequestis still being used.Please verify if
sdkerrors.ErrUnknownRequestshould also be updated to use the new error package for consistency. You can check if similar changes have been made in other files:#!/bin/bash # Check for usage of sdkerrors.ErrUnknownRequest in other files rg --type go 'sdkerrors\.ErrUnknownRequest' . # Check for usage of errors.ErrUnknownRequest in other files rg --type go 'errors\.ErrUnknownRequest' .
4-4: Consider removing unused import.The new import
"cosmossdk.io/errors"has been added, which is consistent with the changes described in the AI summary. However, the old importsdkerrors "github.com/cosmos/cosmos-sdk/types/errors"is still present. Ifsdkerrorsis no longer used in the file, consider removing it to avoid confusion and maintain clean imports.To verify if
sdkerrorsis still used elsewhere in the file, run the following command:module/x/gravity/keeper/signer_set.go (1)
24-24: Approve error handling change in SignerSetExecuted.The modification to use
errors.Wrapfinstead ofsdkerrors.Wrapfis consistent with the import change. The error handling logic remains intact, ensuring that the function's behavior is preserved.module/x/gravity/types/ethereum_signature.go (2)
57-57: Approve error handling updateThe change from
sdkerrors.Wraptoerrors.Wrapis consistent with the import statement modification. The error message and overall logic remain unchanged, which is good for maintaining consistency.
73-73: Approve error handling updateThe change from
sdkerrors.Wraptoerrors.Wrapis consistent with the previous modifications. The error message and overall logic remain unchanged, maintaining consistency throughout the file.module/x/gravity/keeper/contract_call.go (3)
20-20: LGTM: Error handling update is consistent.The change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the import statement modification. The error type and message remain unchanged, preserving the original behavior.
32-32: LGTM: Consistent error handling updates throughout the file.The changes from
sdkerrors.Wrapftoerrors.Wrapfon lines 32 and 56 are consistent with the previous modifications. The error types and messages remain unchanged, preserving the original behavior across all instances.These changes improve consistency in error handling throughout the file without altering the underlying logic or behavior of the functions.
Also applies to: 56-56
8-8: Verify consistency of error package usage across the codebase.The import statement has been changed from
sdkerrorstoerrors. While this change is consistent with the modifications in this file, it's important to ensure that this change is part of a broader refactoring effort.Let's verify if this change is consistent across the codebase:
✅ Verification successful
Consistent usage of error packages confirmed across the codebase.
All instances of
sdkerrors.Wrapfhave been replaced witherrors.Wrapf, and no files contain mixed import statements ofsdkerrorsandcosmossdk.io/errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for mixed usage of error packages in the codebase. # Test 1: Check for remaining uses of sdkerrors.Wrapf echo "Checking for remaining uses of sdkerrors.Wrapf:" rg --type go 'sdkerrors\.Wrapf' # Test 2: Check for new uses of errors.Wrapf echo "Checking for new uses of errors.Wrapf:" rg --type go 'errors\.Wrapf' # Test 3: Check for mixed import statements echo "Checking for files with mixed import statements:" rg --type go -e 'import \(.*?sdkerrors.*?\)' -e 'import \(.*?cosmossdk\.io/errors.*?\)' -l | sort | uniq -dLength of output: 5785
module/x/gravity/types/types.go (2)
21-21: Approve error handling change and verify consistencyThe change from
sdkerrors.Wraptoerrors.Wrapis consistent with the import update and maintains the same functionality.To ensure consistent error handling across the codebase, please run the following script:
This script will help identify any inconsistencies in error handling and highlight any custom error types that might need updating.
9-9: Approve import change and verify consistencyThe change from
sdkerrorstoerrorsfromcosmossdk.io/errorsis approved. This update likely aligns with a broader upgrade or refactoring effort.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies in error handling across the project.
module/x/gravity/types/ethereum_event.go (2)
8-8: LGTM: Import change for improved error handling.The addition of the
"cosmossdk.io/errors"import is consistent with the changes in error handling throughout the file. This change aligns with modern practices in the Cosmos SDK ecosystem.
138-138: LGTM: Consistent error handling improvement.The change in the
BatchExecutedEvent.Validate()method consistently applies the new error wrapping approach usingerrors.Wrap. This improves error context and aligns with the changes made throughout the file.module/x/gravity/types/codec.go (7)
4-4: LGTM: New error package import.The addition of the
errorspackage fromcosmossdk.iois a good improvement for more specific error handling.
89-89: LGTM: Updated error wrapping.The change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the new import while still using the specificsdkerrors.ErrPackAnyerror code.
94-94: LGTM: Consistent error wrapping update.The change from
sdkerrors.Wraptoerrors.Wrapmaintains consistency with the new error handling approach.
104-104: LGTM: Consistent error handling in UnpackEvent.The changes in the UnpackEvent function maintain consistency with the new error handling approach while preserving the use of specific
sdkerrorscodes.Also applies to: 109-109
118-118: LGTM: Consistent error handling in PackConfirmation.The changes in the PackConfirmation function align with the new error handling approach while maintaining the use of specific
sdkerrorscodes.Also applies to: 123-123
147-147: LGTM: Consistent error handling in PackOutgoingTx and UnpackOutgoingTx.The changes in both PackOutgoingTx and UnpackOutgoingTx functions maintain consistency with the new error handling approach while preserving the use of specific
sdkerrorscodes.Also applies to: 152-152, 160-160, 165-165
Line range hint
1-168: Summary: Improved error handling across the file.The changes in this file consistently update the error handling approach by using the
errorspackage fromcosmossdk.io. This improvement provides more specific and potentially more informative error handling throughout the module. The changes maintain the use of specificsdkerrorscodes, ensuring compatibility with existing error handling patterns.A minor inconsistency in an error message was noted and a suggestion for correction was provided.
Overall, these changes represent a positive step towards more robust error handling in the module.
module/x/gravity/keeper/pool.go (3)
7-7: LGTM: New import for improved error handling.The addition of
"cosmossdk.io/errors"is a good practice. This package provides a more robust error handling mechanism for Cosmos SDK modules, allowing for better error wrapping and context preservation.
84-84: LGTM: Updated error wrapping.The change from
sdkerrors.Wraptoerrors.Wrapis consistent with the new import and maintains the same functionality. This update improves error handling by using the latest Cosmos SDK error wrapping mechanism.
Line range hint
1-165: Summary: Improved error handling in the gravity module.The changes in this file consistently update the error handling mechanism to use the
cosmossdk.io/errorspackage. This improvement aligns with best practices for Cosmos SDK modules and provides better error wrapping and context preservation.Key points:
- New import of
cosmossdk.io/errors- Consistent updates from
sdkerrors.Wrapandsdkerrors.Wrapftoerrors.Wrapanderrors.Wrapf- Functionality remains the same, but with improved error handling
These changes enhance the module's compatibility with the latest Cosmos SDK error handling practices and should make debugging and error tracing easier in the future.
integration_tests/validator_out.go (3)
132-157: Improved code structure for validator address handling.The changes enhance readability and maintainability by assigning validator addresses to separate variables before using them in
QueryValidatorRequest. This approach reduces the risk of errors and improves code clarity.
Line range hint
1-165: Overall assessment: Improved code structure with a timing adjustment.The changes in this file primarily focus on improving code readability and structure, particularly in handling validator addresses. These modifications are beneficial for maintainability and reduce the risk of errors.
The increase in the timeout for jailing status confirmation is the only functional change, and its impact should be carefully considered. Once the reasoning behind this change is clarified, we can better assess its appropriateness.
In summary, the changes appear to be positive, with no introduction of major issues or bugs. The code maintains its original functionality while becoming more readable and potentially more reliable in terms of test execution.
163-163: Increased timeout for jailing status confirmation.The timeout for confirming the jailing status of validators has been increased from 5 minutes to 8 minutes. While this change may reduce false negatives if the jailing process occasionally takes longer than expected, it could also potentially mask performance issues.
Could you please clarify the reasoning behind this increase? Are there known scenarios where the jailing process consistently takes longer than 5 minutes?
To help verify the impact of this change, you can run the following script to check the typical time taken for jailing in recent test runs:
This will help determine if the increased timeout is necessary based on actual test data.
module/x/gravity/keeper/ethereum_event_handler.go (8)
6-6: LGTM: Updated error handling package import.The import of
cosmossdk.io/errorsis consistent with the migration fromsdkerrorsto the new error handling approach. This update aligns with best practices for keeping dependencies up-to-date.
18-18: LGTM: Updated error wrapping inDetectMaliciousSupply.The error wrapping has been correctly updated to use
errors.Wrapffrom the new package. The error message and the use oftypes.ErrSupplyOverflowremain unchanged, maintaining the original error semantics.
40-40: LGTM: Updated error wrapping inHandlefunction.The error wrapping for minting coins has been correctly updated to use
errors.Wrapffrom the new package. The error message remains unchanged, maintaining the original error semantics.
82-82: LGTM: Updated error wrapping inHandlefunction's default case.The error wrapping for invalid event types has been correctly updated to use
errors.Wrapffrom the new package. The error message and the use oftypes.ErrInvalidremain unchanged, maintaining the original error semantics.
88-91: LGTM: Updated error wrapping inverifyERC20DeployedEventfunction.Multiple instances of error wrapping have been correctly updated to use
errors.Wrapffrom the new package. All error messages and the use oftypes.ErrInvalidERC20Eventremain unchanged, maintaining the original error semantics throughout the function.Also applies to: 112-115, 119-122, 126-129, 133-136
144-147: LGTM: Updated error wrapping inverifyERC20Tokenfunction.The error wrapping instances have been correctly updated to use
errors.Wrapffrom the new package. All error messages and the use oftypes.ErrInvalidERC20Eventremain unchanged, maintaining the original error semantics.Also applies to: 151-154
182-185: LGTM: Updated final error wrapping inverifyERC20Tokenfunction.The final instance of error wrapping has been correctly updated to use
errors.Wrapffrom the new package. The error message and the use oftypes.ErrInvalidERC20Eventremain unchanged, maintaining the original error semantics.
Line range hint
1-189: Summary: Successful migration to new error handling package.The changes in this file consistently update the error handling from
sdkerrorstocosmossdk.io/errors. All instances ofsdkerrors.Wrapfhave been replaced witherrors.Wrapf, maintaining the original error semantics and messages. The migration has been implemented correctly, and no logic changes were introduced. This update aligns the code with the latest best practices in the Cosmos SDK ecosystem.module/x/gravity/keeper/genesis.go (3)
129-133: Improved readability in SignerSetTxConfirmation packingThe restructuring of the SignerSetTxConfirmation packing improves code readability and maintainability. Each field is now explicitly named, which aligns with best practices for struct initialization and makes the code easier to understand and modify in the future.
146-151: Consistent improvement in BatchTxConfirmation packingThe restructuring of the BatchTxConfirmation packing is consistent with the previous change to SignerSetTxConfirmation. This change enhances code readability and maintainability by explicitly naming each field. The consistency in these changes contributes to a more uniform and understandable codebase.
Line range hint
129-169: Summary of changes in ExportGenesis functionThe modifications in the
ExportGenesisfunction consistently improve the readability and maintainability of the code across all confirmation types (SignerSetTx, BatchTx, and ContractCallTx). These changes:
- Enhance code clarity by explicitly naming fields in struct initializations.
- Maintain consistent structure across different confirmation types.
- Do not alter the underlying functionality of the code.
These improvements align with best practices for code clarity and will likely make future maintenance and understanding of the codebase easier. Consider exploring opportunities for further refactoring to reduce potential code duplication across these similar structures.
module/x/gravity/types/msgs.go (8)
6-6: LGTM: Import ofcosmossdk.io/errorspackage.The addition of this import is consistent with the changes made throughout the file, replacing
sdkerrors.Wrapwitherrors.Wrap. This update aligns with using the latest SDK version and its error handling mechanisms.
45-51: LGTM: Consistent error wrapping update in MsgDelegateKeys.ValidateBasic.The replacement of
sdkerrors.Wrapwitherrors.Wrapis applied consistently across all error wrapping calls in this method. The error types remain unchanged, maintaining backward compatibility. This change aligns with the new error handling approach without altering the method's logic or behavior.
83-83: LGTM: Error wrapping update in MsgSubmitEthereumEvent.ValidateBasic.The replacement of
sdkerrors.Wrapwitherrors.Wrapis consistent with the previous updates. The error type remains unchanged, maintaining backward compatibility. This change aligns with the new error handling approach without altering the method's logic or behavior.
122-122: LGTM: Error wrapping update in MsgSubmitEthereumTxConfirmation.ValidateBasic.The replacement of
sdkerrors.Wrapwitherrors.Wrapis consistent with the previous updates. The error type remains unchanged, maintaining backward compatibility. This change aligns with the new error handling approach without altering the method's logic or behavior.
174-191: LGTM: Consistent error wrapping update in MsgSendToEthereum.ValidateBasic.The replacement of
sdkerrors.Wrapwitherrors.Wrapis applied consistently across all error wrapping calls in this method. The error types remain unchanged, maintaining backward compatibility. These changes align with the new error handling approach without altering the method's logic or behavior.
229-232: LGTM: Error wrapping update in MsgCancelSendToEthereum.ValidateBasic.The use of
errors.Wrapis consistent with the previous updates. The error types remain unchanged, maintaining backward compatibility. These changes align with the new error handling approach without altering the method's logic or behavior.
269-273: LGTM: Error wrapping update in MsgEthereumHeightVote.ValidateBasic.The use of
errors.Wrapis consistent with the previous updates. The error types remain unchanged, maintaining backward compatibility. These changes align with the new error handling approach without altering the method's logic or behavior.
Line range hint
1-293: Summary: Consistent update to error handling mechanism.The changes in this file are focused on updating the error wrapping mechanism from
sdkerrors.Wraptoerrors.Wrap. This update is applied consistently across all methods in the file. The modifications:
- Introduce a new import for the
errorspackage fromcosmossdk.io.- Replace all instances of
sdkerrors.Wrapwitherrors.Wrap.- Maintain the same error types, ensuring backward compatibility.
These changes align the code with more modern error handling practices in the Cosmos SDK without altering the logic or behavior of the existing methods. The systematic and consistent nature of these updates reduces the risk of introducing new bugs while improving the overall code quality.
module/x/gravity/keeper/ethereum_event_vote.go (2)
Line range hint
28-33: LGTM: Updated error wrapping inrecordEventVote.The error wrapping has been correctly updated to use
errors.Wrapffrom the new import, maintaining the same error message structure. This change is consistent with the import modification and should not alter the function's behavior.To ensure the error handling behavior remains consistent, please add a unit test that triggers this error condition:
func TestRecordEventVoteErrorHandling(t *testing.T) { // Setup your keeper and context here event := types.EthereumEvent{ // Set up your event here, ensure the nonce is not contiguous } val := sdk.ValAddress([]byte("testValidator")) _, err := keeper.recordEventVote(ctx, event, val) require.Error(t, err) require.Contains(t, err.Error(), "non contiguous event nonce") // Add more specific checks on the error message if needed }This test will verify that the error is still being wrapped and returned correctly with the new error handling approach.
8-8: LGTM: Import change for error handling.The change from
sdkerrorstocosmossdk.io/errorsaligns with updates in the Cosmos SDK. This should provide similar functionality for error handling.To ensure all instances of
sdkerrorshave been updated, please run the following command:If this command returns any results, those instances should be updated to use the new
errorspackage.module/x/gravity/types/genesis.go (5)
107-107: LGTM: Updated error wrapping in ValidateBasicThe error wrapping has been correctly updated to use
errors.Wrap, consistent with the new import. The functionality remains unchanged.
112-112: LGTM: Updated error wrapping for delegates validationThe error wrapping for delegate key validation has been correctly updated to use
errors.Wrap, consistent with the new import and previous changes.
153-201: LGTM: Comprehensive update of error wrapping in Params.ValidateBasicAll error wrapping calls in the
ValidateBasicmethod ofParamshave been systematically updated to useerrors.Wrap. This comprehensive change ensures consistency with the new error handling approach throughout the method.
Line range hint
1-420: Summary: Consistent update to new error handling packageThe changes in this file represent a systematic update from
sdkerrorstocosmossdk.io/errorsfor error handling. This update is consistent throughout the file and doesn't introduce any functional changes. It appears to be part of a larger effort to modernize the codebase.To ensure this change is applied consistently across the entire codebase, run the following command:
#!/bin/bash # Check for any remaining uses of sdkerrors in the go files rg --type go 'sdkerrors' # Check for any inconsistent use of errors.Wrap rg --type go 'Wrap\(' | grep -v 'errors\.Wrap'These commands will help identify any places where the old error handling might still be in use or where the new error handling is not consistently applied.
8-8: LGTM: Updated error handling importThe change from
sdkerrorstocosmossdk.io/errorsis consistent with modern practices in the Cosmos SDK ecosystem. This update will affect error wrapping throughout the file.To ensure this change is consistent across the codebase, run the following command:
module/x/gravity/keeper/msg_server_test.go (2)
19-21: LGTM: New variable for testing non-existent orchestrator addressThe addition of
nonexistentOrcAddris a good practice for testing error scenarios with invalid addresses.
282-313: LGTM: Error handling tests for CancelSendToEthereumThe new test cases for
TestMsgServer_CancelSendToEthereumimprove the test coverage by checking for invalid IDs and unauthorized cancellation attempts. These scenarios are crucial for ensuring the security and correctness of the cancellation process.module/x/gravity/keeper/grpc_query.go (7)
6-6: LGTM: Updated error handling importThe change to import
errorsfromcosmossdk.iois a good update, aligning with the latest practices in the Cosmos SDK for error handling.
108-108: LGTM: Updated error wrappingThe change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the new error handling approach. The functionality remains unchanged.
126-126: LGTM: Updated error wrappingThe change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the new error handling approach. The functionality remains unchanged.
143-143: LGTM: Updated error wrappingThe change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the new error handling approach. The functionality remains unchanged.
284-286: LGTM: Updated error wrappingThe changes from
sdkerrors.Wrapftoerrors.Wrapfare consistent with the new error handling approach. The functionality remains unchanged, and the multi-line formatting for readability is maintained.Also applies to: 309-312
397-397: LGTM: Updated error wrappingThe change from
sdkerrors.Wrapftoerrors.Wrapfis consistent with the new error handling approach. The functionality remains unchanged.
Line range hint
1-624: Overall: Consistent update to error handlingThe changes in this file consistently update the error handling approach from
sdkerrorstoerrorsfromcosmossdk.io. This update aligns the code with the latest Cosmos SDK practices. No functional changes were made, and the updates were applied consistently throughout the file. The changes improve code maintainability and consistency with the broader Cosmos SDK ecosystem.integration_tests/setup_test.go (1)
Line range hint
1-1000: Ensure integration tests validity with new ConfirmedOutgoingTxWindow valueWhile this is the only change in the file, the significant reduction in the
ConfirmedOutgoingTxWindowparameter could potentially affect the behavior of some integration tests. To maintain the integrity of our test suite:
- Please review all integration tests that might be sensitive to the
ConfirmedOutgoingTxWindowparameter.- Consider adding new tests specifically designed to verify the system's behavior with this new, lower value.
- Update any documentation or comments that might reference the old value.
Let's search for tests that might be affected by this change:
✅ Verification successful
ConfirmedOutgoingTxWindow Parameter Change Does Not Impact Existing Tests
After reviewing the codebase, the
ConfirmedOutgoingTxWindowparameter is utilized in the setup configuration withinintegration_tests/setup_test.goand is referenced in various module files. There are no direct dependencies or references within individual test functions that would be affected by this change.Therefore, the modification does not adversely impact the current integration tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test functions that might be using or depending on ConfirmedOutgoingTxWindow rg "func.*Test.*" --type go | rg -C 10 "ConfirmedOutgoingTxWindow" # Look for any test setup or assertions that use timing or block numbers # which might be affected by the changed window rg "func.*Test.*" --type go | rg -C 10 "time\.|block.*height|block.*number"Length of output: 144
Script:
#!/bin/bash # Search for all occurrences of ConfirmedOutgoingTxWindow in the codebase rg "ConfirmedOutgoingTxWindow" --type go # Search for configuration files or setup scripts that reference ConfirmedOutgoingTxWindow rg "ConfirmedOutgoingTxWindow" --type toml --type yaml --type json # Search for documentation or comments explaining the role of ConfirmedOutgoingTxWindow rg "ConfirmedOutgoingTxWindow" --type md --type go | rg -C 5 ".*"Length of output: 5415
| @@ -3,8 +3,8 @@ package keeper | |||
| import ( | |||
| "sort" | |||
|
|
|||
| "cosmossdk.io/errors" | |||
There was a problem hiding this comment.
💡 Codebase verification
Incomplete replacement of sdkerrors detected.
Multiple instances of sdkerrors are still present in the codebase. Please ensure that all usages of sdkerrors are replaced with errors from cosmossdk.io to maintain consistency in error handling.
🔗 Analysis chain
Approve import change and verify its impact.
The replacement of sdkerrors with errors from cosmossdk.io is consistent with modern practices. This change should standardize error handling across the codebase.
To ensure this change doesn't introduce any unintended consequences, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of replacing sdkerrors with errors from cosmossdk.io
# Test 1: Check if sdkerrors is still used elsewhere in the codebase
echo "Checking for remaining uses of sdkerrors:"
rg --type go 'sdkerrors'
# Test 2: Verify that all uses of errors.Wrapf are from cosmossdk.io/errors
echo "Verifying uses of errors.Wrapf:"
rg --type go 'errors\.Wrapf'
Length of output: 9135
| @@ -3,7 +3,7 @@ package types | |||
| import ( | |||
| "fmt" | |||
|
|
|||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |||
| "cosmossdk.io/errors" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent import statements detected
The change to cosmossdk.io/errors was made in module/x/gravity/types/ethereum_signature.go; however, multiple other files still import github.com/cosmos/cosmos-sdk/types/errors. Please ensure all imports are consistent across the codebase.
module/x/gravity/handler.gomodule/app/app.gomodule/x/gravity/types/ethereum_event.gomodule/x/gravity/types/msgs.gomodule/x/gravity/types/codec.go
🔗 Analysis chain
Approve import change, verify upgrade process
The change from sdkerrors to cosmossdk.io/errors is approved. This update likely reflects a migration to a newer version of the Cosmos SDK or its components.
To ensure this change is part of a broader upgrade process, please run the following script:
This will help verify if the change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of the old and new import statements
echo "Checking for remaining sdkerrors imports:"
rg --type go 'github\.com/cosmos/cosmos-sdk/types/errors'
echo "Checking for new cosmossdk.io/errors imports:"
rg --type go 'cosmossdk\.io/errors'
Length of output: 1897
| @@ -4,7 +4,7 @@ import ( | |||
| "math/big" | |||
| "strings" | |||
|
|
|||
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | |||
| "cosmossdk.io/errors" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of error packages detected
Several files still import and use the old sdkerrors package:
module/app/app.gomodule/x/gravity/handler.gomodule/x/gravity/types/ethereum_event.gomodule/x/gravity/types/codec.gomodule/x/gravity/types/msgs.go
Please update these files to use errors from cosmossdk.io/errors to maintain consistency across the codebase.
🔗 Analysis chain
Verify consistency of error package import across the codebase
The import statement has been updated from sdkerrors to errors from the cosmossdk.io package. This change reflects an update in the Cosmos SDK's error handling.
To ensure consistency across the codebase, run the following script:
This will help identify any files that may still be using the old import or the sdkerrors package, ensuring a consistent upgrade across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old sdkerrors import
# Test 1: Search for any remaining imports of sdkerrors
echo "Checking for remaining sdkerrors imports:"
rg --type go 'sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"'
# Test 2: Search for any uses of sdkerrors package
echo "Checking for uses of sdkerrors package:"
rg --type go '\bsdkerrors\.'
# If both tests pass (no output), the change has been consistently applied
Length of output: 3885
integration_tests/happy_path_test.go
Outdated
| s.Run("Test orchestrator EthereumTxConfirmation after transaction execution", func() { | ||
| val := s.chain.validators[0] | ||
| keyring, err := val.keyring() | ||
| s.Require().NoError(err) | ||
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | ||
| s.Require().NoError(err) | ||
|
|
||
| recipient := s.chain.validators[3].ethereumKey.address | ||
| gravityQueryClient := types.NewQueryClient(clientCtx) | ||
| gravityResponse, err := gravityQueryClient.DenomToERC20(context.Background(), | ||
| &types.DenomToERC20Request{ | ||
| Denom: testDenom, | ||
| }, | ||
| ) | ||
| s.Require().NoError(err, "error querying ERC20 for testgb denom") | ||
| s.Require().True(gravityResponse.CosmosOriginated) | ||
| testDenomERC20 := common.HexToAddress(gravityResponse.Erc20) | ||
|
|
||
| initialBalance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | ||
| s.Require().NoError(err, "error getting initial balance") | ||
| // Turn off one orchestrator (let's use the second one) | ||
| s.T().Log("Turning off orchestrator1") | ||
| err = s.dockerPool.RemoveContainerByName("orchestrator1") | ||
| s.Require().NoError(err, "Failed to remove orchestrator1 container") | ||
|
|
||
| // Prepare SendToEthereum message | ||
| s.T().Log("Preparing SendToEthereum message") | ||
| // Query the balance of testERC20Denom for validator0 | ||
| bankQueryClient := banktypes.NewQueryClient(clientCtx) | ||
| balanceRes, err := bankQueryClient.AllBalances( | ||
| context.Background(), | ||
| &banktypes.QueryAllBalancesRequest{ | ||
| Address: val.address().String(), | ||
| }, | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.T().Logf("Validator0 balances: %s", balanceRes.Balances) | ||
| sendAmount := sdk.NewInt(420) | ||
| feeAmount := sdk.NewInt(1) | ||
| sendToEthereumMsg := types.NewMsgSendToEthereum( | ||
| s.chain.validators[0].address(), | ||
| recipient, | ||
| sdk.NewCoin(testDenom, sendAmount), | ||
| sdk.NewCoin(testDenom, feeAmount), | ||
| ) | ||
| s.T().Logf("Sent %s %s to %s", sendAmount, testDenom, recipient) | ||
|
|
||
| // Send the message | ||
| s.T().Logf("Sending SendToEthereum message with %s", val.address().String()) | ||
| response, err := s.chain.sendMsgs(*clientCtx, sendToEthereumMsg) | ||
| s.Require().NoError(err) | ||
| s.Require().Zero(response.Code, "SendToEthereum failed: %s", response.RawLog) | ||
|
|
||
| // Wait for the transaction to complete on Ethereum | ||
| s.T().Log("Waiting for transaction to complete on Ethereum") | ||
| s.T().Logf("Recipient: %s, initial balance: %s", recipient, initialBalance.String()) | ||
| s.Require().Eventually(func() bool { | ||
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | ||
| if err != nil { | ||
| s.T().Logf("Error getting balance: %v", err) | ||
| return false | ||
| } | ||
| s.T().Logf("Balance: %s", balance.String()) | ||
| expectedBalance := initialBalance.Add(sendAmount) | ||
| return balance.Equal(expectedBalance) | ||
| }, 5*time.Minute, 10*time.Second, "Transaction did not complete on Ethereum") | ||
|
|
||
| // Turn the orchestrator back on | ||
| s.T().Log("Turning orchestrator1 back on") | ||
| err = s.startOrchestrator1() | ||
| s.Require().NoError(err, "Failed to restart orchestrator1") | ||
|
|
||
| // Watch for EthereumTxConfirmation from the restarted orchestrator | ||
| s.T().Log("Watching for EthereumTxConfirmation from restarted orchestrator") | ||
| s.Require().Eventually(func() bool { | ||
| val := s.chain.validators[1] | ||
| keyring, err := val.keyring() | ||
| s.Require().NoError(err) | ||
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Get the validator address with cosmosvaloper prefix | ||
| cosmosValOperAddr, err := getValOperatorAddress(val.address().String()) | ||
| s.Require().NoError(err) | ||
| queryClient := types.NewQueryClient(clientCtx) | ||
| res, err := queryClient.BatchTxConfirmationsByValidator(context.Background(), &types.BatchTxConfirmationsByValidatorRequest{ | ||
| ValidatorAddress: cosmosValOperAddr, | ||
| }) | ||
| if err != nil { | ||
| s.T().Logf("Error querying EthereumTxConfirmation: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| // Check if the height has increased, indicating that the orchestrator has caught up | ||
| return len(res.BatchTxConfirmations) > 0 | ||
| }, 5*time.Minute, 10*time.Second, "Orchestrator did not submit EthereumTxConfirmation after restart") | ||
|
|
||
| s.T().Log("Orchestrator successfully submitted EthereumTxConfirmation after transaction execution") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test readability and robustness
The new test function "Test orchestrator EthereumTxConfirmation after transaction execution" is well-structured and covers important scenarios. However, consider the following improvements:
-
Add comments to explain the purpose of each major step in the test. This will make the test more readable and maintainable.
-
Handle potential errors in the
getEthTokenBalanceOfcall at line 312. Currently, the error is ignored, which could lead to unexpected behavior. -
Consider using constants for the timeout durations used in
Eventuallycalls (e.g., 5time.Minute, 10time.Second). This will make it easier to adjust these values consistently across the test suite.
Here's an example of how you could improve the error handling for getEthTokenBalanceOf:
-initialBalance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20)
+initialBalance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20)
+s.Require().NoError(err, "Failed to get initial balance")Consider applying similar error handling throughout the test function.
📝 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.
| s.Run("Test orchestrator EthereumTxConfirmation after transaction execution", func() { | |
| val := s.chain.validators[0] | |
| keyring, err := val.keyring() | |
| s.Require().NoError(err) | |
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | |
| s.Require().NoError(err) | |
| recipient := s.chain.validators[3].ethereumKey.address | |
| gravityQueryClient := types.NewQueryClient(clientCtx) | |
| gravityResponse, err := gravityQueryClient.DenomToERC20(context.Background(), | |
| &types.DenomToERC20Request{ | |
| Denom: testDenom, | |
| }, | |
| ) | |
| s.Require().NoError(err, "error querying ERC20 for testgb denom") | |
| s.Require().True(gravityResponse.CosmosOriginated) | |
| testDenomERC20 := common.HexToAddress(gravityResponse.Erc20) | |
| initialBalance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | |
| s.Require().NoError(err, "error getting initial balance") | |
| // Turn off one orchestrator (let's use the second one) | |
| s.T().Log("Turning off orchestrator1") | |
| err = s.dockerPool.RemoveContainerByName("orchestrator1") | |
| s.Require().NoError(err, "Failed to remove orchestrator1 container") | |
| // Prepare SendToEthereum message | |
| s.T().Log("Preparing SendToEthereum message") | |
| // Query the balance of testERC20Denom for validator0 | |
| bankQueryClient := banktypes.NewQueryClient(clientCtx) | |
| balanceRes, err := bankQueryClient.AllBalances( | |
| context.Background(), | |
| &banktypes.QueryAllBalancesRequest{ | |
| Address: val.address().String(), | |
| }, | |
| ) | |
| s.Require().NoError(err) | |
| s.T().Logf("Validator0 balances: %s", balanceRes.Balances) | |
| sendAmount := sdk.NewInt(420) | |
| feeAmount := sdk.NewInt(1) | |
| sendToEthereumMsg := types.NewMsgSendToEthereum( | |
| s.chain.validators[0].address(), | |
| recipient, | |
| sdk.NewCoin(testDenom, sendAmount), | |
| sdk.NewCoin(testDenom, feeAmount), | |
| ) | |
| s.T().Logf("Sent %s %s to %s", sendAmount, testDenom, recipient) | |
| // Send the message | |
| s.T().Logf("Sending SendToEthereum message with %s", val.address().String()) | |
| response, err := s.chain.sendMsgs(*clientCtx, sendToEthereumMsg) | |
| s.Require().NoError(err) | |
| s.Require().Zero(response.Code, "SendToEthereum failed: %s", response.RawLog) | |
| // Wait for the transaction to complete on Ethereum | |
| s.T().Log("Waiting for transaction to complete on Ethereum") | |
| s.T().Logf("Recipient: %s, initial balance: %s", recipient, initialBalance.String()) | |
| s.Require().Eventually(func() bool { | |
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | |
| if err != nil { | |
| s.T().Logf("Error getting balance: %v", err) | |
| return false | |
| } | |
| s.T().Logf("Balance: %s", balance.String()) | |
| expectedBalance := initialBalance.Add(sendAmount) | |
| return balance.Equal(expectedBalance) | |
| }, 5*time.Minute, 10*time.Second, "Transaction did not complete on Ethereum") | |
| // Turn the orchestrator back on | |
| s.T().Log("Turning orchestrator1 back on") | |
| err = s.startOrchestrator1() | |
| s.Require().NoError(err, "Failed to restart orchestrator1") | |
| // Watch for EthereumTxConfirmation from the restarted orchestrator | |
| s.T().Log("Watching for EthereumTxConfirmation from restarted orchestrator") | |
| s.Require().Eventually(func() bool { | |
| val := s.chain.validators[1] | |
| keyring, err := val.keyring() | |
| s.Require().NoError(err) | |
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | |
| s.Require().NoError(err) | |
| // Get the validator address with cosmosvaloper prefix | |
| cosmosValOperAddr, err := getValOperatorAddress(val.address().String()) | |
| s.Require().NoError(err) | |
| queryClient := types.NewQueryClient(clientCtx) | |
| res, err := queryClient.BatchTxConfirmationsByValidator(context.Background(), &types.BatchTxConfirmationsByValidatorRequest{ | |
| ValidatorAddress: cosmosValOperAddr, | |
| }) | |
| if err != nil { | |
| s.T().Logf("Error querying EthereumTxConfirmation: %v", err) | |
| return false | |
| } | |
| // Check if the height has increased, indicating that the orchestrator has caught up | |
| return len(res.BatchTxConfirmations) > 0 | |
| }, 5*time.Minute, 10*time.Second, "Orchestrator did not submit EthereumTxConfirmation after restart") | |
| s.T().Log("Orchestrator successfully submitted EthereumTxConfirmation after transaction execution") | |
| }) | |
| s.Run("Test orchestrator EthereumTxConfirmation after transaction execution", func() { | |
| val := s.chain.validators[0] | |
| keyring, err := val.keyring() | |
| s.Require().NoError(err) | |
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | |
| s.Require().NoError(err) | |
| recipient := s.chain.validators[3].ethereumKey.address | |
| gravityQueryClient := types.NewQueryClient(clientCtx) | |
| gravityResponse, err := gravityQueryClient.DenomToERC20(context.Background(), | |
| &types.DenomToERC20Request{ | |
| Denom: testDenom, | |
| }, | |
| ) | |
| s.Require().NoError(err, "error querying ERC20 for testgb denom") | |
| s.Require().True(gravityResponse.CosmosOriginated) | |
| testDenomERC20 := common.HexToAddress(gravityResponse.Erc20) | |
| initialBalance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | |
| s.Require().NoError(err, "Failed to get initial balance") | |
| // Turn off one orchestrator (let's use the second one) | |
| s.T().Log("Turning off orchestrator1") | |
| err = s.dockerPool.RemoveContainerByName("orchestrator1") | |
| s.Require().NoError(err, "Failed to remove orchestrator1 container") | |
| // Prepare SendToEthereum message | |
| s.T().Log("Preparing SendToEthereum message") | |
| // Query the balance of testERC20Denom for validator0 | |
| bankQueryClient := banktypes.NewQueryClient(clientCtx) | |
| balanceRes, err := bankQueryClient.AllBalances( | |
| context.Background(), | |
| &banktypes.QueryAllBalancesRequest{ | |
| Address: val.address().String(), | |
| }, | |
| ) | |
| s.Require().NoError(err) | |
| s.T().Logf("Validator0 balances: %s", balanceRes.Balances) | |
| sendAmount := sdk.NewInt(420) | |
| feeAmount := sdk.NewInt(1) | |
| sendToEthereumMsg := types.NewMsgSendToEthereum( | |
| s.chain.validators[0].address(), | |
| recipient, | |
| sdk.NewCoin(testDenom, sendAmount), | |
| sdk.NewCoin(testDenom, feeAmount), | |
| ) | |
| s.T().Logf("Sent %s %s to %s", sendAmount, testDenom, recipient) | |
| // Send the message | |
| s.T().Logf("Sending SendToEthereum message with %s", val.address().String()) | |
| response, err := s.chain.sendMsgs(*clientCtx, sendToEthereumMsg) | |
| s.Require().NoError(err) | |
| s.Require().Zero(response.Code, "SendToEthereum failed: %s", response.RawLog) | |
| // Wait for the transaction to complete on Ethereum | |
| s.T().Log("Waiting for transaction to complete on Ethereum") | |
| s.T().Logf("Recipient: %s, initial balance: %s", recipient, initialBalance.String()) | |
| s.Require().Eventually(func() bool { | |
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | |
| if err != nil { | |
| s.T().Logf("Error getting balance: %v", err) | |
| return false | |
| } | |
| s.T().Logf("Balance: %s", balance.String()) | |
| expectedBalance := initialBalance.Add(sendAmount) | |
| return balance.Equal(expectedBalance) | |
| }, 5*time.Minute, 10*time.Second, "Transaction did not complete on Ethereum") | |
| // Turn the orchestrator back on | |
| s.T().Log("Turning orchestrator1 back on") | |
| err = s.startOrchestrator1() | |
| s.Require().NoError(err, "Failed to restart orchestrator1") | |
| // Watch for EthereumTxConfirmation from the restarted orchestrator | |
| s.T().Log("Watching for EthereumTxConfirmation from restarted orchestrator") | |
| s.Require().Eventually(func() bool { | |
| val := s.chain.validators[1] | |
| keyring, err := val.keyring() | |
| s.Require().NoError(err) | |
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | |
| s.Require().NoError(err) | |
| // Get the validator address with cosmosvaloper prefix | |
| cosmosValOperAddr, err := getValOperatorAddress(val.address().String()) | |
| s.Require().NoError(err) | |
| queryClient := types.NewQueryClient(clientCtx) | |
| res, err := queryClient.BatchTxConfirmationsByValidator(context.Background(), &types.BatchTxConfirmationsByValidatorRequest{ | |
| ValidatorAddress: cosmosValOperAddr, | |
| }) | |
| if err != nil { | |
| s.T().Logf("Error querying EthereumTxConfirmation: %v", err) | |
| return false | |
| } | |
| // Check if the height has increased, indicating that the orchestrator has caught up | |
| return len(res.BatchTxConfirmations) > 0 | |
| }, 5*time.Minute, 10*time.Second, "Orchestrator did not submit EthereumTxConfirmation after restart") | |
| s.T().Log("Orchestrator successfully submitted EthereumTxConfirmation after transaction execution") | |
| }) |
integration_tests/setup_test.go
Outdated
| @@ -346,7 +346,7 @@ func (s *IntegrationTestSuite) initGenesis() { | |||
| s.Require().NoError(cdc.UnmarshalJSON(appGenState[gravitytypes.ModuleName], &gravityGenState)) | |||
| gravityGenState.Params.GravityId = "gravitytest" | |||
| gravityGenState.Params.BridgeEthereumAddress = gravityContract.String() | |||
| gravityGenState.Params.ConfirmedOutgoingTxWindow = 1000000 | |||
| gravityGenState.Params.ConfirmedOutgoingTxWindow = 300 | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of ConfirmedOutgoingTxWindow across the codebase
The ConfirmedOutgoingTxWindow parameter has been changed from 1000000 to 300 in integration_tests/setup_test.go. However, numerous other instances still reference the old value or rely on this parameter:
-
Module References:
module/x/gravity/keeper/test_common.gomodule/x/gravity/types/genesis.gomodule/x/gravity/types/genesis.pb.go
-
Hardcoded Values:
module/cmd/gravity/cmd/gentx.gouses1000000stake.- Various test files reference
1000000and1000000000in staking and gas limit contexts.
Recommendations:
-
Update All References:
- Ensure that all occurrences of
ConfirmedOutgoingTxWindoware updated to the new value of300to maintain consistency.
- Ensure that all occurrences of
-
Adjust Hardcoded Values:
- Modify hardcoded instances of
1000000related toConfirmedOutgoingTxWindowto align with the new parameter value. - Review and update related staking amounts and gas limits if they are directly influenced by this change.
- Modify hardcoded instances of
-
Comprehensive Testing:
- Run the full suite of integration and unit tests to identify and address any issues arising from this parameter change.
- Validate the overall system behavior to ensure the Gravity Bridge functions as expected with the updated window.
🔗 Analysis chain
Significant reduction in ConfirmedOutgoingTxWindow parameter
The ConfirmedOutgoingTxWindow parameter has been reduced from 1000000 to 300. This is a substantial change that could significantly impact the behavior of the Gravity Bridge, particularly in how it handles outgoing transactions.
- Could you please provide the rationale behind this change?
- Have you considered the potential implications on the integration tests and the overall system behavior?
- Are there any other parts of the codebase that might need adjustment due to this change?
To ensure this change doesn't have unintended consequences, let's check for any other occurrences or dependencies on this parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of ConfirmedOutgoingTxWindow
rg "ConfirmedOutgoingTxWindow" --type go
# Check for any hardcoded values close to the old value (1000000) that might need updating
rg "1000000" --type go
Length of output: 4178
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- integration_tests/happy_path_test.go (2 hunks)
- integration_tests/setup_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/setup_test.go
🧰 Additional context used
| Address: val.address().String(), | ||
| }, | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.T().Logf("Validator0 balances: %s", balanceRes.Balances) | ||
| sendAmount := sdk.NewInt(420) | ||
| feeAmount := sdk.NewInt(1) | ||
| sendToEthereumMsg := types.NewMsgSendToEthereum( | ||
| s.chain.validators[0].address(), | ||
| recipient, | ||
| sdk.NewCoin(testDenom, sendAmount), | ||
| sdk.NewCoin(testDenom, feeAmount), | ||
| ) | ||
| s.T().Logf("Sent %s %s to %s", sendAmount, testDenom, recipient) | ||
|
|
||
| // Getting latest Batch Nonce | ||
| completed, err := gravityQueryClient.CompletedBatchTxs(context.Background(), &types.CompletedBatchTxsRequest{}) | ||
| s.Require().NoError(err, "error querying CompletedBatchTxs") | ||
| // Search through completed batch txs to get the highest nonce | ||
| var highestNonce uint64 | ||
| for _, batchTx := range completed.CompletedBatchTxs { | ||
| if batchTx.BatchNonce > highestNonce { | ||
| highestNonce = batchTx.BatchNonce | ||
| } | ||
| } | ||
| s.T().Logf("Highest completed batch nonce: %d", highestNonce) | ||
|
|
||
| // Send the message | ||
| s.T().Logf("Sending SendToEthereum message with %s", val.address().String()) | ||
| response, err := s.chain.sendMsgs(*clientCtx, sendToEthereumMsg) | ||
| s.Require().NoError(err) | ||
| s.Require().Zero(response.Code, "SendToEthereum failed: %s", response.RawLog) | ||
|
|
||
| // Wait for the transaction to complete on Ethereum | ||
| s.T().Log("Waiting for transaction to complete on Ethereum") | ||
| s.T().Logf("Recipient: %s, initial balance: %s", recipient, initialBalance.String()) | ||
| s.Require().Eventually(func() bool { | ||
| balance, err := s.getEthTokenBalanceOf(common.HexToAddress(recipient), testDenomERC20) | ||
| if err != nil { | ||
| s.T().Logf("Error getting balance: %v", err) | ||
| return false | ||
| } | ||
| s.T().Logf("Balance: %s", balance.String()) | ||
| expectedBalance := initialBalance.Add(sendAmount) | ||
| return balance.Equal(expectedBalance) | ||
| }, 5*time.Minute, 10*time.Second, "Transaction did not complete on Ethereum") | ||
|
|
||
| // Wait for the CompletedOutgoingTx to be created | ||
| s.T().Log("Waiting for CompletedOutgoingTx to be created") | ||
| expectedNonce := highestNonce + 1 | ||
| s.T().Logf("Expected nonce: %d", expectedNonce) | ||
| s.Require().Eventually(func() bool { | ||
| res, err := gravityQueryClient.CompletedBatchTxs(context.Background(), &types.CompletedBatchTxsRequest{}) | ||
| if err != nil { | ||
| s.T().Logf("Error querying CompletedBatchTxs: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| for _, batchTx := range res.CompletedBatchTxs { | ||
| if batchTx.BatchNonce >= expectedNonce { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| }, 5*time.Minute, 3*time.Second, "CompletedBatchTx was not found") | ||
|
|
||
| // Turn the orchestrator back on | ||
| s.T().Log("Turning orchestrator1 back on") | ||
| err = s.startOrchestrator1() | ||
| s.Require().NoError(err, "Failed to restart orchestrator1") | ||
|
|
||
| // Watch for EthereumTxConfirmation from the restarted orchestrator | ||
| s.T().Log("Watching for EthereumTxConfirmation from restarted orchestrator") | ||
| s.Require().Eventually(func() bool { | ||
| val := s.chain.validators[1] | ||
| keyring, err := val.keyring() | ||
| s.Require().NoError(err) | ||
| clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &keyring, "val", val.address()) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Get the validator address with cosmosvaloper prefix | ||
| cosmosValOperAddr, err := getValOperatorAddress(val.address().String()) | ||
| s.Require().NoError(err) | ||
| queryClient := types.NewQueryClient(clientCtx) | ||
| res, err := queryClient.BatchTxConfirmationsByValidator(context.Background(), &types.BatchTxConfirmationsByValidatorRequest{ | ||
| ValidatorAddress: cosmosValOperAddr, | ||
| }) | ||
| if err != nil { | ||
| s.T().Logf("Error querying EthereumTxConfirmation: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| // Check if the height has increased, indicating that the orchestrator has caught up | ||
| return len(res.BatchTxConfirmations) > 0 | ||
| }, 5*time.Minute, 10*time.Second, "Orchestrator did not submit EthereumTxConfirmation after restart") | ||
|
|
||
| s.T().Log("Orchestrator successfully submitted EthereumTxConfirmation after transaction execution") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the test case for improved readability and maintainability
The new test case "Test orchestrator EthereumTxConfirmation after transaction execution" is comprehensive and well-structured. However, there are a few suggestions for improvement:
-
Consider splitting this long test into smaller sub-tests using
s.Run(). This will improve readability and make it easier to identify which part of the test failed. -
Some magic numbers are used (e.g.,
420forsendAmount,5*time.Minutefor timeouts). Consider defining these as constants at the top of the test or the file for better maintainability. -
Error handling could be more consistent. For example, some errors are checked with
s.Require().NoError(err), while others are logged and ignored.
Here's an example of how you could start refactoring this test:
func (s *IntegrationTestSuite) TestOrchestratorEthereumTxConfirmation() {
const (
sendAmount = 420
feeAmount = 1
timeout = 5 * time.Minute
interval = 10 * time.Second
)
s.Run("Setup", func() {
// Setup code here
})
s.Run("Send transaction", func() {
// Transaction sending code here
})
s.Run("Verify transaction completion", func() {
// Transaction verification code here
})
s.Run("Restart orchestrator", func() {
// Orchestrator restart and verification code here
})
}This structure makes the test more modular and easier to maintain.
| func (s *IntegrationTestSuite) startOrchestrator1() error { | ||
| i := 1 | ||
| orch := s.chain.orchestrators[i] | ||
| gorcCfg := fmt.Sprintf(`keystore = "/root/gorc/keystore/" | ||
|
|
||
| [gravity] | ||
| contract = "%s" | ||
| fees_denom = "%s" | ||
|
|
||
| [ethereum] | ||
| key_derivation_path = "m/44'/60'/0'/0/0" | ||
| rpc = "http://%s:8545" | ||
|
|
||
| [cosmos] | ||
| key_derivation_path = "m/44'/118'/1'/0/0" | ||
| grpc = "http://%s:9090" | ||
| gas_price = { amount = %s, denom = "%s" } | ||
| prefix = "cosmos" | ||
| gas_adjustment = 2.0 | ||
| msg_batch_size = 5 | ||
| `, | ||
| gravityContract.String(), | ||
| testDenom, | ||
| // NOTE: container names are prefixed with '/' | ||
| s.ethResource.Container.Name[1:], | ||
| s.valResources[i].Container.Name[1:], | ||
| minGasPrice, | ||
| testDenom, | ||
| ) | ||
|
|
||
| val := s.chain.validators[i] | ||
|
|
||
| gorcCfgPath := path.Join(val.configDir(), "gorc") | ||
| err := os.MkdirAll(gorcCfgPath, 0755) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| filePath := path.Join(gorcCfgPath, "config.toml") | ||
| err = writeFile(filePath, []byte(gorcCfg)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // We must first populate the orchestrator's keystore prior to starting | ||
| // the orchestrator gorc process. The keystore must contain the Ethereum | ||
| // and orchestrator keys. These keys will be used for relaying txs to | ||
| // and from the test network and Ethereum. The gorc_bootstrap.sh scripts encapsulates | ||
| // this entire process. | ||
| // | ||
| // NOTE: If the Docker build changes, the script might have to be modified | ||
| // as it relies on busybox. | ||
| err = copyFile( | ||
| filepath.Join("integration_tests", "gorc_bootstrap.sh"), | ||
| filepath.Join(gorcCfgPath, "gorc_bootstrap.sh"), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| resource, err := s.dockerPool.RunWithOptions( | ||
| &dockertest.RunOptions{ | ||
| Name: orch.instanceName(), | ||
| NetworkID: s.dockerNetwork.Network.ID, | ||
| Repository: "orchestrator", | ||
| Tag: "prebuilt", | ||
| Mounts: []string{ | ||
| fmt.Sprintf("%s/:/root/gorc", gorcCfgPath), | ||
| }, | ||
| Env: []string{ | ||
| fmt.Sprintf("ORCH_MNEMONIC=%s", orch.mnemonic), | ||
| fmt.Sprintf("ETH_PRIV_KEY=%s", val.ethereumKey.privateKey), | ||
| "RUST_BACKTRACE=full", | ||
| "RUST_LOG=debug", | ||
| }, | ||
| Entrypoint: []string{ | ||
| "sh", | ||
| "-c", | ||
| "chmod +x /root/gorc/gorc_bootstrap.sh && /root/gorc/gorc_bootstrap.sh", | ||
| }, | ||
| }, | ||
| noRestart, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| s.orchResources[i] = resource | ||
| s.T().Logf("started orchestrator container: %s", resource.Container.ID) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the startOrchestrator1 function for better flexibility and error handling
The startOrchestrator1 function is well-implemented, but there are several areas for improvement:
-
The function name suggests it's specific to orchestrator1, but it uses an index. Consider renaming it to
startOrchestratorand passing the index as a parameter. -
Some values are hardcoded (e.g., key derivation paths, gas adjustment). Consider making these configurable or constants.
-
Error handling could be more consistent and informative.
Here's a suggested refactoring:
func (s *IntegrationTestSuite) startOrchestrator(index int) error {
const (
ethereumKeyDerivationPath = "m/44'/60'/0'/0/0"
cosmosKeyDerivationPath = "m/44'/118'/1'/0/0"
gasAdjustment = 2.0
msgBatchSize = 5
)
if index < 0 || index >= len(s.chain.orchestrators) {
return fmt.Errorf("invalid orchestrator index: %d", index)
}
orch := s.chain.orchestrators[index]
val := s.chain.validators[index]
gorcCfg := fmt.Sprintf(`keystore = "/root/gorc/keystore/"
[gravity]
contract = "%s"
fees_denom = "%s"
[ethereum]
key_derivation_path = "%s"
rpc = "http://%s:8545"
[cosmos]
key_derivation_path = "%s"
grpc = "http://%s:9090"
gas_price = { amount = %s, denom = "%s" }
prefix = "cosmos"
gas_adjustment = %f
msg_batch_size = %d
`,
gravityContract.String(),
testDenom,
ethereumKeyDerivationPath,
s.ethResource.Container.Name[1:],
cosmosKeyDerivationPath,
s.valResources[index].Container.Name[1:],
minGasPrice,
testDenom,
gasAdjustment,
msgBatchSize,
)
// ... (rest of the function)
return nil
}This refactoring improves flexibility, readability, and maintainability of the function.
| func getValOperatorAddress(address string) (string, error) { | ||
| // Decode the old address | ||
| _, bz, err := bech32.DecodeAndConvert(address) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to decode address: %w", err) | ||
| } | ||
|
|
||
| // Encode with the new prefix | ||
| newAddress, err := bech32.ConvertAndEncode("cosmosvaloper", bz) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to encode address: %w", err) | ||
| } | ||
|
|
||
| return newAddress, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying getValOperatorAddress function
The getValOperatorAddress function is concise and focused. However, it can be simplified and improved:
- The function can be simplified by using the
sdk.ValAddresstype conversion. - Error messages could be more descriptive to aid in debugging.
Consider replacing the current implementation with the following:
func getValOperatorAddress(address string) (string, error) {
accAddr, err := sdk.AccAddressFromBech32(address)
if err != nil {
return "", fmt.Errorf("failed to decode address %s: %w", address, err)
}
valAddr := sdk.ValAddress(accAddr)
return valAddr.String(), nil
}This simplification removes the need for manual bech32 encoding and decoding, making the code more robust and easier to maintain. It also provides a more descriptive error message.
Then, update the function call at line 409:
-cosmosValOperAddr, err := getValOperatorAddress(val.address().String())
+cosmosValOperAddr := sdk.ValAddress(val.address()).String()This change simplifies the code and removes the need for error handling at the call site.
Summary by CodeRabbit
Documentation
Chores
New Features
Bug Fixes