allow authz msgupdateoracle to systemlane #126
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Warning Rate limit exceeded@sh-cha has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
+ Coverage 49.42% 49.43% +0.01%
==========================================
Files 57 57
Lines 4249 4262 +13
==========================================
+ Hits 2100 2107 +7
- Misses 1719 1722 +3
- Partials 430 433 +3
|
![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 (3)
x/opchild/lanes/free.go (1)
49-51: Document whitelist matching behaviorThe whitelist matching logic now includes both fee payer and fee granter, but this behavior isn't documented. Consider adding a comment to clarify this dual-matching approach.
Add a comment above the loop:
+ // Match if either the fee payer or fee granter (if present) is in the whitelist for _, addr := range whitelist {x/opchild/lanes/system_test.go (1)
46-66: Consider enhancing test coverage with additional scenariosWhile the current test cases cover the basic functionality, consider adding:
- Test case with explicit grantee address instead of nil
- Test case with multiple messages inside MsgExec
- Edge case where MsgExec contains both system and non-system messages
Example enhancement:
+ // Test with explicit grantee + grantee := sdk.AccAddress([]byte("test_grantee")) + authzMsg = authz.NewMsgExec(grantee, []sdk.Msg{ + &opchildtypes.MsgUpdateOracle{}, + }) + require.True(t, handler(ctx, MockTx{ + msgs: []sdk.Msg{ + &authzMsg, + }, + })) + + // Test mixed messages in MsgExec + authzMsg = authz.NewMsgExec(grantee, []sdk.Msg{ + &opchildtypes.MsgUpdateOracle{}, + &banktypes.MsgSend{}, + }) + require.False(t, handler(ctx, MockTx{ + msgs: []sdk.Msg{ + &authzMsg, + }, + }))x/opchild/lanes/free_test.go (1)
101-101: Consider adding a test case for nil fee granter.The implementation is correct, but consider adding a test case where
feeGranteris nil to ensure complete coverage of the interface behavior.func Test_FreeLaneMatchHandler(t *testing.T) { // ... existing tests ... + require.False(t, handler(ctx, MockTx{ + feePayer: []byte{2, 3, 4, 5}, + feeGranter: nil, + })) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
x/opchild/lanes/free.go(1 hunks)x/opchild/lanes/free_test.go(2 hunks)x/opchild/lanes/system.go(2 hunks)x/opchild/lanes/system_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/opchild/lanes/free.go
[warning] 46-47: x/opchild/lanes/free.go#L46-L47
Added lines #L46 - L47 were not covered by tests
x/opchild/lanes/system.go
[warning] 25-25: x/opchild/lanes/system.go#L25
Added line #L25 was not covered by tests
🔇 Additional comments (8)
x/opchild/lanes/system.go (3)
8-8: LGTM: Import addition is appropriate
The authz package import is required for handling the new MsgExec message type.
20-20: LGTM: Idiomatic type switch usage
The switch statement with type assertion into a variable is the correct approach for handling multiple message types.
22-28: Implementation looks good, but needs additional test coverage
The authz.MsgExec handling is implemented correctly with proper validation of:
- Single message requirement
- Inner message type checking
- Error handling
However, the error case (when GetMessages() fails) is not covered by tests.
Let's verify the message handling across the codebase:
Would you like me to help create additional test cases for:
- GetMessages() error scenario
- Multiple inner messages scenario
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-25: x/opchild/lanes/system.go#L25
Added line #L25 was not covered by tests
x/opchild/lanes/free.go (1)
42-48: Add test coverage for fee granter error handling
The error handling path for fee granter address conversion is not covered by tests. This could mask potential issues when invalid addresses are provided.
Let's verify the test coverage:
Would you like me to help generate test cases that cover the error handling scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-47: x/opchild/lanes/free.go#L46-L47
Added lines #L46 - L47 were not covered by tests
x/opchild/lanes/system_test.go (2)
8-8: LGTM: Import addition is appropriate
The authz import is correctly added and necessary for the new test cases.
46-55: LGTM: Test case properly validates authz.MsgExec with system message
The test correctly verifies that SystemLaneMatchHandler accepts an authorized MsgUpdateOracle wrapped in authz.MsgExec.
x/opchild/lanes/free_test.go (2)
56-69: LGTM! Comprehensive test coverage for fee granter functionality.
The new test cases effectively validate the lane matching logic with fee granter support, covering both positive and negative scenarios.
76-78: LGTM! Proper implementation of SDK interfaces.
The MockTx struct correctly implements the required fields for testing fee granting functionality.
Summary by CodeRabbit
New Features
Bug Fixes
Tests