Connection proofs refactor #170

plafer · 2022-10-12T00:27:42Z

Closes: #156
Closes: #165
Closes: #168

Description

Refactors the connection handlers to remove crossing hellos and refactor proof verification. Also fixes a bug I discovered while refactoring (#168).

Main choices made:

Proof verifications are now inline. I find that way easier to review and make sure that we're passing the right variables.
I chose not to create any special domain type for proofs, contrary to what's suggested in Proposal for redesigning parts of the ibc-rs modules' API #25. I didn't see the value in doing that, since we never pass all the proofs around. They're pretty much always together in the context of the msg (i.e. ConnectionOpen{Try,Ack,Confirm}), and I found "wrapping them" together in a new type superfluous.

PR author checklist:

Linked to GitHub issue.
Tagged one reviewer who will be the one responsible for shepherding this PR.
Updated code comments and documentation (e.g., docs/).
Added changelog entries, using unclog.
Added tests.
Choose a consistent variable naming convention (see Tracking issue: Handler proof verification refactor proposition #15)

Reviewer checklist:

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

Remove connection::verify.rs

hu55a1n1

I really like the idea of inlining the proofs and verification. 🙌
Since the PR partially implements crossing-hellos I suggest we don't merge it until either the remaining work is complete or the current work is released (to ease the release process).

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

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

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

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

crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs

crates/ibc/tests/mbt.rs

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

hu55a1n1

This is an awesome PR - it greatly improves readability and removes a lot of unwanted code! Thank you! 🙏 I've approved it but would recommend we don't merge it before the release.

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

Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]>

Remove redundant log and adjust logging level.

* conn_open_confirm refactor * conn_open_ack handler * disable mbt * ConnOpenConfirm handler Remove connection::verify.rs * conn_open_ack: readability * remove ConnectionOpenTry::with_previous_connection_id * Make previous_connection_id deprecated * remove mbt * Remove hold_oldest_height check * changelog * Output logs for handlers again * conn_open_ack: apply naming convention * conn_open_try apply naming convention * conn_open_init apply naming convention * conn_open_try naming fix * conn_open_confirm apply naming convention * Document naming convention * fmt * comments * Update crates/ibc/src/core/ics03_connection/handler.rs Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> * Move naming convention docstring * fix * update deprecated annotation Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

* variable naming in ics26 * Rework ics26 channel handler * cleanup Ics4ChannelMsg handler * channel callbacks now return ModuleExtras * channel_dispatch now returns a `MsgReceipt` * remove crossing hellos logic from chan_open_try Also removes tests that tested that logic specifically * deprecate `MsgChannelOpenTry::previous_channel_id` field * ics20: refactor on_chan_open_init * Remove `version` field of `on_chan_open_try()` * ICS20: inline on_chan_open_init * ics20: on_chan_open_try * ics20: refactor rest of handshake callbacks * fmt * ics26_routing: Derive Hash for ModuleId (#179) * ics26_routing: Derive Hash for ModuleId Signed-off-by: Kevin Ji <[email protected]> * Add changelog for #179 Signed-off-by: Kevin Ji <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]> * Release v0.20.0 (#186) * release v0.20.0 * bump crate version to 0.20.0 * update CONTRIBUTING * CONTRIBUTING change * Update ibc-proto-rs to v0.21.0 * Update CONTRIBUTING.md * Connection proofs refactor (#170) * conn_open_confirm refactor * conn_open_ack handler * disable mbt * ConnOpenConfirm handler Remove connection::verify.rs * conn_open_ack: readability * remove ConnectionOpenTry::with_previous_connection_id * Make previous_connection_id deprecated * remove mbt * Remove hold_oldest_height check * changelog * Output logs for handlers again * conn_open_ack: apply naming convention * conn_open_try apply naming convention * conn_open_init apply naming convention * conn_open_try naming fix * conn_open_confirm apply naming convention * Document naming convention * fmt * comments * Update crates/ibc/src/core/ics03_connection/handler.rs Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> * Move naming convention docstring * fix * update deprecated annotation Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]> * changelog * ics20 on_chan_open_init tests * ics20 on_chan_open_try tests * update deprecation versions * update comment with issue * fmt * use ModuleExtras::empty() Signed-off-by: Kevin Ji <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Kevin Ji <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

) Bumps [github.com/cosmos/gogoproto](https://github.com/cosmos/gogoproto) from 1.4.3 to 1.4.11. - [Release notes](https://github.com/cosmos/gogoproto/releases) - [Changelog](https://github.com/cosmos/gogoproto/blob/main/CHANGELOG.md) - [Commits](cosmos/gogoproto@v1.4.3...v1.4.11) --- updated-dependencies: - dependency-name: github.com/cosmos/gogoproto dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <[email protected]>

* conn_open_confirm refactor * conn_open_ack handler * disable mbt * ConnOpenConfirm handler Remove connection::verify.rs * conn_open_ack: readability * remove ConnectionOpenTry::with_previous_connection_id * Make previous_connection_id deprecated * remove mbt * Remove hold_oldest_height check * changelog * Output logs for handlers again * conn_open_ack: apply naming convention * conn_open_try apply naming convention * conn_open_init apply naming convention * conn_open_try naming fix * conn_open_confirm apply naming convention * Document naming convention * fmt * comments * Update crates/ibc/src/core/ics03_connection/handler.rs Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> * Move naming convention docstring * fix * update deprecated annotation Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

* variable naming in ics26 * Rework ics26 channel handler * cleanup Ics4ChannelMsg handler * channel callbacks now return ModuleExtras * channel_dispatch now returns a `MsgReceipt` * remove crossing hellos logic from chan_open_try Also removes tests that tested that logic specifically * deprecate `MsgChannelOpenTry::previous_channel_id` field * ics20: refactor on_chan_open_init * Remove `version` field of `on_chan_open_try()` * ICS20: inline on_chan_open_init * ics20: on_chan_open_try * ics20: refactor rest of handshake callbacks * fmt * ics26_routing: Derive Hash for ModuleId (#179) * ics26_routing: Derive Hash for ModuleId Signed-off-by: Kevin Ji <[email protected]> * Add changelog for #179 Signed-off-by: Kevin Ji <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]> * Release v0.20.0 (#186) * release v0.20.0 * bump crate version to 0.20.0 * update CONTRIBUTING * CONTRIBUTING change * Update ibc-proto-rs to v0.21.0 * Update CONTRIBUTING.md * Connection proofs refactor (#170) * conn_open_confirm refactor * conn_open_ack handler * disable mbt * ConnOpenConfirm handler Remove connection::verify.rs * conn_open_ack: readability * remove ConnectionOpenTry::with_previous_connection_id * Make previous_connection_id deprecated * remove mbt * Remove hold_oldest_height check * changelog * Output logs for handlers again * conn_open_ack: apply naming convention * conn_open_try apply naming convention * conn_open_init apply naming convention * conn_open_try naming fix * conn_open_confirm apply naming convention * Document naming convention * fmt * comments * Update crates/ibc/src/core/ics03_connection/handler.rs Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> * Move naming convention docstring * fix * update deprecated annotation Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]> * changelog * ics20 on_chan_open_init tests * ics20 on_chan_open_try tests * update deprecation versions * update comment with issue * fmt * use ModuleExtras::empty() Signed-off-by: Kevin Ji <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]> Co-authored-by: Kevin Ji <[email protected]> Co-authored-by: Shoaib Ahmed <[email protected]>

plafer added 4 commits October 11, 2022 19:16

conn_open_confirm refactor

493d644

conn_open_ack handler

c53e2d7

disable mbt

4534af3

ConnOpenConfirm handler

22d3f73

Remove connection::verify.rs

plafer requested a review from hu55a1n1 October 12, 2022 00:27

hu55a1n1 reviewed Oct 12, 2022

View reviewed changes

plafer added 15 commits October 12, 2022 13:06

conn_open_ack: readability

53b6e90

remove ConnectionOpenTry::with_previous_connection_id

253780a

Make previous_connection_id deprecated

192f903

remove mbt

a799452

Remove hold_oldest_height check

25e58a6

changelog

f9aface

Output logs for handlers again

6931db8

conn_open_ack: apply naming convention

73cf502

conn_open_try apply naming convention

858c5cf

conn_open_init apply naming convention

0c6839b

conn_open_try naming fix

25c658d

conn_open_confirm apply naming convention

a491731

Document naming convention

e549ebe

fmt

b32c229

comments

7ab967b

hu55a1n1 approved these changes Oct 14, 2022

View reviewed changes

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

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

plafer and others added 2 commits October 14, 2022 08:28

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

72916f6

Co-authored-by: Shoaib Ahmed <[email protected]> Signed-off-by: Philippe Laferrière <[email protected]>

Move naming convention docstring

f6ce907

livelybug added a commit to octopus-network/ibc-rs that referenced this pull request Oct 14, 2022

Merge pull request cosmos#170 from octopus-network/feature/beefy-m2

786f119

Remove redundant log and adjust logging level.

plafer added 2 commits October 14, 2022 09:05

fix

9c9b8a9

update deprecated annotation

2e69024

plafer merged commit 8c49e7b into main Oct 19, 2022

plafer deleted the plafer/165-connection-proofs-refactor branch October 19, 2022 17:22

plafer mentioned this pull request Jan 5, 2023

Refactor packet handler proofs #323

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection proofs refactor #170

Connection proofs refactor #170

plafer commented Oct 12, 2022 •

edited

Loading

hu55a1n1 left a comment

hu55a1n1 left a comment

Connection proofs refactor #170

Connection proofs refactor #170

Conversation

plafer commented Oct 12, 2022 • edited Loading

Description

PR author checklist:

Reviewer checklist:

hu55a1n1 left a comment

Choose a reason for hiding this comment

hu55a1n1 left a comment

Choose a reason for hiding this comment

plafer commented Oct 12, 2022 •

edited

Loading