Skip to content
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

original timeoutTimestamp value in forwarding #1163

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

sangier
Copy link
Contributor

@sangier sangier commented Nov 7, 2024

This PR is a spec adjustment following the discussion with @chatton and @AdityaSripal regarding the usage of the original packet timeout instead of currentTimet() + DefaultHopTimeoutPeriod in the forwarded packet.

@@ -456,7 +459,7 @@ function onRecvPacket(

packetSequence=handler.sendPacket(
forwarding.hops[0].channelId,
currentTime() + DefaultHopTimeoutPeriod,
originalTimeoutTimestamp,
forwardingPayload
)
// store previous packet sequence and destChannelId for future sending ack
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One open question is: when we to store the forwarded packet info, is the payload.destPort really necessary?

I added it thinking that the payload.destPort may be slightly different for different ecosystems. @chatton make me notice it may be always equal instead.
If that's the case then we can just reuse the payload.destPort in the
function revertInFlightChanges( sentPacketDestChannelId: bytes, sentPacketPayload: Payload, receivedPacketDestChannelId: bytes, receivedPacketDestPort: bytes,

and thus reduce its input parameter by deleting the receivedPacketDestPort.

What's your opinion here @AdityaSripal ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be different across ecosystems. In fact there are already transfer modules in cosmos that do not use the transfer port :)

Thus it is correct as it is.

@AdityaSripal AdityaSripal merged commit ccc522a into feat/v2-spec Nov 7, 2024
2 checks passed
@AdityaSripal AdityaSripal deleted the stefano/app-data-timeout branch November 7, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants