-
Notifications
You must be signed in to change notification settings - Fork 79
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 store_* and get_* methods to take *Path structs instead #419
Conversation
Codecov ReportBase: 55.63% // Head: 55.93% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
+ Coverage 55.63% 55.93% +0.30%
==========================================
Files 122 122
Lines 16996 17093 +97
==========================================
+ Hits 9455 9561 +106
+ Misses 7541 7532 -9
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. |
crates/ibc/src/core/context.rs
Outdated
@@ -534,14 +519,14 @@ pub trait ExecutionContext: ValidationContext { | |||
/// Stores the given connection_end at path | |||
fn store_connection( | |||
&mut self, | |||
connections_path: ConnectionsPath, | |||
connection_path: ConnectionPath, |
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.
Contrary to what was stated in #319, I think all path arguments should be passed by reference, since either the host will generate the path string with to_string()
, or read the internal fields to access its store data structure.
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.
Because of the recent change, I was hesitant to say that :). My thought was there should be a reservation.
But yes, I also think so
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.
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.
🙏
* Implement *Path for get_* and store_* methods * Refactor Path structure to be singular * cargo fmt * Add changelog entry * Fix some args * Fix some other path args in ics04 * Mend some naming mistakes * Pass path args by reference into store_* methods
Closes: #382
Description
I initiated reorganizing the
store_*
methods, but it quickly escalated to more changes.store_*
methods take aPath_*
argument while theget_*
methods had a different approach, accepting the needed parameters directly. IMHO, we should take a consistent approach throughout the repo. I believe that usingPath_*
is easier to maintain, understand the purpose of args, and align with the spec.verify_*
methods under theClientState
trait and allowed us to eliminate some of the#[allow(clippy::too_many_arguments)]
annotations.*Path
structs to be in the singular form as each abstraction refers to a specific path. This previous naming caused confusion while working with the paths. (See also ibc-go)PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.