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.

Sign up for GitHub

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

Jump to bottom

Remove crossing hellos from Channel #181

Merged
merged 25 commits into from
Oct 24, 2022
Merged

Remove crossing hellos from Channel #181

merged 25 commits into from
Oct 24, 2022

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Oct 14, 2022 •
edited
Loading

Closes: #157

Note that #184 was opened to resolve the current awkward and inconsistent strategy to return values from our handlers/callbacks.

Description

Still todo:

  • cleanup channel_dispatch() and channel_callback()
    • it currently works but is messy
  • Remove ModuleOutputBuilder argument from all channel callbacks
  • Remove crossing hellos logic from chan_open_try::process()
  • Deprecate MsgChannelOpenTry::previous_channel_id field
  • Modify ICS-20 implementation to work with new handshake protocol

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

plafer and others added 22 commits October 13, 2022 15:29
@plafer
variable naming in ics26
ea27a92
@plafer
Merge remote-tracking branch 'origin/main' into plafer/157-crossing-h…
05dfe7b 
…ellos-channel
@plafer
Rework ics26 channel handler
f63418f
@plafer
cleanup Ics4ChannelMsg handler
a83a436
@plafer
channel callbacks now return ModuleExtras
1e7a06f
@plafer
channel_dispatch now returns a MsgReceipt
ff1ed77
@plafer
remove crossing hellos logic from chan_open_try
1f94dbd 
Also removes tests that tested that logic specifically
@plafer
deprecate MsgChannelOpenTry::previous_channel_id field
c936676
@plafer
ics20: refactor on_chan_open_init
6bcd70f
@plafer
Remove version field of on_chan_open_try()
dd7018c
@plafer
ICS20: inline on_chan_open_init
652a629
@plafer
ics20: on_chan_open_try
14a6a2b
@plafer
ics20: refactor rest of handshake callbacks
e90e6f8
@plafer
fmt
c0fbdfb
@kevinji @hu55a1n1 @plafer
ics26_routing: Derive Hash for ModuleId (#179)
28ff2a4 
* 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]>
@plafer
Release v0.20.0 (#186)
4e7b3eb 
* 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
@plafer @hu55a1n1
Connection proofs refactor (#170)
040e808 
* 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]>
@plafer
Merge branch 'main' into plafer/157-crossing-hellos-channel
ee5447e
@plafer
changelog
69b6caa
@plafer
ics20 on_chan_open_init tests
4192585
@plafer
ics20 on_chan_open_try tests
905a218
@plafer
update deprecation versions
d37021b
@plafer plafer mentioned this pull request Oct 21, 2022
Open
@plafer plafer requested a review from romac October 21, 2022 15:49
@plafer plafer marked this pull request as ready for review October 21, 2022 15:49
romac
romac approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Left a nitpick but looks good otherwise!

crates/ibc/src/mock/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Outdated
version: &Version,
) -> Result<(ModuleExtras, Version), Error> {
Ok((
ModuleExtras {
Copy link
Member

Choose a reason for hiding this comment

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

We could use a Default instance here, or an empty() constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

516f520 (fixes all)

crates/ibc/src/core/ics26_routing/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics26_routing/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics26_routing/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics26_routing/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Outdated Show resolved Hide resolved
@plafer plafer merged commit 1feb771 into main Oct 24, 2022
@plafer plafer deleted the plafer/157-crossing-hellos-channel branch October 24, 2022 16:20
shuoer86 pushed a commit to shuoer86/ibc-rs that referenced this pull request Nov 4, 2023
@kevinji
chore: avoid cloning and use .map/.entry in Rust code (informalsy…
9f86b67 
…stems#181)
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
@plafer @kevinji @hu55a1n1
Remove crossing hellos from Channel (#181)
7e49b26 
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove "crossing hellos" logic from channel handshake
3 participants
@plafer @romac @kevinji