Update Middleware with Best Practices #903
Conversation
AdityaSripal
requested review from
mpoke,
cwgoes,
colin-axner and
angbrav
as code owners
| ## Packet Data Structuring For Middleware | ||
|
|
||
| Packet senders may choose to send input data not just to the base application but also the middlewares in the stack. The packet data should be structured in the same way that the middleware stack is; i.e. nested from the top level middleware to the base application. | ||
|
|
||
| Similar to the version negotiation, the `app_packet_data` may be marshalling packet data for underlying middleware as well. | ||
|
|
||
| ```json | ||
| { | ||
| "<middleware_name>": "<middleware_packet_data>", | ||
| "app_packet_data": "<application_packet_data>", | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The OnRecvPacket callbacks for ibc apps accept the arg channeltypes.Packet. That means we would have to reassign packet.Data bytes to AppPacketData before calling the next handler in the stack.
If we consider the OnRecvPacket callback of ics29 fee middleware and imagine that there is some middleware specific packet data encoded within the packet we would have to do something like:
var packetData types.FeePacketData
if err := cdc.Unmarshal(packet.GetData(), packetData); err != nil {
// handle err or pass through to underlying app
}
// do whatever logic with fee specific packet data
handleFeePacketData(packetData.FeePacketData)
// reassign packet data bytes before passing packet to next handler
packet.Data = packetData.AppPacketData
return im.app.OnRecvPacket(ctx, packet, relayer)Would this be the intended usage here?
There was a problem hiding this comment.
Hmm yes, this is a good point. Correct this will be necessary, but again only for middleware that wish to modify the packet data
There was a problem hiding this comment.
The OnRecvPacket packet interface would need to be updated within the spec if we are going to allow packet data nesting as a best practice?
| @@ -0,0 +1,71 @@ | |||
| # Best Practices When Designing Middleware | |||
There was a problem hiding this comment.
Does the spec repo have any style conventions? We use the google docs style convention on ibc-go which states only the first word in a title is capitalized
| ```json | ||
| { | ||
| "<middleware_name>": "<middleware_version_value>", | ||
| "app_version": "<application_version_value>", | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Is there an issue tracking the work of making the version/packet data/ack a flat map? I think that improvement would be a really nice dev UX improvement
| ## Packet Data Structuring For Middleware | ||
|
|
||
| Packet senders may choose to send input data not just to the base application but also the middlewares in the stack. The packet data should be structured in the same way that the middleware stack is; i.e. nested from the top level middleware to the base application. | ||
|
|
||
| Similar to the version negotiation, the `app_packet_data` may be marshalling packet data for underlying middleware as well. | ||
|
|
||
| ```json | ||
| { | ||
| "<middleware_name>": "<middleware_packet_data>", | ||
| "app_packet_data": "<application_packet_data>", | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The OnRecvPacket packet interface would need to be updated within the spec if we are going to allow packet data nesting as a best practice?
| packetData = unmarshal(packet.data) | ||
|
|
||
| // do whatever logic with middleware packet data | ||
| handleMiddlewareData(packetData.MiddlewareData) | ||
|
|
||
| // reassign packet data bytes before passing packet to next handler | ||
| packet.Data = packetData.AppPacketData | ||
|
|
||
| return im.app.OnRecvPacket(ctx, packet, relayer) |
There was a problem hiding this comment.
Is this intended to handle arbitrary nesting of packet data when provided the full packet?
| interface AppWrapper { | ||
| getAppVersion(portID: Identifier, channelId: Identifier, version: string): (string) | ||
| getAppPacketData(portID: Identifier, channelId: Identifier, packetData: bytes): (bytes) | ||
| getAppAcknowledgement(portID: Identifier, channelId: Identifier, acknowledgement: bytes): (bytes) | ||
| } |
There was a problem hiding this comment.
How do these functions work exactly? any app in the stack would call the app above it?
|
These changes are not relevant anymore, thus closing. We will improve middleware stacks with cosmos/ibc-go#5814 and cosmos/ibc-go#5819. |
The following lays the groundwork for ADR-8, and establishes design patterns for more complex middleware flows