ICS26: Add previous version to upgrade callbacks #806
Conversation
| @@ -86,7 +83,8 @@ function onChanUpgradeTry( | |||
| channelIdentifier: Identifier, | |||
| counterpartyPortIdentifier: Identifier, | |||
| counterpartyChannelIdentifier: Identifier, | |||
| counterpartyVersion: string) => (version: string, err: Error) { | |||
| counterpartyVersion: string, | |||
| previousVersion: string) => (version: string, err: Error) { | |||
There was a problem hiding this comment.
Should this be previousCounterpartyVersion?
There was a problem hiding this comment.
No, counterpartyVersion is the new version the counterparty is proposing to upgrade to
previousVersion is the current version of our chain before the upgrade. Maybe we should rename to current version?
There was a problem hiding this comment.
Ah okay, thanks, sounds good to me!
There was a problem hiding this comment.
I think previousVersion makes sense for naming, but maybe we could have previousVersion come before counterpartyVersion in the list of args? Would it makes more sense for the "self" to be stated before the counterparty. It doesn't make too much of a difference to me either way
| to the default version which MAY be a new default from when the channel was first initiated. | ||
| If there is no default version string for the application, | ||
| it should return an error if provided version is empty string. | ||
| The callback is provided both the previous version of the channel and the new proposed version. It may perform the necessary logic and state changes necessary to upgrade the channel from the previous version to the new version. If upgrading the application from the previous version to the new version is not supported, it must return an error. |
There was a problem hiding this comment.
So including the previous channel version is intended such that an application cannot jump versions??
For example: ics20-1 cannot be upgraded to ics20-3, is that correct?
There was a problem hiding this comment.
No, certainly some applications could choose to make that restriction
But perhaps ics20-1 -> ics20-3` requires state changes A
But ics20-2 -> ics20-3 require a different sequence of state changes B.
Knowing what version you are upgrading from gives you the information you need to decide if the upgrade is allowed and if allowed, what changes to make.
THis information could be stored in the app state itself, but we pass it in here to be explicit
| version: string, | ||
| previousVersion: string) => (version: string, err: Error) { |
There was a problem hiding this comment.
Thoughts on reordering these so that previousVersion comes before version?
No description provided.