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

ICS25: Packet Handling Interface #5083

Closed
mossid opened this issue Sep 20, 2019 · 2 comments
Closed

ICS25: Packet Handling Interface #5083

mossid opened this issue Sep 20, 2019 · 2 comments

Comments

@mossid
Copy link
Contributor

mossid commented Sep 20, 2019

Packet handling functions sendPacket and recvPacket are defined under Packet relay in ICS25.

(Other interface methods (excluding this one and misbehavior) can be safely implemented as simple message-handler architecture without effecting application writer's UX).

sendPacket

sendPacket is currently implemented as channel.Manager.Send(ctx sdk.Context, portid, chanid string, packet Packet) error or channel.Port.Send(ctx sdk.Context, chanid string, packet Packet). The function is not directly exposed to the user interface - there is no message type or handler function that processes packet sending request. The method is intended to be called by the application modules, and they have to implement the message type and handler.

The interface to sending packet is different from local method calling. Transferring coins between two accounts will invoke two functions sequentially:

func SendCoins(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins) {
  bankKeeper.SubtractCoins(ctx, from, amount)
  bankKeeper.AddCoins(ctx, to, amount)
}

but transferring coins between two accounts on two chains will invoke two functions in different locations in the codebase, and not calling the method of the keeper itself:

// in sending function
bankKeeper.SubtractCoins(ctx, from, amount)
ibcPort.SendPacket(ctx, chanid, PacketCoins{from, to, amount})

// in receiving handler
switch packet := packet.(type) {
case PacketCoins:
  bankKeeper.AddCoins(ctx, packet.to, packet.amount)
}

which is nonintuitive. The UX should be refactored.

recvPacket

recvPacket is currently implemented as MsgPacket and ibc.AnteHandler. ibc.AnteHandler iterates over the message types, and if there is any MsgPacket, the antehandler calls channel.Manager.Receive() to verify the proofs and increase the sequence. The Packets are directly routed to the application modules.

While this gives clean interface to the applications, it gives the restriction that the applications cannot return any error. Due to the property of the protocol, the applications should only return the error log with CodeOK, and either push the error receipt packet to the queue or disconnect the connection. The current implementation does not have a fluent approach to this error handling and should be added.

@mossid mossid added the x/ibc label Sep 20, 2019
@alexanderbez alexanderbez added this to the IBC Implementation & Integration milestone Oct 1, 2019
@fedekunze
Copy link
Collaborator

@mossid can you update this issue description with the new implementation? Also, therecvPacket is related to #5230

@fedekunze fedekunze removed this from the IBC Implementation & Integration milestone Dec 10, 2019
@fedekunze
Copy link
Collaborator

Replaced by #5230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants