Implement ValidationContext and ExecutionContext for connections (ICS-3) #257

plafer · 2022-11-22T17:15:59Z

Closes: #251
Closes: #271

Ensured that fixes done in #272 were applied in this branch as well

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).

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs

…-execution-ics3

plafer · 2022-12-02T21:51:14Z

@hu55a1n1 ready for review again 🙏

* Track code coverage with `cargo-llvm-cov` * Add changelog entry * Add coverage badge to the README

* Add ClientState::check_misbehaviour_and_update_state() * Implement misbehaviour handler * impl Protobuf<Any> for Misbehaviour * Remove redundant definition of decode_header() * Implement ChainId::with_version() * Getters for Tm Misbehaviour * Add missing checks for conversion from RawMisbehaviour * Make TmClientState::with_frozen_height() infallible * Implement TmClientState::check_misbehaviour_and_update_state() * Cleanup inner functions * Cleanup errors * Clippy fix * Ctor for TmMisbehaviour * Use git dependencies for tendermint crates * Add VerifyCommitLightTrusting check * Add VerifyCommit check * Clippy fix * Patch tendermint deps for no-std-check * Convert Tendermint VerificationError * Add helpers `Header::as_{un}trusted_block_state()` * Reorder untrusted verification logic * Reorder trusted verification logic * Misbehavior -> misbehaviour * Check for matching chain-ids * cargo update ci/no-std-check * Fix build failure after merge with main * Update for API changes in tm PR * Delete ci/no-std-check/Cargo.lock * Add changelog entry * Cleanup (naming & comments) * Rename check_trusted_header() -> check_header_validator_set() * Rename check_misbehaviour_header() -> check_header_and_validator_set() * Rename MisbehaviourConsensusStateTimestampGteTrustingPeriod -> ConsensusStateTimestampGteTrustingPeriod * Rename verify_misbehaviour_header_commit() -> verify_header_commit_against_trusted() * Remove redundant client state expired check * Impl Protobuf conversions for mock Misbehaviour * Impl check_misbehaviour_and_update_state() for mock Misbehaviour * Remove cargo patches * Fixes after tendermint-rs bump * Fix typo * Add tests * MockClientState::with_frozen_height() * Provide MockContext helper to set client chain-id * Conversions from HostBlock -> TmLightBlock -> TmHeader * Fix tests * Clippy fix * Cleanup tests * Add comments for tests * Clippy fix * Rustfmt

codecov · 2022-12-05T15:41:32Z

Codecov Report

Base: 65.31% // Head: 64.84% // Decreases project coverage by -0.46% ⚠️

Coverage data is based on head (b995322) compared to base (5bceb90).
Patch coverage: 52.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
- Coverage   65.31%   64.84%   -0.47%     
==========================================
  Files         125      126       +1     
  Lines       13210    13854     +644     
==========================================
+ Hits         8628     8984     +356     
- Misses       4582     4870     +288

Impacted Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/host_helpers.rs	`0.00% <ø> (ø)`
crates/ibc/src/core/handler.rs	`0.00% <0.00%> (ø)`
...rc/core/ics03_connection/handler/conn_open_init.rs	`57.84% <15.68%> (-42.16%)`	⬇️
crates/ibc/src/mock/context.rs	`70.41% <23.35%> (-5.52%)`	⬇️
crates/ibc/src/core/context.rs	`33.33% <54.54%> (+23.75%)`	⬆️
...core/ics03_connection/handler/conn_open_confirm.rs	`77.50% <65.64%> (-22.50%)`	⬇️
...src/core/ics03_connection/handler/conn_open_ack.rs	`78.05% <66.87%> (-14.51%)`	⬇️
...src/core/ics03_connection/handler/conn_open_try.rs	`80.99% <71.03%> (-14.93%)`	⬇️

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.

hu55a1n1

Looks great! Added some suggestions mostly for error handling.

hu55a1n1 · 2022-12-06T19:32:54Z

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs

+        return Err(ContextError::ConnectionError(
+            ConnectionError::ConnectionMismatch {
+                connection_id: msg.conn_id_on_b.clone(),
+            },
+        ));


I think it'll be nice to use the Into impl for converting ConnectionError to ContextError (here and elsewhere) ->

Suggested change

return Err(ContextError::ConnectionError(

ConnectionError::ConnectionMismatch {

connection_id: msg.conn_id_on_b.clone(),

},

));

return Err(ConnectionError::InvalidConsensusHeight {

target_height: msg.consensus_height_of_a_on_b,

current_height: host_height,

}

.into());

