-
Notifications
You must be signed in to change notification settings - Fork 586
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.
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
update the Order
check ValidateUpgradeFields
util method to allow for all supported Order
types
#3635
Comments
9 tasks
Partially addressed by removing dead code in #3634. We should add the following in a follow up PR to ensure the order is supported by the underlying connection. ref getVersions := connectionEnd.GetVersions()
if len(getVersions) != 1 {
return "", nil, errorsmod.Wrapf(
connectiontypes.ErrInvalidVersion,
"single version must be negotiated on connection before opening channel, got: %v",
getVersions,
)
}
if !connectiontypes.VerifySupportedFeature(getVersions[0], order.String()) {
return "", nil, errorsmod.Wrapf(
connectiontypes.ErrInvalidVersion,
"connection version %s does not support channel ordering: %s",
getVersions[0], order.String(),
)
} This should probably be reflected in the spec also if not already. cc @AdityaSripal |
9 tasks
charleenfei
added a commit
that referenced
this issue
Jun 7, 2023
…ub.com:cosmos/ibc-go into charly/#3635-order-check-validateupgradefields
Closed by #3774. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
In a previous version of the upgradability spec, a channel upgrade could only update the
Ordering
of the channel within a subset of the existing ordering (ie:Unordered
channels could not be upgraded toOrdered
, whereasOrdered
could be updated to bothUnordered
andOrdered
.) However, because the latest iteration of the spec allows for packets to finish their lifecycles, we can now upgrade channel ordering to any type of ordering, provided it is supported by the connection.The util method for validating upgrade fields should reflect this.
For Admin Use
The text was updated successfully, but these errors were encountered: