Problem: not payee set for relayer caller #1654
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1654 +/- ##
===========================================
+ Coverage 16.71% 34.99% +18.28%
===========================================
Files 72 123 +51
Lines 6187 11783 +5596
===========================================
+ Hits 1034 4124 +3090
- Misses 5030 7245 +2215
- Partials 123 414 +291
|
WalkthroughThe changes in this pull request encompass updates to 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 (
|
mmsqe
requested review from
JayT106 and
thomas-nguy
and removed request for
a team
![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 (1)
x/cronos/keeper/precompiles/relayer.go (1)
234-242: Consider a more generic approach for setting the signerThe current implementation duplicates the signer-setting logic in both handlers. Consider introducing a helper function or middleware that automatically sets the signer for all IBC messages that require it. This would:
- Reduce code duplication
- Ensure consistent signer handling
- Make it easier to add similar functionality to other handlers
Example approach:
func (bc *RelayerContract) withSigner(evm *vm.EVM, fn interface{}) interface{} { // Implementation that wraps handlers and sets signer from evm.TxContext.Origin }Would you like me to elaborate on this design pattern?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 234-237: x/cronos/keeper/precompiles/relayer.go#L234-L237
Added lines #L234 - L237 were not covered by tests
[warning] 239-242: x/cronos/keeper/precompiles/relayer.go#L239-L242
Added lines #L239 - L242 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- integration_tests/cosmoscli.py (4 hunks)
- integration_tests/test_ibc_rly.py (1 hunks)
- x/cronos/keeper/precompiles/relayer.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/cronos/keeper/precompiles/relayer.go
[warning] 234-237: x/cronos/keeper/precompiles/relayer.go#L234-L237
Added lines #L234 - L237 were not covered by tests
[warning] 239-242: x/cronos/keeper/precompiles/relayer.go#L239-L242
Added lines #L239 - L242 were not covered by tests
🔇 Additional comments (8)
x/cronos/keeper/precompiles/relayer.go (3)
4-4: LGTM: Context import additionThe context import is appropriately added to support the new context-aware handler implementations.
239-242: Implementation looks good but needs test coverageThe implementation correctly sets the signer for timeout messages, following the same pattern as the acknowledgement handler.
However, this critical IBC functionality lacks test coverage.
Let's verify the existing test coverage:
#!/bin/bash # Search for existing tests related to relayer timeout rg -l "TestRelayer.*Timeout" -g "**/*_test.go"Would you like me to help create test cases that verify:
- The signer is correctly set from the EVM transaction origin
- The timeout handler is called with the correct context and message
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-242: x/cronos/keeper/precompiles/relayer.go#L239-L242
Added lines #L239 - L242 were not covered by tests
234-237: Implementation looks good but needs test coverageThe implementation correctly sets the signer for acknowledgement messages by converting the EVM transaction origin to a Cosmos address. This addresses the missing payee issue.
However, this critical IBC functionality lacks test coverage.
Let's verify the existing test coverage:
Would you like me to help create test cases that verify:
- The signer is correctly set from the EVM transaction origin
- The acknowledgement handler is called with the correct context and message
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 234-237: x/cronos/keeper/precompiles/relayer.go#L234-L237
Added lines #L234 - L237 were not covered by testsintegration_tests/test_ibc_rly.py (1)
295-296:⚠️ Potential issueRemove duplicate fee distribution assertions.
These lines appear to be duplicates of the previous fee distribution assertions. This duplication might be unintentional and could make the test assert incorrect behavior.
Apply this diff to remove the duplicate assertions:
- distribute_fee(src_relayer, fee), - *send_coins(feeibc_addr, src_relayer, src_amount, fee_denom),Let's verify if this is the intended behavior by checking the test implementation:
CHANGELOG.md (1)
12-12: LGTM! The changelog entry is well-formatted and descriptive.The entry clearly describes the bug fix for setting the relayer as payee when incentivized packet functionality is enabled, and follows the consistent format with proper PR linking.
integration_tests/cosmoscli.py (3)
604-609: Improved error handling in broadcast_tx.The changes enhance the transaction broadcasting flow by properly handling the response code and querying transaction events only for successful transactions.
Line range hint
1618-1634: Enhanced register_payee implementation.The changes improve the register_payee method by:
- Simplifying the kwargs handling
- Adding consistent error handling with broadcast_tx
- Properly setting up the home directory parameter
These changes directly address the PR objective of fixing the payee setting for relayer caller.
1646-1647: Consistent implementation in pay_packet_fee.The changes maintain consistency with the register_payee implementation by using the same pattern for home directory and kwargs handling.
7dd3362
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Release Notes for Version v1.4.0-rc2
New Features
Bug Fixes
Improvements