-
Notifications
You must be signed in to change notification settings - Fork 385
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
ICS4: Simplify sendPacket interface #731
Conversation
If the changes are approved, I'll create different PRs for handling the tasks in the description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, makes sense to me (and I don't think this leads to any loss of functionality at the application layer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement! 🙌
Will a changelog be added to the repo soon? I fear we will forget to apply some of these changes in ibc-go if they aren't logged somewhere
@colin-axner That's a good point. @AdityaSripal @crodriguezvega what do you think? |
function sendPacket( | ||
capability: CapabilityKey, | ||
sourcePort: Identifier, | ||
sourceChannel: Identifier, | ||
timeoutHeight: Height, | ||
timeoutTimestamp: uint64, | ||
data: bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I am just noticing this now, but I think the packet sequence should be returned. This will be useful for UX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin-axner Could you please open an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #708
Before merging update the calls to sendPacket throughout the standard, i.e.,
createOutgoingPacket
(already inconsistent with ICS-4, see ICS20: Update createOutgoingPacket name, signature and sendPacket logic #709)SendTx
(already inconsistent with ICS-4)