feat: allow gov and proposer to delete invalid output root #120
Conversation
WalkthroughThe changes introduced in this pull request primarily focus on the Changes
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
==========================================
+ Coverage 49.37% 49.42% +0.04%
==========================================
Files 57 57
Lines 4249 4249
==========================================
+ Hits 2098 2100 +2
+ Misses 1721 1719 -2
Partials 430 430
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/ophost/keeper/msg_server_test.go (1)
Line range hint
1-154: Consider adding negative test cases for better coverage.While the current test coverage is good, consider adding these scenarios:
- Attempt to delete an already deleted output
- Attempt to delete with an invalid bridge ID
_, err = ms.DeleteOutput(ctx, types.NewMsgDeleteOutput(addrsStr[0], 1, 1)) require.NoError(t, err) + + // attempt to delete already deleted output + _, err = ms.DeleteOutput(ctx, types.NewMsgDeleteOutput(addrsStr[0], 1, 1)) + require.Error(t, err) + + // attempt to delete with invalid bridge ID + _, err = ms.DeleteOutput(ctx, types.NewMsgDeleteOutput(addrsStr[0], 999, 1)) + require.Error(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/ophost/keeper/msg_server.go(1 hunks)x/ophost/keeper/msg_server_test.go(2 hunks)
🔇 Additional comments (6)
x/ophost/keeper/msg_server_test.go (5)
122-123: LGTM! Unauthorized access test is well implemented.
The test correctly verifies that unauthorized addresses cannot delete outputs.
Line range hint 125-143: LGTM! Challenger deletion flow is thoroughly tested.
The test covers:
- Successful deletion by challenger
- Verification that the output is deleted
- Error handling when accessing deleted output
- Resubmission capability
- Invalid output index handling
144-146: LGTM! Governance deletion capability is verified.
The test confirms that the governance authority can delete outputs, aligning with the PR objectives.
148-150: LGTM! Resubmission after governance deletion is verified.
The test ensures that outputs can be resubmitted after being deleted by governance.
152-154: LGTM! Proposer deletion capability is verified.
The test confirms that the proposer can delete outputs, which aligns with the PR objectives of allowing proposers to delete invalid output roots.
x/ophost/keeper/msg_server.go (1)
186-188: Verify the impact of batch deletion behavior.
The DeleteOutput function deletes all outputs from the specified index up to the next output index. While this implementation is correct, please ensure that all authorized roles (gov, proposer, and challenger) are aware of this batch deletion behavior, as it could have significant implications on the system state.
Let's verify the usage patterns:
Description
This would allow a invalid output to be deleted by gov proposal or bridge operator.
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
Bug Fixes
Tests