-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: handle gmp message for leverage messages #2479
Conversation
WalkthroughThe update focuses on refining memo handling within the codebase, specifically in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2479 +/- ##
==========================================
- Coverage 75.38% 69.34% -6.05%
==========================================
Files 100 189 +89
Lines 8025 10944 +2919
==========================================
+ Hits 6050 7589 +1539
- Misses 1589 2727 +1138
- Partials 386 628 +242
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- gmp handling must be done after the transfer - otherwise, if we execute a bank
Send
, before the transfer is recorded, we will miss the funds - Need to add checks as we do in the normal hooks
- If we handle GMP, then we must abort the "normal" hooks execution
- Need to add GMP types and support fallback.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/uibc/gmp/gmp_middleware.go (2 hunks)
- x/uibc/gmp/gmp_middleware_test.go (2 hunks)
- x/uibc/uics20/memo_handler.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
codecov/patch failure (4)
x/uibc/gmp/gmp_middleware.go: [warning] 20-20: x/uibc/gmp/gmp_middleware.go#L20
Added line #L20 was not covered by tests
x/uibc/gmp/gmp_middleware.go: [warning] 39-39: x/uibc/gmp/gmp_middleware.go#L39
Added line #L39 was not covered by tests
x/uibc/uics20/memo_handler.go: [warning] 65-68: x/uibc/uics20/memo_handler.go#L65-L68
Added lines #L65 - L68 were not covered by tests
x/uibc/uics20/memo_handler.go: [warning] 70-70: x/uibc/uics20/memo_handler.go#L70
Added line #L70 was not covered by tests
Additional comments not posted (3)
x/uibc/gmp/gmp_middleware.go (1)
17-39
: TheOnRecvPacket
function has been updated to handle amemoStr
parameter and return aMessage
struct along with an error. This change aligns with the PR's objective to enhance GMP message handling. However, there are a couple of points to consider:
- The function now directly returns errors without any specific handling or logging for different error types, which might be worth adding for better error diagnostics.
- The handling for
TypeGeneralMessageWithToken
simply returnsmsg, nil
, which seems inconsistent with the other case handling. It might be beneficial to clarify the intended behavior here or add additional logic if necessary.Additionally, the removal of
HandleGeneralMessage
andHandleGeneralMessageWithToken
functions suggests a significant refactor. Ensure that their functionalities are either no longer needed or have been integrated elsewhere in the codebase.- return msg, nil + // Assuming additional logic or a specific return message might be required here. + // Placeholder for further implementation or clarification. + return msg, fmt.Errorf("handling for TypeGeneralMessageWithToken not implemented")x/uibc/gmp/gmp_middleware_test.go (1)
40-67
: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-57]
The test cases in
TestGmpMemoHandler
have been updated to reflect changes in memo handling, specifically testing for "invalid memo" and "not supporting the msg" scenarios. These updates are crucial for ensuring theOnRecvPacket
function behaves as expected under various conditions. However, consider the following improvements:
- Adding more descriptive names for test cases to clearly indicate the scenario being tested.
- Including test cases for scenarios not covered, such as receiving an empty
memoStr
and handling unrecognized GMP message types, to ensure comprehensive test coverage.Consider enhancing test case names for clarity and adding additional test cases for scenarios not currently covered.
x/uibc/uics20/memo_handler.go (1)
115-118
: The GMP handling section in theexecute
method has been commented out, potentially indicating a shift in how GMP messages are processed. Given the comments fromgsk967
and the PR author's response, this change seems intentional. However, ensure that this commented-out code is either removed or replaced with an updated implementation if no longer needed, to avoid confusion and maintain code cleanliness.Consider removing or updating the commented-out GMP handling section in the
execute
method for clarity and cleanliness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Need to fix error handling
- Please add more tests, or list all missing tests in the subtask of the epic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/uibc/gmp/gmp_middleware.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/uibc/gmp/gmp_middleware.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/uibc/gmp/gmp_middleware.go (2 hunks)
- x/uibc/gmp/gmp_middleware_test.go (3 hunks)
- x/uibc/gmp/types.go (1 hunks)
- x/uibc/uics20/memo_handler.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/uibc/uics20/memo_handler.go
Additional comments not posted (2)
x/uibc/gmp/types.go (1)
18-19
: The renaming ofMessage
toGMPMemo
is clear and aligns with the intended use within the ICS20 packet memo field. This change enhances clarity and specificity in the codebase.x/uibc/gmp/gmp_middleware_test.go (1)
35-58
: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [23-79]
The test cases for
ParseMemo
function are comprehensive and cover a variety of scenarios, including empty, invalid, unsupported, and valid memos. Ensure that the payload type aligns with the Axelar documentation, as previously discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/uibc/gmp/gmp_middleware.go (1 hunks)
- x/uibc/gmp/gmp_middleware_test.go (4 hunks)
- x/uibc/uics20/memo_handler.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/uibc/gmp/gmp_middleware.go
- x/uibc/gmp/gmp_middleware_test.go
- x/uibc/uics20/memo_handler.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-approving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/oracle/keeper/historic_price.go (1 hunks)
- x/uibc/gmp/gmp_middleware.go (1 hunks)
- x/uibc/gmp/gmp_middleware_test.go (4 hunks)
- x/uibc/gmp/types.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/uibc/gmp/gmp_middleware_test.go
- x/uibc/gmp/types.go
Additional comments not posted (1)
x/oracle/keeper/historic_price.go (1)
48-48
: Ensure the removal of the//nolint: goconst
directive does not introduce any linter warnings or errors.
- Verify that the change complies with the project's coding standards and linter configurations.
- Consider if the repeated string
"denom: "+denom
could be refactored to avoid potential linter warnings about string duplication.
Description
closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers 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
onRecvPacketPrepare
method for improved processing.memo
andmemoPayload
for better deserialization.execute
method for potential processing changes.Message
struct toMemo
for consistency in ICS20 packet memo field.