Organize host chain methods under a new `HostChainContext` trait #386

Farhad-Shabani · 2023-01-27T17:55:19Z

Description

Following the discussion around client state validation methods that check parameters (fields) of the client state implementation. After a bit of thought, I came up with an idea (around validate_self_client) of combining all host-related methods under a single chain called "HostChainContext" and separating related host errors under the "HostError" enum, plus added some docstrings.
These methods do not have chain-specific arguments and returns, so ideally, any chain should be able to provide them.
There just remains one small cavity. Having the "unbonding_period" method in it, which looks more of Tendermint chains. Couldn't find a better way at the moment, and I addressed it by changing the return type into the Option<Duration>

Note: The change in this PR is also connected to #418. That should have been handled here, hopefully.

PR author checklist:

Added changelog entry, using unclog.
Added tests.
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).

codecov · 2023-01-27T17:59:53Z

Codecov Report

Base: 55.63% // Head: 55.94% // Increases project coverage by +0.31% 🎉

Coverage data is based on head (037e069) compared to base (649e7bd).
Patch coverage: 12.93% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   55.63%   55.94%   +0.31%     
==========================================
  Files         122      122              
  Lines       16996    17103     +107     
==========================================
+ Hits         9455     9568     +113     
+ Misses       7541     7535       -6

Impacted Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/host_helpers.rs	`0.00% <0.00%> (ø)`
crates/ibc/src/core/context.rs	`3.91% <0.00%> (+0.02%)`	⬆️
crates/ibc/src/core/ics02_client/error.rs	`4.76% <ø> (ø)`
crates/ibc/src/core/ics03_connection/error.rs	`7.69% <ø> (ø)`
crates/ibc/src/core/ics04_channel/context.rs	`81.41% <ø> (+2.62%)`	⬆️
crates/ibc/src/core/ics24_host/error.rs	`0.00% <0.00%> (ø)`
crates/ibc/src/mock/context.rs	`61.85% <53.57%> (-0.11%)`	⬇️
crates/ibc/src/test_utils.rs	`53.72% <0.00%> (-0.45%)`	⬇️
...bc/src/core/ics02_client/handler/upgrade_client.rs	`63.74% <0.00%> (-0.44%)`	⬇️
...ibc/src/core/ics02_client/handler/update_client.rs	`38.46% <0.00%> (-0.21%)`	⬇️
... and 25 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

plafer · 2023-02-14T15:22:39Z

I came up with an idea (around validate_self_client) of combining all host-related methods under a single chain called "HostChainContext"

ValidationContext and ExecutionContext are all the host-related methods; in the previous API, Client/Connection/Channel{Keeper,Reader} contained the host methods. What criterion did you use to determine whether a method should be moved to the HostChainContext? For example, isn't consensus_state() a host method too? Or connection_end()? Or any other method?

These methods do not have chain-specific arguments and returns, so ideally, any chain should be able to provide them.

What do you mean by "chain-specific arguments and returns"? As designed, ValidationContext & ExecutionContext are implementable by any chain. To better understand where you're coming from, could you give an example of a method in ValidationContext/ExecutionContext that a hypothetical chain would not be able to implement?

plafer · 2023-02-14T15:30:51Z

crates/ibc/src/clients/ics07_tendermint/host_helpers.rs

+    counterparty_client_state: Any,
+) -> Result<(), HostError>
+where
+    Ctx: HostChainContext,


ValidateSelfClientContext is designed to ask for an implementation of all the methods that it needs in its implementation; some only implementable by Tendermint hosts (such as unbonding_period()). I believe this is the root cause for why you had to change the signature to Option<Duration>; not all hosts can implement it.

This is why I don't think it is appropriate for ValidationContext to have HostChainContext as a supertrait. Hypothetically, say we also provided the implementation of validate_self_client() for NEAR hosts; would we need to have that Context be a supertrait of ValidationContext as well, requiring all hosts to implement NEAR-specific methods (and have the signature return an Option for those too)?

I understand your concern, but rather than the unbonding_period() method, which I also found it there to be a wart, other methods should be available by any chain

plafer · 2023-02-14T15:35:27Z

crates/ibc/src/core/ics04_channel/context.rs

+        let host_height = self.host_height()?;
+        let host_consensus_state = self.host_consensus_state(&host_height)?;
+        Ok(host_consensus_state.timestamp())


