-
Notifications
You must be signed in to change notification settings - Fork 100
imp: remove order_matches from ChannelEnd impl #1095
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
imp: remove order_matches from ChannelEnd impl #1095
Conversation
…r from ChannelEnd impl
Hi @Farhad-Shabani, please take a look at this PR. Thank you. |
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.
Thanks @tropicaldog for the PR 🙏
Last nit before I approve the PR !
Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]>
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.
Awesome ! Thanks ✨
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
- Coverage 66.58% 66.53% -0.06%
==========================================
Files 204 204
Lines 20519 20532 +13
==========================================
- Hits 13663 13661 -2
- Misses 6856 6871 +15 ☔ View full report in Codecov by Sentry. |
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.
Thank you @tropicaldog for handling this PR and @rnbguy for the review. Just wanted to make a point:
We consider Order::None
as an invalid order type, explicitly disallowing the creation of a channel with this order type. This validation aligns with the check conducted in ibc-go.
Therefore, if our IBC handlers encounter a channel end with an order of None
during the retrieval process from a store, it signals a potential manipulation in the storage. In such cases, I believe it should be treated as an error.
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.
Thanks @tropicaldog ! Just a nit request. Then we are ready to merge 🙂
additional blank line for consistency Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]>
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.
Nice work, @tropicaldog 🚀
Thanks for the first contribution 💜
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.
Thank you for the fix! 👌🏻
* replace if-else branch to match statement, remove order_matches helper from ChannelEnd impl * add unclog * deprecate instead of delete the method * update changelog * Update .changelog/unreleased/improvements/394-remove-order-match.md Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]> * fix: return err on Order::None * Update ibc-core/ics04-channel/src/handler/timeout.rs additional blank line for consistency Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]> --------- Signed-off-by: Tuan Tran <[email protected]> Co-authored-by: Rano | Ranadeep <[email protected]>
Closes: #394
Description
Remove the
order_matches
method, refactor the current usage oforder_matches
to use match statements instead.PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.