Add ChanUpgradeOpen core handler. #3844
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -302,6 +302,132 @@ func (k Keeper) WriteUpgradeAckChannel( | |
| emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, upgrade) | ||
| } | ||
|
|
||
| // ChanUpgradeOpen is called by a module to complete the channel upgrade handshake and move the channel back to an OPEN state. | ||
| // This method should only be called after both channels have flushed any in-flight packets. | ||
| // This method should only be called directly by the core IBC message server. | ||
| func (k Keeper) ChanUpgradeOpen( | ||
| ctx sdk.Context, | ||
| portID, | ||
| channelID string, | ||
| counterpartyChannelState types.State, | ||
| proofChannel []byte, | ||
| proofHeight clienttypes.Height, | ||
| ) error { | ||
| if k.hasInflightPackets(ctx, portID, channelID) { | ||
| return errorsmod.Wrapf(types.ErrPendingInflightPackets, "port ID (%s) channel ID (%s)", portID, channelID) | ||
| } | ||
|
|
||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
| if !found { | ||
| return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) | ||
| } | ||
|
|
||
| if !collections.Contains(channel.State, []types.State{types.TRYUPGRADE, types.ACKUPGRADE}) { | ||
| return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.TRYUPGRADE, types.ACKUPGRADE, channel.State) | ||
| } | ||
|
|
||
| if channel.FlushStatus != types.FLUSHCOMPLETE { | ||
| return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected %s, got %s", types.FLUSHCOMPLETE, channel.FlushStatus) | ||
| } | ||
|
|
||
| connection, err := k.GetConnection(ctx, channel.ConnectionHops[0]) | ||
| if err != nil { | ||
| return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") | ||
| } | ||
|
|
||
| if connection.GetState() != int32(connectiontypes.OPEN) { | ||
| return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String()) | ||
| } | ||
|
|
||
| var counterpartyChannel types.Channel | ||
| switch counterpartyChannelState { | ||
| case types.OPEN: | ||
| upgrade, found := k.GetUpgrade(ctx, portID, channelID) | ||
| if !found { | ||
| return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) | ||
| } | ||
|
|
||
| counterpartyChannel = types.Channel{ | ||
| State: types.OPEN, | ||
| Ordering: upgrade.Fields.Ordering, | ||
| ConnectionHops: upgrade.Fields.ConnectionHops, | ||
| Counterparty: types.NewCounterparty(portID, channelID), | ||
| Version: upgrade.Fields.Version, | ||
| UpgradeSequence: channel.UpgradeSequence, | ||
| FlushStatus: types.NOTINFLUSH, | ||
| } | ||
|
|
||
| case types.TRYUPGRADE: | ||
| // If the counterparty is in TRYUPGRADE, then we must have gone through the ACKUPGRADE step. | ||
| if channel.State != types.ACKUPGRADE { | ||
| return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected %s, got %s", types.ACKUPGRADE, channel.State) | ||
| } | ||
|
|
||
| counterpartyChannel = types.Channel{ | ||
| State: types.TRYUPGRADE, | ||
| Ordering: channel.Ordering, | ||
| ConnectionHops: []string{connection.GetCounterparty().GetConnectionID()}, | ||
| Counterparty: types.NewCounterparty(portID, channelID), | ||
| Version: channel.Version, | ||
| UpgradeSequence: channel.UpgradeSequence, | ||
| FlushStatus: types.FLUSHCOMPLETE, | ||
| } | ||
|
|
||
| case types.ACKUPGRADE: | ||
| counterpartyChannel = types.Channel{ | ||
| State: types.ACKUPGRADE, | ||
| Ordering: channel.Ordering, | ||
| ConnectionHops: []string{connection.GetCounterparty().GetConnectionID()}, | ||
| Counterparty: types.NewCounterparty(portID, channelID), | ||
| Version: channel.Version, | ||
| UpgradeSequence: channel.UpgradeSequence, | ||
| FlushStatus: types.FLUSHCOMPLETE, | ||
| } | ||
|
|
||
| default: | ||
| panic(fmt.Sprintf("counterparty channel state should be in one of [%s, %s, %s]; got %s", types.TRYUPGRADE, types.ACKUPGRADE, types.OPEN, counterpartyChannelState)) | ||
| } | ||
|
|
||
| err = k.connectionKeeper.VerifyChannelState(ctx, connection, proofHeight, proofChannel, portID, channelID, counterpartyChannel) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(err, "failed to verify counterparty channel, expected counterparty channel state: %s", counterpartyChannel.String()) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // 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) { | ||
|
There was a problem hiding this comment. this will need to be exported if we are calling it from the message server layer right? There was a problem hiding this comment. yess! Will do so in #3895 to keep things relevant There was a problem hiding this comment. do we need unit tests for this write function? There was a problem hiding this comment. Yeah, guess the idea for Ack was test since there's the conditional there that leads to two different states. I do think we're lacking test cov on some checks in the write functions for the other handlers so I'd definitely be in favour of testing the rest in a follow up. |
||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
| if !found { | ||
| panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID)) | ||
| } | ||
|
|
||
| upgrade, found := k.GetUpgrade(ctx, portID, channelID) | ||
| if !found { | ||
| panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID)) | ||
| } | ||
|
|
||
| // Switch channel fields to upgrade fields and set channel state to OPEN | ||
| previousState := channel.State | ||
| channel.Ordering = upgrade.Fields.Ordering | ||
| channel.Version = upgrade.Fields.Version | ||
| channel.ConnectionHops = upgrade.Fields.ConnectionHops | ||
| channel.State = types.OPEN | ||
| channel.FlushStatus = types.NOTINFLUSH | ||
|
|
||
| k.SetChannel(ctx, portID, channelID, channel) | ||
|
|
||
| // Delete auxiliary state. | ||
| k.deleteUpgrade(ctx, portID, channelID) | ||
| k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) | ||
|
Comment on lines
+431
to
+432
There was a problem hiding this comment. potential candidate for another helper fn since it's used in |
||
|
|
||
| k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState.String(), "new-state", types.OPEN.String()) | ||
| emitChannelUpgradeOpenEvent(ctx, portID, channelID, channel) | ||
| } | ||
|
|
||
| // ChanUpgradeCancel is called by a module to cancel a channel upgrade that is in progress. | ||
| func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height) error { | ||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
|
|
@@ -597,38 +723,6 @@ func (k Keeper) startFlushUpgradeHandshake( | |
| return nil | ||
| } | ||
|
|
||
| // validateUpgradeFields validates the proposed upgrade fields against the existing channel. | ||
| // It returns an error if the following constraints are not met: | ||
| // - there exists at least one valid proposed change to the existing channel fields | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] these two sections are identical with the exception of expected counterparty state, maybe we could handle these in a single case?
Happy to leave this out for now though, it might make some of the conditionals a little harder to reason about.
As an alternative, we could maybe add a function
getExpectedCounterPartyChannel(currentState, ...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will remember to bring this up during review call, I'm thinking there's backing for keeping the switches separate to follow spec in structure.