Yes much cleaner 093cc9e

hu55a1n1 · 2022-12-06T19:39:07Z

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs

+                .client_state(client_id_on_b)
+                .map_err(|_| ConnectionError::Other {
+                    description: "failed to fetch client state".to_string(),
+                })?;


Is there a reason why we don't simply want to return the ContextError in such cases? I mean why not ->

Suggested change

.client_state(client_id_on_b)

.map_err(|_| ConnectionError::Other {

description: "failed to fetch client state".to_string(),

})?;

.client_state(client_id_on_b)?;

This would work now, but would break when we implement #269. So this is future-proof.

hu55a1n1 · 2022-12-06T19:40:39Z

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs

+            .consensus_state(client_id_on_b, msg.proof_height_on_a)
+            .map_err(|_| ConnectionError::Other {
+                description: "failed to fetch client consensus state".to_string(),
+            })?;


Same here and elsewhere, where we call context methods that return ContextError ->

Suggested change

.consensus_state(client_id_on_b, msg.proof_height_on_a)

.map_err(|_| ConnectionError::Other {

description: "failed to fetch client consensus state".to_string(),

})?;

.consensus_state(client_id_on_b, msg.proof_height_on_a)?;

Same reasoning here

hu55a1n1 · 2022-12-06T19:41:34Z

crates/ibc/src/core/mod.rs


 pub use context::ExecutionContext;
 pub use context::ValidationContext;
+
+pub use handler::execute;
+pub use handler::validate;


Do we also want to reexport context::ContextError here?

