Implement openUpgradeHandshake subprotocol #3759

DimitrisJim · 2023-06-05T21:14:43Z

Description

Add the openUpgradeHandshake sub protocol (renamed to WriteUpgradeOpenChannel as per the comments in the issue.

closes: #3644

Commit Message / Changelog Entry

Implement openUpgradeHandshake subprotocol

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

modules/core/04-channel/keeper/keeper.go

DimitrisJim · 2023-06-05T21:23:32Z

modules/core/04-channel/keeper/upgrade.go

+// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel fields. This can be called in one of two cases:
+// - In the UpgradeAck step of the handshake both sides have already flushed any in-flight packets.
+// - In the UpgradeOpen step of the handshake.
+func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) {


Question. It would be nice if we could get information about who called us (Ack or Open) since that can be used in the error messages and when incrementing the telemetry counter. Would a bool (or enum :^)) be the preferred approach?

I don't think we can use the previousState here to make assumptions about who called us but I haven't thought too hard about it yet.

why can't we use the previousState?

If my understanding of the state transitions are correct (ref recent PR https://github.com/cosmos/ibc/pull/978/files). We can end up in the subprotocol from both open and from ack with channel state being ACKUPGRADE for channel A.

are you talking about the case where this subprotocol is being called on chain B, where chain A has automatically moved from ACK -> OPEN bc the pending in flight packets check results in an automatic chan OPEN?

if so, i think we could then still use this because the previousState of the chain B channel should be accurate.

wouldn't B be in TRYUPGRADE?

To be a bit more verbose about my confusion, during ChanUpgradeAck and after startFlushUpgradeHandshake the states of A and B will be (ACKUPGRADE, TRYUPGRADE) (referencing the PR linked in the previous comment). If at this point both A and B have finished flushing, openUpgradeHandShake will be called with those two states for A and B.

OTOH, if they have not finished flushing, after they do, the relayer will submit the ChanUpgradeClose messages to both A and B and the states, having not changed, will still be (ACKUPGRADE, TRYUPGRADE).

I feel I'm missing something here but can't see it yet. Either way, this doesn't affect the core logic, its mostly for the telemetry counter and the err messages.

modules/core/04-channel/keeper/upgrade.go

charleenfei · 2023-06-06T14:28:40Z

modules/core/04-channel/keeper/upgrade.go

+// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel fields. This can be called in one of two cases:
+// - In the UpgradeAck step of the handshake both sides have already flushed any in-flight packets.
+// - In the UpgradeOpen step of the handshake.
+func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) {


why can't we use the previousState?

colin-axner

Nice! LGTM

damiannolan · 2023-06-07T11:03:24Z

modules/core/04-channel/keeper/upgrade.go

+// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel, sets the channel flush status to `NOTINFLUSH` and sets the channel state back to `OPEN`. This can be called in one of two cases:
+// - In the UpgradeAck step of the handshake if both sides have already flushed all in-flight packets.
+// - In the UpgradeOpen step of the handshake.
+func (k Keeper) writeUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) {


will this method not be called from the core msg server and need to be unexported?

can be updated when its used

damiannolan

LGTM

damiannolan · 2023-06-07T11:04:41Z

modules/core/04-channel/keeper/upgrade.go

+// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel, sets the channel flush status to `NOTINFLUSH` and sets the channel state back to `OPEN`. This can be called in one of two cases:
+// - In the UpgradeAck step of the handshake if both sides have already flushed all in-flight packets.
+// - In the UpgradeOpen step of the handshake.
+func (k Keeper) writeUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) {


can be updated when its used

AdityaSripal

One note is that I didn't use checks in spec since they would be duplicated. So in spec it is caller responsibility. Up to implementation team whether it makes sense to have checks in the function or not

Co-authored-by: Charly <[email protected]>

…ehandshake-subprotocol

crodriguezvega

Nice, @DimitrisJim!

modules/core/04-channel/keeper/upgrade.go

Co-authored-by: Carlos Rodriguez <[email protected]>

…ehandshake-subprotocol

DimitrisJim requested review from colin-axner, AdityaSripal and damiannolan as code owners June 5, 2023 21:14

DimitrisJim commented Jun 5, 2023

View reviewed changes

modules/core/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved

DimitrisJim commented Jun 5, 2023

View reviewed changes

charleenfei reviewed Jun 6, 2023

View reviewed changes

DimitrisJim force-pushed the jim/3644-implement-openupgradehandshake-subprotocol branch from a41350e to d394e0a Compare June 6, 2023 14:52

colin-axner approved these changes Jun 7, 2023

View reviewed changes

damiannolan reviewed Jun 7, 2023

View reviewed changes

damiannolan approved these changes Jun 7, 2023

View reviewed changes

DimitrisJim mentioned this pull request Jun 7, 2023

Add deleteUpgrade and deleteCounterpartyLastPacketSequence to keeper. #3780

Merged

9 tasks

charleenfei approved these changes Jun 7, 2023

View reviewed changes

AdityaSripal approved these changes Jun 7, 2023

View reviewed changes

DimitrisJim and others added 3 commits June 8, 2023 11:39

Add WriteUpgradeOpenChannel.

ddb9a9f

Apply suggestions from code review

9ce065d

Co-authored-by: Charly <[email protected]>

Add wrapper function to exported_test.go

563548d

DimitrisJim force-pushed the jim/3644-implement-openupgradehandshake-subprotocol branch from 0f5c081 to 563548d Compare June 8, 2023 08:41

Merge branch '04-channel-upgrades' into jim/3644-implement-openupgrad…

76d0ee6

…ehandshake-subprotocol

crodriguezvega reviewed Jun 11, 2023

View reviewed changes

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved

DimitrisJim and others added 3 commits June 12, 2023 00:32

Update modules/core/04-channel/keeper/upgrade.go

f0a9df7

Co-authored-by: Carlos Rodriguez <[email protected]>

Merge branch '04-channel-upgrades' into jim/3644-implement-openupgrad…

6536755

…ehandshake-subprotocol

Merge branch '04-channel-upgrades' into jim/3644-implement-openupgrad…

7a464bc

…ehandshake-subprotocol

DimitrisJim merged commit 44ed96a into 04-channel-upgrades Jun 13, 2023

DimitrisJim deleted the jim/3644-implement-openupgradehandshake-subprotocol branch June 13, 2023 13:04

DimitrisJim mentioned this pull request Jun 14, 2023

Implement openUpgradeHandshake subprotocol #3644

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement openUpgradeHandshake subprotocol #3759

Implement openUpgradeHandshake subprotocol #3759

DimitrisJim commented Jun 5, 2023

DimitrisJim Jun 5, 2023

charleenfei Jun 6, 2023

DimitrisJim Jun 6, 2023 •

edited

Loading

charleenfei Jun 6, 2023

charleenfei Jun 6, 2023

DimitrisJim Jun 8, 2023 •

edited

Loading

charleenfei Jun 6, 2023

colin-axner left a comment

damiannolan Jun 7, 2023

damiannolan Jun 7, 2023

damiannolan left a comment

damiannolan Jun 7, 2023

AdityaSripal left a comment

crodriguezvega left a comment

Implement openUpgradeHandshake subprotocol #3759

Implement openUpgradeHandshake subprotocol #3759

Conversation

DimitrisJim commented Jun 5, 2023

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

DimitrisJim Jun 6, 2023 •

edited

Loading

DimitrisJim Jun 8, 2023 •

edited

Loading