Skip to content
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: make custom Validation/ExecutionContext under ICS-07 usable by any light clients #1166

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Apr 15, 2024

Closes: #1163

Step before: #1167


PR author checklist:

  • Added changelog entry, using unclog.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 78.78788% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 63.79%. Comparing base (c659e21) to head (278c609).

Files Patch % Lines
...nts/ics07-tendermint/src/client_state/execution.rs 68.75% 5 Missing ⚠️
...ts/ics07-tendermint/src/client_state/validation.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1166   +/-   ##
=======================================
  Coverage   63.79%   63.79%           
=======================================
  Files         219      219           
  Lines       21391    21389    -2     
=======================================
- Hits        13647    13646    -1     
+ Misses       7744     7743    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review April 15, 2024 14:39
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; just some nits and a question on my end.

ibc-clients/ics07-tendermint/src/context.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
/// [`ExtClientValidationContext`] and [`ClientExecutionContext`]
pub trait ExtClientExecutionContext: ExtClientValidationContext + ClientExecutionContext {}

impl<T> ExtClientExecutionContext for T where T: ExtClientValidationContext + ClientExecutionContext {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, based off of how this trait is being used, it's essentially a trait alias for any types that implement ExtClientValidationContext and ClientExecutionContext. Is that an appropriate way to think about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Updated the docstring: d4e66c2
Hope it's more clear now.

Copy link
Member

@rnbguy rnbguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I added some comments.

ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/context/src/context.rs Outdated Show resolved Hide resolved
ibc-clients/ics07-tendermint/src/verifier.rs Outdated Show resolved Hide resolved
Comment on lines +12 to +20
/// In order to wire up the custom verifier, the `verify_client_message` method
/// on the `ClientStateValidation` trait must be implemented. The simplest way
/// to do this is to import and call the standalone `verify_client_message`
/// function located in the `ibc::clients::tendermint::client_state` module,
/// passing in your custom verifier type as its `verifier` parameter. The rest
/// of the methods in the `ClientStateValidation` trait can be implemented by
/// importing and calling their analogous standalone version from the
/// `tendermint::client_state` module, unless bespoke logic is desired for any
/// of those functions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reading this, but the short context makes it a bit confusing. Can we add an example (less than 10 lines) in a codeblock here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very spot-on comment! I have been trying to add an example and realized we really don't need to be maintaining TmVerifier. Everywhere we can simply use tendermint_light_client_verifier::Verifier as the trait bound.

Farhad-Shabani and others added 2 commits April 15, 2024 11:51
Co-authored-by: Rano | Ranadeep <[email protected]>
Signed-off-by: Farhad Shabani <[email protected]>
@Farhad-Shabani Farhad-Shabani added this pull request to the merge queue Apr 15, 2024
Merged via the queue into main with commit 68513d8 Apr 15, 2024
18 checks passed
@Farhad-Shabani Farhad-Shabani deleted the farhad/custom-context-under-ics02 branch April 15, 2024 19:08
Farhad-Shabani added a commit that referenced this pull request Sep 9, 2024
… any light clients (#1166)

* imp: move TmValidation/ExecutionContext into ICS-02 for general usage

* chore: add changelog

* review: rename context.rs to verifier.rs

* review: better docstring for ExtClientExecutionContext

* fix: apply review comments

* nit: update changelog

Co-authored-by: Rano | Ranadeep <[email protected]>
Signed-off-by: Farhad Shabani <[email protected]>

---------

Signed-off-by: Farhad Shabani <[email protected]>
Co-authored-by: Rano | Ranadeep <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make custom Validation/ExecutionContext under ICS-07 portable
3 participants