Add a docstring and rename the "validate_self_client" argument for better #436
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 62.21% // Head: 61.12% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 62.21% 61.12% -1.09%
==========================================
Files 131 131
Lines 17789 18114 +325
==========================================
+ Hits 11068 11073 +5
- Misses 6721 7041 +320
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. |
plafer
suggested changes
Feb 16, 2023
Comment on lines
+302
to
+303
| /// Additionally, implementations specific to individual chains can be found | ||
| /// in the [hosts](crate::hosts) module. |
crates/ibc/src/hosts/mod.rs
Outdated
| @@ -0,0 +1 @@ | |||
| pub mod tendermint_helper; | |||
There was a problem hiding this comment.
Suggested change
| pub mod tendermint_helper; | |
| pub mod tendermint; |
Since we're in a "hosts" module, I would find just tendermint sufficient.
Perhaps we can add a module docstring explaining that this module contains items (helpers? utils?) that hosts can use.
| let counterparty_client_state = TmClientState::try_from(counterparty_client_state) | ||
| .map_err(|_| ConnectionError::InvalidClientState { | ||
| reason: "client must be a tendermint client".to_string(), | ||
| pub trait ValidateSelfTendermintClientContext { |
There was a problem hiding this comment.
Since we are in the tendermint_helper module, I think we should remove Tendermint from the name.
crates/ibc/src/core/context.rs
Outdated
| /// in the [hosts](crate::hosts) module. | ||
| fn validate_self_client( | ||
| &self, | ||
| host_client_state_on_counterparty: Any, |
There was a problem hiding this comment.
Suggested change
| host_client_state_on_counterparty: Any, | |
| client_state_of_host_on_counterparty: Any, |
This is closer to the convention we use in the rest of the codebase
Co-authored-by: Philippe Laferrière <plafer@protonmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com>
|
Thank you so much. Got better now 🙏 |
Farhad-Shabani
added a commit
that referenced
this pull request
Sep 9, 2024
…tter (#436) * Add docstring and rename the validate_self_client argument * Rename to host_client_state_on_counterparty * Fix mock arg * Rename ValidateSelfClientContext trait * Reword validate_self_client docstring Co-authored-by: Philippe Laferrière <plafer@protonmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * Apply comments * Rename validate_self_tendermint_client arg --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Philippe Laferrière <plafer@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #434
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.