ICS4: Implement Packet Sequence Logic for upgrades #1062
Conversation
AdityaSripal
requested review from
mpoke,
cwgoes,
colin-axner and
angbrav
as code owners
There was a problem hiding this comment.
Thanks, @AdityaSripal. I left a bunch of naming suggestions that, I think, align closer with existing naming.
Also, shouldn't we do privateStore.set(counterpartyNext... with counterpartyUpgrade.nextPacketSend in chanUpgradeAck and chanUpgradeConfirm?
Co-authored-by: Carlos Rodriguez <[email protected]>
| // the recv and ack sequences appropriately | ||
| if channel.order == "UNORDERED" && upgrade.fields.ordering == "ORDERED" { | ||
| selfNextSequenceSend = provableStore.get(nextSequenceSendPath(portIdentifier, channelIdentifier)) | ||
| counterpartyNextSequenceSend = privateStore.get(counterpartyNextPacketSendSequencePath(portIdentifier, channelIdentifier)) |
There was a problem hiding this comment.
where do we set this though? (Ah, Carlos mentioned this in top comment #1062 (review))
There was a problem hiding this comment.
above, it says to be set in the flushing handler but im not sure how that is possible... can we just fetch it from the stored counterparty upgrade?
There was a problem hiding this comment.
that is what I was under the impression would happen 😅. We get it from counterparty upgrade and set it in store in openUpgradeHandshake (though the setting in store might be an implementation detail that shouldn't belong in spec) in order to use it after upgrade it complete.
| #### CounterpartyNextSequenceSend Path | ||
|
|
||
| The chain must store the counterparty's next sequence send on `startFlushUpgradeHandshake`. This will be stored in the `counterpartyNextSequenceSend` path on the private store. | ||
|
|
There was a problem hiding this comment.
is it possible to access the counterparty upgrade information for example OnChanUpgradeTry? I think the full counterparty upgrade is not set until WriteUpgradeAck / WriteUpgradeConfirm, at this point we only have the upgrade fields from the counterparty, not ie: next packet send
There was a problem hiding this comment.
You're right, I think the spec is missing the call to privateStore.set(counterpartyNext... with counterpartyUpgrade.nextPacketSend that should happen in chanUpgradeAck and chanUpgradeConfirm. I mentioned that to @AdityaSripal this morning and he was going to look into it.
There was a problem hiding this comment.
and we're also missing that key counterpartyNext... alltogether in the implementation. Adding it now.
There was a problem hiding this comment.
You're correct. Thanks, added the counterparty set now to ack and confirm
There was a problem hiding this comment.
Everything clicks for me! Thank you @AdityaSripal and all you guys for catching the issues! 💪
Closes: #1060
Closes: #1061