imp: address ADR 8 review suggestions #3319
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| # ADR 008: Callback to IBC Actors | ||
|
|
||
| ## Changelog | ||
|
|
||
| * 2022-08-10: Initial Draft | ||
| * 2023-03-22: Merged | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted, packet callback interface implemented | ||
|
|
||
| ## Context | ||
|
|
||
|
|
@@ -51,40 +53,47 @@ type IBCActor interface { | |
| type PacketActor interface { | ||
| // OnRecvPacket will be called on the IBCActor after the IBC Application | ||
| // handles the RecvPacket callback if the packet has an IBC Actor as a receiver. | ||
| OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) error | ||
|
|
||
| // OnAcknowledgementPacket will be called on the IBC Actor | ||
| // after the IBC Application handles its own OnAcknowledgementPacket callback | ||
| OnAcknowledgmentPacket( | ||
| ctx sdk.Context, | ||
| packet channeltypes.Packet, | ||
| ack exported.Acknowledgement, | ||
| relayer sdk.AccAddress, | ||
| ) error | ||
|
|
||
| // OnTimeoutPacket will be called on the IBC Actor | ||
| // after the IBC Application handles its own OnTimeoutPacket callback | ||
| OnTimeoutPacket( | ||
| ctx sdk.Context, | ||
| packet channeltypes.Packet, | ||
| relayer sdk.AccAddress, | ||
| ) error | ||
| } | ||
| ``` | ||
|
|
||
| The CallbackPacketData interface will get created to add `GetSourceCallbackAddress` and `GetDestCallbackAddress` methods. These may return an address | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| or they may return the empty string. The address may reference an PacketActor or it may be a regular user address. If the address is not a PacketActor, the actor callback must continue processing (no-op). Any IBC application or middleware that uses these methods must handle these cases. In most cases, the `GetSourceCallbackAddress` will be the sender address and the `GetDestCallbackAddress` will be the receiver address. However, these are named generically so that implementors may choose a different contract address for the callback if they choose. | ||
|
There was a problem hiding this comment. renamed Also renamed |
||
|
|
||
| The interface also defines a `UserDefinedGasLimit` method. Any middleware targeting this interface for callback handling should cap the gas that a callback is allowed to take (especially on AcknowledgePacket and TimeoutPacket) so that a custom callback does not prevent the packet lifecycle from completing. However, since this is a global cap it is likely to be very large. Thus, users may specify a smaller limit to cap the amount of fees a relayer must pay in order to complete the packet lifecycle on the user's behalf. | ||
|
There was a problem hiding this comment. targetting -> targeting |
||
|
|
||
| IBC Apps which provide the base packet data type must implement the `CallbackPacketData` interface to allow `PacketActor` callbacks. | ||
|
There was a problem hiding this comment. I wanted to add a minor note on who needs to implement this interface. Currently we don't support wrapping packet data, so a packet can really only have a single packet data structure. I suspect when we fix this, we would actually move to a packet data map. In this situation it is unclear to me if an ADR 8 callback would do multiple callbacks where each callback is focused on a different packet data type? There was a problem hiding this comment. Sounds like it would be possible using the packet data map structure!
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ```go | ||
| // Implemented by any packet data type that wants to support PacketActor callbacks | ||
| // PacketActor's will be unable to act on any packet data type that does not implement | ||
| // this interface. | ||
| type CallbackPacketData interface { | ||
| // GetSourceCallbackAddress should return the callback address of a packet data on the source chain. | ||
| // This may or may not be the sender of the packet. If no source callback address exists for the packet, | ||
| // an empty string may be returned. | ||
| GetSourceCallbackAddress() string | ||
|
|
||
| // GetDestCallbackAddress should return the callback address of a packet data on the destination chain. | ||
| // This may or may not be the receiver of the packet. If no dest callback address exists for the packet, | ||
| // an empty string may be returned. | ||
| GetDestCallbackAddress() string | ||
|
|
||
| // UserDefinedGasLimit allows the sender of the packet to define inside the packet data | ||
|
|
@@ -103,7 +112,8 @@ IBC Apps or middleware can then call the IBCActor callbacks like so in their own | |
|
|
||
| ### Handshake Callbacks | ||
|
|
||
| The `OnChanOpenInit` handshake callback will need to include an additional field so that the initiating actor can be tracked and called upon during handshake completion. | ||
| The actor provided in the `OnChanOpenInit` callback will be the signer of the `MsgChanOpenInit` message. | ||
|
Comment on lines
+115
to
+116
There was a problem hiding this comment. I thought about leaving it where a new actor can be specified on the There was a problem hiding this comment. |
||
|
|
||
| ```go | ||
| func OnChanOpenInit( | ||
|
|
@@ -141,42 +151,26 @@ func OnChanOpenAck( | |
| ibcActor, _ := acc.(IBCActor) | ||
| ibcActor.OnChanOpen(ctx, portID, channelID, version) | ||
| } | ||
|
|
||
| // the same actor will be used for channel closure | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. removed since we don't support crossing hello's. |
||
| func OnChanCloseInit( | ||
| ctx sdk.Context, | ||
| portID, | ||
| channelID, | ||
| ) error { | ||
| // run any necesssary logic first | ||
|
|
||
| actor := k.getActor(ctx, portID, channelID) | ||
| if actor != "" { | ||
| ibcActor, _ := acc.(IBCActor) | ||
| ibcActor.OnChanClose(ctx, portID, channelID) | ||
| } | ||
| // cleanup state | ||
| k.deleteActor(ctx, portID, channelID) | ||
| } | ||
|
|
||
| func OnChanCloseConfirm( | ||
| ctx sdk.Context, | ||
| portID, | ||
|
|
@@ -194,14 +188,16 @@ func OnChanCloseConfirm( | |
| } | ||
| ``` | ||
|
|
||
| NOTE: The handshake calls `OnChanOpenTry` and `OnChanOpenConfirm` are explicitly left out as it is still to be determined how the actor of the `OnChanOpenTry` step should be provided. Initially only the initiating side of the channel handshake may support setting a channel actor, future improvements should allow both sides of the channel handshake to set channel actors. | ||
|
There was a problem hiding this comment. We can open an issue for this. I think it makes sense to just ignore for now until there's a reason to fix it |
||
|
|
||
| ### PacketCallbacks | ||
|
|
||
| No packet callback API will need to change. | ||
|
|
||
| ```go | ||
| // Call the IBCActor recvPacket callback after processing the packet | ||
| // if the recvPacket callback exists. If the callback returns an error | ||
| // then return an error ack to revert all packet data processing. | ||
| func OnRecvPacket( | ||
| ctx sdk.Context, | ||
| packet channeltypes.Packet, | ||
|
|
@@ -210,10 +206,16 @@ func OnRecvPacket( | |
| // run any necesssary logic first | ||
| // IBCActor logic will postprocess | ||
|
|
||
| // postprocessing should only if the underlying application | ||
| // returns a successful ack | ||
|
|
||
| // unmarshal packet data into expected interface | ||
| var cbPacketData callbackPacketData | ||
| unmarshalInterface(packet.GetData(), cbPacketData) | ||
|
|
||
| if cbPacketData == nil { | ||
| // the packet data does not implement the CallbackPacketData interface | ||
| // continue processing (no-op) | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -229,6 +231,21 @@ func OnRecvPacket( | |
| // deduct consumed gas from original context | ||
| ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed()) | ||
| if err != nil { | ||
| // NOTE: by returning an error acknowledgement, it is assumed that the | ||
| // base IBC application on the counterparty callback stack will be able | ||
| // to properly unmarshal the error acknowledgement. It should not expect | ||
| // some custom error acknowledgement. If it does, failed acknowledgements | ||
| // will be unsuccessfully processed which can be catastrophic in processing | ||
| // refund logic. | ||
| // | ||
| // If this issue is a serious concern, an ADR 8 implementation can construct its own | ||
| // acknowledgement type which wraps the underlying application acknowledgement. This | ||
| // would require deployment on both sides of the packet flow, in addition to version | ||
| // negotiation to enable the custom acknowledgement type usage. | ||
| // | ||
| // Future improvmenets should allow for each IBC application in a stack of | ||
| // callbacks to provide their own acknowledgement without disrupting the unmarshaling | ||
| // of an application above or below it in the stack. | ||
|
Comment on lines
+234
to
+248
There was a problem hiding this comment. I think this is a serious concern, but I think for practical purposes it's okay to move forward with some assumption which is partially unsafe. Improvements to the middleware design should fix this issue. I thought it best to leave a lot of documentation here. It might be nice to double check no base applications are using custom failed acks |
||
| return AcknowledgementError(err) | ||
| } | ||
| } | ||
|
|
@@ -245,14 +262,17 @@ func (im IBCModule) OnAcknowledgementPacket( | |
| ctx sdk.Context, | ||
| packet channeltypes.Packet, | ||
| acknowledgement []byte, | ||
| relayer sdk.AccAddress, | ||
| ) error { | ||
| // application-specific onAcknowledgmentPacket logic | ||
|
|
||
| // unmarshal packet data into expected interface | ||
| var cbPacketData callbackPacketData | ||
| unmarshalInterface(packet.GetData(), cbPacketData) | ||
|
|
||
| if cbPacketData == nil { | ||
| // the packet data does not implement the CallbackPacketData interface | ||
| // continue processing (no-op) | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -261,32 +281,42 @@ func (im IBCModule) OnAcknowledgementPacket( | |
| unmarshal(acknowledgement, ack) | ||
|
|
||
| // send acknowledgement to original actor | ||
| acc := k.getAccount(ctx, cbPacketData.GetSourceCallbackAddress()) | ||
| ibcActor, ok := acc.(IBCActor) | ||
| if ok { | ||
| gasLimit := getGasLimit(ctx, cbPacketData) | ||
|
|
||
|
|
||
| handleCallback := func() error { | ||
| // create cached context with gas limit | ||
| cacheCtx, writeFn := ctx.CacheContext() | ||
| cacheCtx = cacheCtx.WithGasLimit(gasLimit) | ||
|
|
||
| defer func() { | ||
| if e := recover(); e != nil { | ||
| log("ran out of gas in callback. reverting callback state") | ||
| } else if err == nil { | ||
| // only write callback state if we did not panic during execution | ||
| // and the error returned is nil | ||
| writeFn() | ||
| } | ||
|
Comment on lines
+290
to
+302
There was a problem hiding this comment. The previous pseudo code was incorrect as it didn't handle where the error returned was nil or not. There's also a more subtle issue that you only want to catch the panic of the function call using the |
||
| } | ||
|
|
||
| err := ibcActor.OnAcknowledgementPacket(cacheCtx, packet, ack, relayer) | ||
|
|
||
| // deduct consumed gas from original context | ||
| ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed()) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| if err := handleCallback(); err != nil { | ||
| setAckCallbackError(ctx, packet, err) // optional | ||
| emitAckCallbackErrorEvents(err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Call the IBCActor timeoutPacket callback after processing the packet | ||
|
|
@@ -298,44 +328,56 @@ func (im IBCModule) OnAcknowledgementPacket( | |
| func (im IBCModule) OnTimeoutPacket( | ||
| ctx sdk.Context, | ||
| packet channeltypes.Packet, | ||
| relayer sdk.AccAddress, | ||
| ) error { | ||
| // application-specific onTimeoutPacket logic | ||
|
|
||
| // unmarshal packet data into expected interface | ||
| var cbPacketData callbackPacketData | ||
| unmarshalInterface(packet.GetData(), cbPacketData) | ||
|
|
||
| if cbPacketData == nil { | ||
| // the packet data does not implement the CallbackPacketData interface | ||
| // continue processing (no-op) | ||
| return | ||
| } | ||
|
|
||
| // call timeout callback on original actor | ||
| acc := k.getAccount(ctx, cbPacketData.GetSourceCallbackAddress()) | ||
| ibcActor, ok := acc.(IBCActor) | ||
| if ok { | ||
| gasLimit := getGasLimit(ctx, cbPacketData) | ||
|
|
||
| handleCallback := func() error { | ||
| // create cached context with gas limit | ||
| cacheCtx, writeFn := ctx.CacheContext() | ||
| cacheCtx = cacheCtx.WithGasLimit(gasLimit) | ||
|
|
||
| defer func() { | ||
| if e := recover(); e != nil { | ||
| log("ran out of gas in callback. reverting callback state") | ||
| } else if err == nil { | ||
| // only write callback state if we did not panic during execution | ||
| // and the error returned is nil | ||
| writeFn() | ||
| } | ||
| } | ||
|
|
||
| err := ibcActor.OnTimeoutPacket(ctx, packet, relayer) | ||
|
|
||
| // deduct consumed gas from original context | ||
| ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed()) | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| if err := handleCallback(); err != nil { | ||
| setTimeoutCallbackError(ctx, packet, err) // optional | ||
| emitTimeoutCallbackErrorEvents(err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func getGasLimit(ctx sdk.Context, cbPacketData CallbackPacketData) uint64 { | ||
|
|
@@ -367,10 +409,13 @@ Chains are expected to specify a `chainDefinedActorCallbackLimit` to ensure that | |
| ### Negative | ||
|
|
||
| - Callbacks may now have unbounded gas consumption since the actor may execute arbitrary logic. Chains implementing this feature should take care to place limitations on how much gas an actor callback can consume. | ||
|
|
||
| ### Neutral | ||
|
|
||
| - Application packets that want to support ADR-8 must additionally have their packet data implement the `CallbackPacketData` interface and register their implementation on the chain codec | ||
|
|
||
| ## References | ||
|
|
||
| - [Original issue](https://github.com/cosmos/ibc-go/issues/1660) | ||
| - [CallbackPacketData interface implementation](https://github.com/cosmos/ibc-go/pull/3287) | ||
| - [ICS 20, ICS 27 implementations of the CallbackPacketData interface](https://github.com/cosmos/ibc-go/pull/3287) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
fixes: #1976 (comment)