I don't think we can assume that this will work for all hosts. Basecoin-rs is an example: we added pending_host_consensus_state() because host_consensus_state() was not quite right. I think it should simply be made abstract; each host can implement it however they see fit. We should do it in a separate PR though to keep PRs as self-contained as possible.

Actually, moving the host_timestamp() methods brought a need to handle them here. but, sure alright.

what do you mean by "moving the host_timestamp() methods brought a need to handle them here"?

Farhad-Shabani · 2023-02-14T15:44:33Z

ValidationContext and ExecutionContext are all the host-related methods; in the previous API, Client/Connection/Channel{Keeper,Reader} contained the host methods. What criterion did you use to determine whether a method should be moved to the HostChainContext? For example, isn't consensus_state() a host method too? Or connection_end()? Or any other method?

For a clearer picture of where it's instantiated, consider the below grouping:

Group 1: Methods that retrieve state from the consensus engine (or we shall say from non-module parts) and validate a state against them
Group 2: Methods that call the value of a state from paths (module-related parts) and validate them
Group 3: Methods that set a new state by storing in the paths (module-related parts)

Farhad-Shabani · 2023-02-14T16:01:14Z

The idea had the aim of demonstrating how it could make implementation smoother, but looking at your comment, if it's not going to make it easier for us, then it wouldn't work for that purpose.

plafer · 2023-02-15T16:34:52Z

Closing as discussed in sync meeting.

Expose validate method for client state interface

d40bd07

Farhad-Shabani self-assigned this Jan 27, 2023

Farhad-Shabani changed the title ~~Expose validate method for client state interface~~ Expose validate method on client state interface Jan 27, 2023

Add changelog entry

52104f4

Farhad-Shabani added 5 commits February 7, 2023 19:40

Merge branch 'main' into farhad/separate-validate-from-new

2b8d1c5

Merge branch 'main' into farhad/separate-validate-from-new

e6d5435

Define HostChainContext and HostError

1c798ec

Drop exposed validate method and revert relative changes

00d237c

Revise changelog entry

a23c79a

Farhad-Shabani changed the title ~~Expose validate method on client state interface~~ Organize host chain methods under a new HostChainContext trait Feb 14, 2023

Change return type for unbonding_period method to Option<Duration>

037e069

Farhad-Shabani marked this pull request as ready for review February 14, 2023 14:49

Farhad-Shabani requested a review from plafer February 14, 2023 14:49

plafer reviewed Feb 14, 2023

View reviewed changes

plafer mentioned this pull request Feb 15, 2023

Refactor ValidationContext and ExecutionContext #325

Open

plafer closed this Feb 15, 2023

Farhad-Shabani deleted the farhad/separate-validate-from-new branch February 21, 2023 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize host chain methods under a new `HostChainContext` trait #386

Organize host chain methods under a new `HostChainContext` trait #386

Farhad-Shabani commented Jan 27, 2023 •

edited

Loading

codecov bot commented Jan 27, 2023 •

edited

Loading

plafer commented Feb 14, 2023

plafer Feb 14, 2023

Farhad-Shabani Feb 14, 2023

plafer Feb 14, 2023

Farhad-Shabani Feb 14, 2023

plafer Feb 14, 2023

Farhad-Shabani commented Feb 14, 2023

Farhad-Shabani commented Feb 14, 2023

plafer commented Feb 15, 2023

Organize host chain methods under a new HostChainContext trait #386

Organize host chain methods under a new HostChainContext trait #386

Conversation

Farhad-Shabani commented Jan 27, 2023 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Jan 27, 2023 • edited Loading

Codecov Report

plafer commented Feb 14, 2023

plafer Feb 14, 2023

Choose a reason for hiding this comment

Farhad-Shabani Feb 14, 2023

Choose a reason for hiding this comment

plafer Feb 14, 2023

Choose a reason for hiding this comment

Farhad-Shabani Feb 14, 2023

Choose a reason for hiding this comment

plafer Feb 14, 2023

Choose a reason for hiding this comment

Farhad-Shabani commented Feb 14, 2023

Farhad-Shabani commented Feb 14, 2023

plafer commented Feb 15, 2023

Organize host chain methods under a new `HostChainContext` trait #386

Organize host chain methods under a new `HostChainContext` trait #386

Farhad-Shabani commented Jan 27, 2023 •

edited

Loading

codecov bot commented Jan 27, 2023 •

edited

Loading