-
Notifications
You must be signed in to change notification settings - Fork 92
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
Refactor verify_delay_passed
and move it under ics03_connection
section
#498
Conversation
verify_delay_passed
and move it under ics03_connection
sectionverify_delay_passed
and move it under ics03_connection
section
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
==========================================
+ Coverage 71.36% 71.94% +0.57%
==========================================
Files 125 126 +1
Lines 15752 15771 +19
==========================================
+ Hits 11242 11347 +105
+ Misses 4510 4424 -86
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
This is a great change! The fact that we were taking in a ConnectionEnd
in client verification functions was a big red flag. In the timeout case, we were performing that verification twice, as a result of the poor separation of concerns.
Thank you for this!
as for the removed tests, could you also create an issue to remind us to add them back? |
Glad you liked this change 🤗
I tried to cover needed tests in this PR, but the changing scope was beyond PR's concern, and I didn't want to wrestle with merge conflicts :) Here is the opened issue: #505 |
…ection (#498) * Export standalone commitment creators for Ack & Packet * Remove compute mod + make compute functions private * Fix clippy catch * Move verify_delay_passed under the ICS03 * Change connection_end args to prefix * Fix tests * Use core::cmp::Ord for Timestamp comparison * Remove after method of Timestamp
Closes: #404
Description
This incorrectly placed
verify_delay_passed
caused entanglements for some packet validation methods and forced them to acceptctx
as an argument, which was unnecessary.This PR got pushed to the top since the side effect is that we got limited and can't well explore new changes regarding
*Context
andRouter
traits. This should make it easier to work on issues like #490Remark
Upon this change, previous
verify_delay_passed
tests no longer apply, and appropriate tests for two conditions of having connections with and without delay should be added for appropriate message types likerecv_packet
. (as a later PR?!)PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.