feat: track latest 256 cosmos block hashes #76
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces changes to the handling of block hash tracking in the Cosmos SDK's EVM module. The Changes
Possibly related PRs
Poem
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: 0
🧹 Outside diff range and nitpick comments (2)
x/evm/abci.go (2)
13-14: Enhance function documentation for clarity.While the current comment provides a basic description, it could be more informative. Consider expanding the documentation to include:
- The purpose of tracking the latest 256 block hashes
- How this relates to the EVM module's functionality
- Any important side effects or state changes
Example:
// EndBlocker tracks the latest 256 block hashes for the EVM module. // This function is called at the end of each block to update the block hash history, // which is crucial for certain EVM operations and maintaining chain state consistency. // It returns an error if the tracking operation fails.
15-17: LGTM: Implementation is sound, with a suggestion for improvement.The function correctly implements the block hash tracking with appropriate type conversions and telemetry. However, consider adding some error logging before returning the error:
func EndBlocker(ctx sdk.Context, k *keeper.Keeper) error { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker) err := k.TrackBlockHash(ctx, uint64(ctx.BlockHeight()), common.BytesToHash(ctx.HeaderHash())) if err != nil { ctx.Logger().Error("failed to track block hash", "error", err) } return err }This will provide more context in case of failures, aiding in debugging and monitoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/abci.go (1 hunks)
- x/evm/abci.go (1 hunks)
- x/evm/module.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
x/evm/abci.go (2)
3-11: LGTM: Imports are appropriate and well-organized.The imports are relevant to the function's implementation, with no unused imports. The use of aliases follows Go conventions.
1-17: Overall assessment: Good implementation with minor suggestions for improvement.This new
EndBlockerfunction successfully addresses the PR objective of tracking cosmos block hashes. It aligns with the Cosmos SDK module patterns and provides a solution to the limitation mentioned in cosmos-sdk issue #22246.Key points:
- The function correctly integrates with the Cosmos SDK context and keeper.
- It uses telemetry for performance monitoring.
- Proper type conversions are used for compatibility with Ethereum types.
Suggestions for improvement:
- Enhance the function documentation for better clarity.
- Add error logging for improved debugging and monitoring.
These changes provide a solid foundation for tracking the latest 256 cosmos block hashes, which is crucial for maintaining consistency between the Cosmos SDK and EVM module.
indexer/abci.go (1)
204-210: Alignment with PR objectives and follow-up tasksThe changes align with the PR objectives to address the cosmosSDK limitation and track cosmos block hashes instead of EVM block hashes. However, there are a few points to consider:
The TODO comment could be more specific about the planned implementation. Consider updating it to something like:
"TODO: Implement tracking of cosmos block hashes in the x/evm endblocker to address cosmosSDK limitation (issue #22246)."Ensure that the cosmos block hash tracking is properly implemented in the x/evm endblocker, as mentioned in the comment.
Consider adding a link to the relevant pull request or issue where the endblocker changes are being implemented for better traceability.
To verify the implementation of cosmos block hash tracking in the x/evm endblocker, please run the following script:
This script will help verify if the cosmos block hash tracking has been implemented in the x/evm endblocker and if there are any remaining TODO comments related to this task.
x/evm/module.go (2)
32-33: ImplementedHasEndBlockerInterfaceThe
AppModulestruct now correctly implements theappmodule.HasEndBlockerinterface, enabling end-of-block processing within the module.
161-167: AddedEndBlockMethod for End-of-Block LogicThe
EndBlockmethod is properly implemented to execute end-of-block logic. Unwrapping thesdk.Contextusingsdk.UnwrapSDKContext(ctx)ensures compatibility with the underlying SDK context required by theEndBlockerfunction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
x/evm/abci.go (3)
13-14: LGTM: Function declaration is appropriate, with a minor suggestion for the comment.The
PreBlockfunction declaration aligns well with the PR objective to track cosmos block hashes. The function signature and return types are consistent with Cosmos SDK conventions.Consider updating the comment to be more specific:
// PreBlock tracks the latest 256 cosmos block hashesThis change would more accurately reflect the PR's objective and the function's purpose.
15-17: LGTM: Implementation is concise and correct, with a suggestion for error handling.The
PreBlockfunction implementation effectively tracks the latest cosmos block hashes:
- Good use of telemetry for performance monitoring.
- Correct usage of the previous block height for tracking.
- Appropriate conversion of the header hash to a common hash format for Ethereum compatibility.
Consider adding explicit error handling:
func PreBlock(ctx sdk.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, error) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker) err := k.TrackBlockHash(ctx, uint64(ctx.BlockHeight()-1), common.BytesToHash(ctx.HeaderHash())) if err != nil { // Log the error or handle it as appropriate return sdk.ResponsePreBlock{}, fmt.Errorf("failed to track block hash: %w", err) } return sdk.ResponsePreBlock{}, nil }This change would provide more explicit error handling and potentially useful error information.
1-17: Summary: Implementation successfully addresses PR objectivesThis new file
x/evm/abci.goeffectively implements thePreBlockfunction to track the latest 256 cosmos block hashes, addressing the limitation mentioned in the PR objectives. The implementation is concise, correct, and focuses on the core functionality required.Key points:
- The function correctly tracks cosmos block hashes instead of EVM block hashes.
- It integrates well with the Cosmos SDK architecture by implementing the
PreBlockfunction.- The implementation uses appropriate Cosmos SDK and Ethereum types and conventions.
The changes in this file, combined with the modifications mentioned in other files (such as commenting out the block hash tracking in
indexer/abci.go), successfully achieve the goal of tracking cosmos block hashes in the EVM module.To further improve the robustness of this implementation, consider:
- Adding unit tests to verify the correct behavior of the
PreBlockfunction.- Documenting the rationale behind tracking 256 block hashes, possibly in a comment or in the module's documentation.
- Ensuring that other parts of the system that previously relied on EVM block hashes are updated to use these cosmos block hashes correctly.
x/evm/module.go (1)
164-167: Consider reorganizing method placement for better readability.While the placement of the PreBlock method at the end of the file is acceptable, consider moving it closer to other lifecycle methods (e.g., InitGenesis, ExportGenesis) for better code organization and readability.
app/app.go (1)
Line range hint
210-218: Verify initialization and error handling forevmIndexerin indexer setup.When setting the EVM indexer using
app.SetEVMIndexer(evmIndexer), ensure thatevmIndexeris properly initialized and notnil. Consider adding error handling to manage potential initialization failures and prevent nil assignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/app.go (2 hunks)
- x/evm/abci.go (1 hunks)
- x/evm/module.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
x/evm/abci.go (1)
1-11: LGTM: Package declaration and imports are appropriate.The package name
evmis suitable for the EVM module, and the imports are relevant to the function's purpose. There are no unused imports, which is good for code cleanliness.x/evm/module.go (3)
32-33: LGTM: Appropriate interface implementation added.The addition of
appmodule.HasPreBlockerinterface implementation is consistent with the PR objectives. This change enables the module to perform pre-block processing, which is essential for tracking block hashes as described in the PR summary.
164-167: LGTM: PreBlock method correctly implemented.The PreBlock method is implemented as expected, unwrapping the SDK context and calling the PreBlock function with the appropriate parameters. This implementation aligns with the PR objectives for tracking block hashes in the pre-block phase.
Line range hint
1-167: Overall assessment: Changes align with PR objectives.The modifications to
x/evm/module.gosuccessfully implement the required functionality for pre-block processing and block hash tracking. The newPreBlockmethod and theHasPreBlockerinterface implementation are correctly added. Minor improvements in comments and code organization have been suggested. These changes effectively address the PR's goal of tracking the latest 256 cosmos block hashes.app/app.go (4)
70-70: Appropriate import ofevmtypesadded.The addition of the import statement for
evmtypesis necessary for referencing EVM module constants likeevmtypes.ModuleName.
Line range hint
144-144: AddedevmIndexerfield toMinitiaAppstruct.Including
evmIndexer evmindexer.EVMIndexerin theMinitiaAppstruct enhances the application's ability to manage EVM indexing functionality.
232-232: Confirm the addition ofevmtypes.ModuleNametoSetOrderPreBlockersis intentional.Adding
evmtypes.ModuleNametoSetOrderPreBlockerschanges the execution order of pre-blockers. Verify that this change aligns with the desired module processing sequence and does not introduce unintended side effects.
Line range hint
176-183: Ensure all calls toNewMinitiaAppare updated with the newevmConfigparameter.Adding
evmConfig evmconfig.EVMConfigto theNewMinitiaAppfunction signature may break existing code that relies on this function. Please verify that all instances whereNewMinitiaAppis called have been updated to include the new parameter, or consider providing a default value to maintain backward compatibility.Run the following script to find all calls to
NewMinitiaAppthat may need updating:✅ Verification successful
Verification Successful: All
NewMinitiaAppcalls include theevmConfigparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `NewMinitiaApp` and ensure they include the `evmConfig` parameter. # Expected: All calls should include `evmConfig`. rg 'NewMinitiaApp\(' -A 5Length of output: 4324
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 28.43% 28.46% +0.03%
==========================================
Files 120 121 +1
Lines 13000 12999 -1
==========================================
+ Hits 3696 3700 +4
+ Misses 8759 8754 -5
Partials 545 545
|
Description
cosmosSDK currently does not allow to change state at abci listener
cosmos/cosmos-sdk#22246
We cannot store the EVM block hashes due to above reason, so just track cosmos block hash instead of EVM block hash.
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
PreBlockfunction to track the latest 256 block hashes in the EVM module.AppModulestructure to include pre-block processing capabilities.Bug Fixes