feat: add submsg impl #277
Conversation
…ade handler for testnet upgrade
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several updates across multiple files, primarily focusing on version upgrades, command modifications, and enhancements to message handling within the application. Key changes include updating the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later OpenSSF Scorecard
Scanned Manifest Filesgo.mod
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
==========================================
+ Coverage 40.53% 40.59% +0.05%
==========================================
Files 267 267
Lines 25314 25400 +86
==========================================
+ Hits 10262 10310 +48
- Misses 13464 13503 +39
+ Partials 1588 1587 -1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (3)
x/move/keeper/common_test.go (1)
563-566: LGTM: Added failure condition for testing.The new condition allows for testing failure scenarios in the
MsgExecuteJSONhandler, which is valuable for comprehensive testing.Consider enhancing the error message to be more descriptive, e.g.,
fmt.Errorf("intentional failure for testing"). This would make the purpose of the error clearer in test outputs.if msg.FunctionName == "fail" { - return nil, fmt.Errorf("fail") + return nil, fmt.Errorf("intentional failure for testing") }x/move/keeper/handler.go (2)
461-461: Typo in comment: "becuase" should be "because"There's a minor typographical error in the comment at line 461. The word "becuase" should be corrected to "because".
471-473: Ensure correct formatting of callback argumentsWhen formatting the callback arguments using
fmt.Sprintf, ensure that the values are correctly formatted:
- Line 471:
fmt.Sprintf("\"%d\"", callback.Id)adds extra quotes around the integer ID, resulting in a string like"\"123\"". If the callback expects a string representation of the integer without extra quotes, consider removing the escaped quotes.- Line 472:
fmt.Sprintf("%v", success)formats a boolean value. Ensure that this matches the expected input type for the callback function.Consider updating the code as follows:
fmt.Sprintf("%d", callback.Id), fmt.Sprintf("%t", success),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (10)
- Dockerfile (1 hunks)
- app/upgrade.go (1 hunks)
- cmd/initiad/root.go (1 hunks)
- cmd/move/move.go (5 hunks)
- x/move/keeper/common_test.go (3 hunks)
- x/move/keeper/contracts/sources/Submsg.move (1 hunks)
- x/move/keeper/handler.go (2 hunks)
- x/move/keeper/handler_test.go (9 hunks)
- x/move/types/events.go (2 hunks)
- x/move/types/params.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
Dockerfile (2)
Line range hint
1-62: Verify build process with the new LIBMOVEVM versionThe update to LIBMOVEVM v0.5.0 is the only change in this Dockerfile. While the change itself looks good, it's crucial to ensure that the entire build process works correctly with this new version.
Please make sure to:
- Run the full CI/CD pipeline to verify that the build succeeds with the new LIBMOVEVM version.
- Test the resulting Docker image to ensure all functionalities work as expected.
- If possible, perform a local build and test before merging this change.
This will help catch any potential issues that might arise from the version update early in the development process.
9-9: LGTM. Verify compatibility and update documentation.The update of LIBMOVEVM_VERSION from v0.4.12 to v0.5.0 looks good. This change might introduce new features or bug fixes in the LIBMOVEVM library.
Please ensure the following:
- Verify that v0.5.0 of LIBMOVEVM is compatible with the rest of the project.
- Update any relevant documentation or dependency lists to reflect this version change.
To verify the compatibility and usage of this new version, you can run the following script:
This script will help identify any areas of the codebase that might need attention due to the version update.
✅ Verification successful
Verification Successful: All references to LIBMOVEVM version v0.4.12 have been successfully updated to v0.5.0. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for LIBMOVEVM version references and potential incompatibilities # Check for any hardcoded version references that might need updating echo "Checking for hardcoded LIBMOVEVM version references:" rg --type-add 'go:*.{go,mod}' --type go -i 'libmovevm.*0\.4\.12' # Check go.mod for any LIBMOVEVM dependencies that might need updating echo "Checking go.mod for LIBMOVEVM dependencies:" rg --type-add 'go:*.{go,mod}' --type go '^require.*libmovevm' go.mod # Look for any LIBMOVEVM API usage that might have changed echo "Checking for LIBMOVEVM API usage:" rg --type-add 'go:*.go' --type go 'libmovevm\.' # Check for any tests related to LIBMOVEVM that might need updating echo "Checking for LIBMOVEVM-related tests:" rg --type-add 'go:*.go' --type go 'Test.*LIBMOVEVM'Length of output: 615
Script:
#!/bin/bash # Description: Check for LIBMOVEVM version references and potential incompatibilities # Check for any hardcoded version references that might need updating echo "Checking for hardcoded LIBMOVEVM version references:" rg --type-add 'go:*.{go,mod}' --type go -i 'libmovevm.*0\.4\.12' # Check go.mod for any LIBMOVEVM dependencies that might need updating echo "Checking go.mod for LIBMOVEVM dependencies:" rg --type-add 'go:*.{go,mod}' --type go '^require.*libmovevm' go.mod # Look for any LIBMOVEVM API usage that might have changed echo "Checking for LIBMOVEVM API usage:" rg --type-add 'go:*.go' --type go 'libmovevm\.' # Check for any tests related to LIBMOVEVM that might need updating echo "Checking for LIBMOVEVM-related tests:" rg --type-add 'go:*.go' --type go 'Test.*LIBMOVEVM'Length of output: 615
x/move/types/params.go (1)
23-23: Significant change in default contract shared revenue ratioThe
DefaultContractSharedRevenueRatiohas been changed from 50% (math.LegacyNewDecWithPrec(50, 2)) to 0% (math.LegacyZeroDec()). This change will affect the default behavior of the Move module's revenue sharing model for contracts.Potential impacts:
- Existing systems or users expecting the previous 50% ratio might experience unexpected behavior.
- This change could significantly alter the economics of contract interactions within the system.
While this change aligns with the PR objectives of implementing new options for Stargate messages and handling failures, it's important to ensure that this modification is intentional and well-documented.
Could you please clarify the rationale behind this change and confirm if any documentation or migration guides need to be updated to reflect this new default behavior?
✅ Verification successful
DefaultContractSharedRevenueRatio Change Verified
The
DefaultContractSharedRevenueRatiohas been successfully changed from 50% to 0%, and this modification is localized to the parameter definition and its related tests. No references to the old default value were found in the documentation or other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old default value in comments, documentation, or test files # Test: Search for references to the old 50% default value echo "Searching for references to the old 50% default value:" rg --type md --type go "50%" -C 2 echo "---" # Test: Search for references to contract shared revenue ratio in test files echo "Searching for contract shared revenue ratio tests:" rg --type go "ContractSharedRevenueRatio" -C 2 $(fd -e go test)Length of output: 11472
x/move/keeper/common_test.go (2)
490-490: LGTM: New variable for submsg module.The addition of
submsgModuleis consistent with the existing pattern for other Move modules in this test file.
499-499: LGTM: Initialization of submsg module.The
submsgModuleis properly initialized in theinit()function, consistent with other module initializations.x/move/keeper/contracts/sources/Submsg.move (3)
7-19: Functionstargateis implemented correctlyThe
stargatefunction appropriately handles theallow_failureflag to determine the correct options forcosmos::stargate_with_options. The function parameters are well-defined, and the logic aligns with the intended functionality.
34-41:callback_with_signerfunction correctly emits event with signerThe
callback_with_signerfunction properly emits theResultEventWithSignerevent (orResultEventif consolidated) with the correct account address and parameters. The use ofsigner::address_of(account)accurately captures the signer's address.
43-48:callback_without_signerfunction correctly emits event without signerThe
callback_without_signerfunction correctly emits theResultEventevent with the providedidandsuccessparameters. The implementation is straightforward and functions as intended.app/upgrade.go (1)
15-15: Update ofupgradeNamereflects the new versionThe
upgradeNameconstant has been updated to"0.5.4", which aligns with the PR objective of implementing changes for version0.5.4. This is appropriate and ensures that the upgrade handler targets the correct version.x/move/keeper/handler.go (2)
8-8: Importing "fmt" package is necessaryThe addition of the
"fmt"package import is appropriate, asfmt.Sprintfis used in the code for string formatting.
417-424: Refactoring improves code readability and maintainabilityExtracting the message dispatch logic into the
dispatchMessagemethod enhances code modularity and readability. This separation of concerns makes theDispatchMessagesfunction cleaner and the codebase easier to maintain.x/move/keeper/handler_test.go (1)
169-169: Verify event index calculations to prevent off-by-one errorsIn several test functions (
TestDispatchDelegateMessage,TestDispatchFundCommunityPoolMessage,TestDispatchTransferMessage,TestDispatchPayFeeMessage,TestDispatchFundMoveExecute,TestDispatchFundMoveExecuteWithJson,TestDispatchFundMoveScript,TestDispatchFundMoveScriptWithJson), the event is retrieved usingevents[len(events)-2].Using hard-coded indices may lead to fragile tests if the number of emitted events changes. Please verify that this index accurately references the intended event after the test execution.
Consider refactoring the event retrieval logic to search for the event based on its type or attributes. This would make the tests more robust against changes in the event list.
Also applies to: 203-203, 272-272, 339-339, 390-390, 440-440, 483-483, 524-524
cmd/move/move.go (3)
102-102: PassinguseMinlibtomoveNewCmdThe call to
moveNewCmdnow includes theuseMinlibparameter, allowing themove newcommand to utilize the minimal library option as intended.
311-311: Update function signature ofmoveNewCmdto acceptuseMinlibThe function
moveNewCmdnow acceptsuseMinlibas a parameter, enabling conditional logic based on this flag within the command.
89-89: Update function signature to includeuseMinlibThe function
MoveCommandnow accepts a new parameteruseMinlib. Ensure that all calls toMoveCommandare updated to pass the correctuseMinlibvalue to prevent compile-time errors.Run the following script to verify that all invocations of
MoveCommandare updated:✅ Verification successful
Further Verification Needed
The previous script may have captured function definitions instead of function calls. Please run the following script to identify actual calls to
MoveCommandthat do not include theuseMinlibparameter:
All
MoveCommandCalls are Correctly UpdatedAll invocations of
MoveCommandinclude theuseMinlibparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `MoveCommand` include the new `useMinlib` parameter. # Test: Search for function calls to `MoveCommand` in Go files. # Expect: All calls should pass `ac` and `useMinlib` arguments. rg --type go -o 'MoveCommand\([^\)]*\)' | grep -v 'MoveCommand(ac, useMinlib)'Length of output: 225
Script:
#!/bin/bash # Description: Verify all calls to `MoveCommand` include the new `useMinlib` parameter, excluding function definitions. # Test: Search for function calls to `MoveCommand` in Go files, excluding definitions. # Expect: All calls should pass `ac` and `useMinlib` arguments. rg --type go 'MoveCommand\(' | grep -v 'func MoveCommand' | grep -v 'MoveCommand(ac, useMinlib)'Length of output: 232
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
x/move/keeper/handler.go (2)
509-525: LGTM: Proper message handling and event emissionThe implementation of message handler lookup, execution, and event emission is well-structured and includes appropriate error handling.
Consider adding more context to the error returned when a handler is not found. This can help with debugging and provide more information about which message type caused the issue. Here's a suggested improvement:
handler := k.msgRouter.Handler(msg) if handler == nil { - err = types.ErrNotSupportedCosmosMessage + err = fmt.Errorf("%w for message type %T", types.ErrNotSupportedCosmosMessage, msg) return }This change will include the message type in the error, making it easier to identify unsupported message types during debugging.
433-435: LGTM: Robust panic recovery addedThe addition of panic recovery in the deferred function is an excellent improvement that enhances the overall robustness of the code.
Consider adding more context to the panic error message to aid in debugging. Here's a suggested improvement:
defer func() { if r := recover(); r != nil { - err = fmt.Errorf("panic: %v", r) + err = fmt.Errorf("panic in dispatchMessage: %v", r) } // ... rest of the deferred function }()This change provides more specific information about where the panic occurred, which can be helpful during troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/move/keeper/handler.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
x/move/keeper/handler.go (2)
417-424: LGTM: Improved code structure and error handlingThe refactoring of
DispatchMessagesto use the newdispatchMessagefunction improves code organization and readability. The added error handling ensures that execution stops if any message dispatch fails, which is appropriate for maintaining consistency.
Line range hint
1-725: Overall assessment: Significant improvements in code structure and robustnessThe changes in this file represent a substantial enhancement to the message handling logic. Key improvements include:
- Refactoring of the
DispatchMessagesfunction for better code organization.- Introduction of the
dispatchMessagefunction with comprehensive error handling and panic recovery.- Improved handling of different message types (Stargate and non-Stargate).
- Enhanced event emission and callback execution.
These changes contribute to better readability, maintainability, and robustness of the code. The minor suggestions provided in the review comments are aimed at further improving consistency in error handling and enhancing debugging capabilities.
Great job on these improvements!
Description
Allow executing Stargate message with options to allow callback and failure.
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...