…S-3) (#257) * `ConnOpenInit::validate` * conn_open_init: execute * conn_open_try `validate` and `execute` * conn_open_ack::validate * conn_open_ack::execute * conn_open_confirm::validate * conn_open_confirm::execute * changelog * LocalVars * validate_impl and execute_impl * Remove useless clone * fix ConnOpenInit::validate * fix conn_open_ack (as in #272) * conn_open_try: LocalVars * conn_open_try validate/execute impl * conn_open_ack LocalVars * conn_open_ack impl * Track code coverage with `cargo-llvm-cov` and codecov.io (#277) * Track code coverage with `cargo-llvm-cov` * Add changelog entry * Add coverage badge to the README * Misbehaviour handling implementation (#215) * Add ClientState::check_misbehaviour_and_update_state() * Implement misbehaviour handler * impl Protobuf<Any> for Misbehaviour * Remove redundant definition of decode_header() * Implement ChainId::with_version() * Getters for Tm Misbehaviour * Add missing checks for conversion from RawMisbehaviour * Make TmClientState::with_frozen_height() infallible * Implement TmClientState::check_misbehaviour_and_update_state() * Cleanup inner functions * Cleanup errors * Clippy fix * Ctor for TmMisbehaviour * Use git dependencies for tendermint crates * Add VerifyCommitLightTrusting check * Add VerifyCommit check * Clippy fix * Patch tendermint deps for no-std-check * Convert Tendermint VerificationError * Add helpers `Header::as_{un}trusted_block_state()` * Reorder untrusted verification logic * Reorder trusted verification logic * Misbehavior -> misbehaviour * Check for matching chain-ids * cargo update ci/no-std-check * Fix build failure after merge with main * Update for API changes in tm PR * Delete ci/no-std-check/Cargo.lock * Add changelog entry * Cleanup (naming & comments) * Rename check_trusted_header() -> check_header_validator_set() * Rename check_misbehaviour_header() -> check_header_and_validator_set() * Rename MisbehaviourConsensusStateTimestampGteTrustingPeriod -> ConsensusStateTimestampGteTrustingPeriod * Rename verify_misbehaviour_header_commit() -> verify_header_commit_against_trusted() * Remove redundant client state expired check * Impl Protobuf conversions for mock Misbehaviour * Impl check_misbehaviour_and_update_state() for mock Misbehaviour * Remove cargo patches * Fixes after tendermint-rs bump * Fix typo * Add tests * MockClientState::with_frozen_height() * Provide MockContext helper to set client chain-id * Conversions from HostBlock -> TmLightBlock -> TmHeader * Fix tests * Clippy fix * Cleanup tests * Add comments for tests * Clippy fix * Rustfmt * Fix wrong main branch name in code coverage job (#280) * implement `ValidationContext` for `MockContext` * conn_open_init: test validate() * Add `execute` entrypoint * re-export validate and execute * test validate() in connection handlers * Use into() instead of ContextError directly * reexport ContextError * fmt Co-authored-by: Romain Ruetschi <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

plafer added 9 commits November 22, 2022 12:14

ConnOpenInit::validate

739c50f

Merge branch 'main' into plafer/251-validation-execution-ics3

79f17a8

conn_open_init: execute

31ecda0

conn_open_try validate and execute

5ad234e

conn_open_ack::validate

621adc5

conn_open_ack::execute

aedcb28

conn_open_confirm::validate

c6cf9f2

conn_open_confirm::execute

43009dc

changelog

a39cfde

plafer requested a review from hu55a1n1 November 22, 2022 22:38

plafer marked this pull request as ready for review November 22, 2022 22:38

hu55a1n1 reviewed Nov 28, 2022

View reviewed changes

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs Outdated Show resolved Hide resolved

plafer mentioned this pull request Nov 29, 2022

Avoid code duplication in implementation of ValidationContext and ExecutionContext #271

Closed

plafer added 10 commits November 29, 2022 16:09

LocalVars

f3620f2

validate_impl and execute_impl

52fa8fd

Remove useless clone

d28b6ae

Merge remote-tracking branch 'origin/main' into plafer/251-validation…

73c7393

…-execution-ics3

fix ConnOpenInit::validate

feffb7e

fix conn_open_ack (as in #272)

6d36e15

conn_open_try: LocalVars

08bf05a

conn_open_try validate/execute impl

e711a9a

conn_open_ack LocalVars

7556e4c

conn_open_ack impl

14fa3f8

plafer mentioned this pull request Dec 2, 2022

Remove legacy Readers and Keepers #279

Closed

romac and others added 3 commits December 5, 2022 10:34

Track code coverage with cargo-llvm-cov and codecov.io (#277)

f13f480

* Track code coverage with `cargo-llvm-cov` * Add changelog entry * Add coverage badge to the README

Fix wrong main branch name in code coverage job (#280)

18b9006

plafer changed the base branch from main to v0.21.x December 5, 2022 15:48

plafer changed the base branch from v0.21.x to main December 5, 2022 15:48

plafer added 7 commits December 5, 2022 10:49

Merge branch 'main' into plafer/251-validation-execution-ics3

dc55479

implement ValidationContext for MockContext

b9f3045

conn_open_init: test validate()

9e66deb

Add execute entrypoint

b81df87

re-export validate and execute

6dc1eb7

test validate() in connection handlers

01673ae

Merge branch 'main' into plafer/251-validation-execution-ics3

5fd3661

hu55a1n1 approved these changes Dec 6, 2022

View reviewed changes

plafer added 3 commits December 6, 2022 14:54

Use into() instead of ContextError directly

093cc9e

reexport ContextError

a8eeff0

fmt

b995322

plafer merged commit 691b27f into main Dec 6, 2022

plafer deleted the plafer/251-validation-execution-ics3 branch December 6, 2022 20:33

Farhad-Shabani mentioned this pull request Mar 16, 2023

ConnectionOpenTry: double-check check_client_consensus_height impl #167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ValidationContext and ExecutionContext for connections (ICS-3) #257

Implement ValidationContext and ExecutionContext for connections (ICS-3) #257

plafer commented Nov 22, 2022 •

edited

Loading

plafer commented Dec 2, 2022

codecov bot commented Dec 5, 2022 •

edited

Loading

hu55a1n1 left a comment

hu55a1n1 Dec 6, 2022

plafer Dec 6, 2022

hu55a1n1 Dec 6, 2022

plafer Dec 6, 2022

hu55a1n1 Dec 6, 2022

plafer Dec 6, 2022

hu55a1n1 Dec 6, 2022

plafer Dec 6, 2022

-        return Err(ContextError::ConnectionError(
-            ConnectionError::ConnectionMismatch {
-                connection_id: msg.conn_id_on_b.clone(),
-            },
-        ));
+        return Err(ConnectionError::InvalidConsensusHeight {
+            target_height: msg.consensus_height_of_a_on_b,
+            current_height: host_height,
+        }
+        .into());

Implement ValidationContext and ExecutionContext for connections (ICS-3) #257

Implement ValidationContext and ExecutionContext for connections (ICS-3) #257

Conversation

plafer commented Nov 22, 2022 • edited Loading

PR author checklist:

Reviewer checklist:

plafer commented Dec 2, 2022

codecov bot commented Dec 5, 2022 • edited Loading

Codecov Report

hu55a1n1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer commented Nov 22, 2022 •

edited

Loading

codecov bot commented Dec 5, 2022 •

edited

Loading