feat: handler for ChanUpgradeTry
#3628
Conversation
…e-upgrade-type-and-validate-basic-functions
…e-basic-functions' into cian/issue#3451-add-verifiable-upgrade-type-and-validate-basic-functions
…it-to-use-new-upgrade-type' into charly/#3448-modify-upgrade-try
…init-to-use-new-upgrade-type' into charly/#3448-modify-upgrade-try
…init-to-use-new-upgrade-type' into charly/#3448-modify-upgrade-try
…ype' of https://github.com/cosmos/ibc-go into cian/issue#3447-modify-upgradeinit-to-use-new-upgrade-type
| if !found { | ||
| return fmt.Errorf("could not find upgrade for channel %s", endpoint.ChannelID) | ||
| } |
There was a problem hiding this comment.
This is unreachable since it will fail on line 611, maybe you can add the error string to line 611
| proposedConnectionHops = []string{path.EndpointB.ConnectionID} | ||
| upgrade := types.NewUpgrade( | ||
| types.NewUpgradeFields( | ||
| types.UNORDERED, proposedConnectionHops, fmt.Sprintf("%s-v2", mock.Version), | ||
| ), | ||
| types.NewTimeout(path.EndpointA.Chain.GetTimeoutHeight(), 0), | ||
| 0, | ||
| ) |
There was a problem hiding this comment.
is it possible to use path.EndpointB.GetProposedUpgrade()?
| proposedConnectionHops := []string{path.EndpointB.ConnectionID} | ||
| upgrade = types.NewUpgrade( | ||
| types.NewUpgradeFields( | ||
| types.UNORDERED, proposedConnectionHops, fmt.Sprintf("%s-v2", mock.Version), | ||
| ), | ||
| types.NewTimeout(path.EndpointA.Chain.GetTimeoutHeight(), 0), | ||
| 0, | ||
| ) |
| path = ibctesting.NewPath(suite.chainA, suite.chainB) | ||
| suite.coordinator.Setup(path) | ||
|
|
||
| path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v2", mock.Version) |
There was a problem hiding this comment.
if we use path.EndpointB.GetProposedUpgrade() below then here we can assign the upgrade version for both endpoints instead of just endpointA
There was a problem hiding this comment.
fmt.Sprintf("%s-v2", mock.Version" can be replaced with mock.UpgradeVersion
There was a problem hiding this comment.
haha we are on the same page #3628 (comment)
| return types.Upgrade{}, errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") | ||
| } | ||
|
|
||
| // make sure connection is OPEN |
There was a problem hiding this comment.
nit: I feel like this comment doesn't really add much value, I prefer to avoid inline comments which are self explanatory, this part of the code is self documenting and pretty obvious whats happening
| ) | ||
| } | ||
|
|
||
| // check if packet is timed out on the receiving chain |
There was a problem hiding this comment.
ditto re. this comment - this is also incorrect as it mentions packet but I understand this piece of timeout code will change later with the new Timeout API
| true, | ||
| }, | ||
| // { | ||
| // "success with later upgrade sequence", |
There was a problem hiding this comment.
I'm not sure I follow. If I uncomment the test it passes
| @@ -67,21 +68,69 @@ func (k Keeper) WriteUpgradeInitChannel(ctx sdk.Context, portID, channelID strin | |||
| emitChannelUpgradeInitEvent(ctx, portID, channelID, currentChannel, upgrade) | |||
| } | |||
|
|
|||
| // upgradeTry | |||
| // ChanUpgradeTry is called by a module to accept the first step of a channel upgrade handshake initiated by | |||
| // a module on another chain. If this function is successful, the proposed upgrade will be returned. If the upgrade fails, the upgrade sequence will still be incremented but an error will be returned. | |||
| func (k Keeper) ChanUpgradeTry( | |||
| ctx sdk.Context, | |||
| portID, | |||
| channelID string, | |||
| proposedConnectionHops []string, | |||
There was a problem hiding this comment.
@AdityaSripal is it intentional that the TRY step must use the same version as the counterparty? I don't see a mention in the spec for this reasoning. There's no check in startFlushUpgradeHandshake for this (potentially because we automatically use the counterparty upgrade version, but there is a check for the ordering being the same (despite the fact we also use the counterparty upgrade ordering), so there's an inconsistency in construction
yup! |
|
I will do followup testing nits. In parallel we can start work on the rest of this handler, but lets open a pr immediately once work is started on it, so we can work together on it (since it is blocking the next set of prs) |
This PR introduces the ChanUpgradeTry handler.
closes: #3739
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.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.