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.

Sign up for GitHub

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

Jump to bottom

imp: address ADR 8 review suggestions #3319

Merged
merged 8 commits into from
Mar 22, 2023
Merged

imp: address ADR 8 review suggestions #3319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7f39d75
chore: add adr 8 to architecture readme
colin-axner Mar 22, 2023
f03761b
chore: apply review suggestion https://github.com/cosmos/ibc-go/pull/…
colin-axner Mar 22, 2023
9c0624f
chore: add reference and move requirement of implmenting callback int…
colin-axner Mar 22, 2023
8a690c9
imp: update ADR 8 documentation and code handling
colin-axner Mar 22, 2023
3e93505
fix: update pseudo code for OnAck and OnTimeout handling
colin-axner Mar 22, 2023
3176855
chore: update godoc
colin-axner Mar 22, 2023
9b26da8
Update docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cb…
colin-axner Mar 22, 2023
ace9da9
Update docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cb…
colin-axner Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/architecture/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ To suggest an ADR, please make use of the [ADR template](./adr-template.md) prov
| [002](./adr-002-go-module-versioning.md) | Go module versioning | Accepted |
| [003](./adr-003-ics27-acknowledgement.md) | ICS27 acknowledgement format | Accepted |
| [004](./adr-004-ics29-lock-fee-module.md) | ICS29 module locking upon escrow out of balance | Accepted |
| [008](./adr-008-app-caller-cbs/adr-008-app-caller-cbs.md) | Callback to IBC ACtors | Accepted |
| [015](./adr-015-ibc-packet-receiver.md) | IBC Packet Routing | Accepted |
| [025](./adr-025-ibc-passive-channels.md) | IBC passive channels | Deprecated |
| [026](./adr-026-ibc-client-recovery-mechanisms.md) | IBC client recovery mechansisms | Accepted |
Expand Down
191 changes: 118 additions & 73 deletions docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cbs.md
View file
Open in desktop
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

Proposed
Accepted, packet callback interface implemented

## Context

Expand Down Expand Up @@ -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 string) error
OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes: #1976 (comment)


// 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 string
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 string
relayer sdk.AccAddress,
) error
}
```

The CallbackPacketData interface will get created to add `GetSrcCallbackAddress` and `GetDestCallbackAddress` methods. These may return an address
or they may return the empty string. The address may reference an IBCActor or it may be a regular user address. If the address is not an IBCActor, 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 `GetSrcCallbackAddress` 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.
The `CallbackPacketData` interface will get created to add `GetSourceCallbackAddress` and `GetDestCallbackAddress` methods. These may return an address
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.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

targetting -> targeting


The interface also defines a `UserDefinedGasLimit` method. Any middleware targetting 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.
IBC applications which provide the base packet data type must implement the `CallbackPacketData` interface to allow `PacketActor` callbacks.

```go
// Implemented by any packet data type that wants to support
// PacketActor callbacks
// 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 {
// may return the empty string
GetSrcCallbackAddress() string

// may return the empty string
// 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
Expand All @@ -103,7 +112,8 @@ IBC Apps or middleware can then call the IBCActor callbacks like so in their own

### Handshake Callbacks

The handshake init callbacks (`OnChanOpenInit` and `OnChanCloseInit`) will need to include an additional field so that the initiating actor can be tracked and called upon during handshake completion.
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about leaving it where a new actor can be specified on the OnChanCloseInit, but this flow is very underspecified (where does the actor get provided? The base application is unlikely to know of what to provide). Since this seems like a feature/dev UX improvement, I think it makes sense to start with more basic functionality of requiring the channel handshake initiator to also provide the closing channel handshake logic. If it proves useful to separate the two concerns, changes can be made at a later date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1976 (comment)


```go
func OnChanOpenInit(
Expand Down Expand Up @@ -141,42 +151,26 @@ func OnChanOpenAck(
ibcActor, _ := acc.(IBCActor)
ibcActor.OnChanOpen(ctx, portID, channelID, version)
}
// cleanup state
k.deleteActor(ctx, portID, channelID)

// the same actor will be used for channel closure
}

func OnChanOpenConfirm(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed since we don't support crossing hello's. OnChanOpenConfirm would never have a preset actor if OnChanOpenTry doesn't support that functionality

func OnChanCloseInit(
ctx sdk.Context,
portID,
channelID string,
channelID,
) error {
// run any necesssary logic first
// retrieve final version

actor := k.getActor(ctx, portID, channelID)
if actor != "" {
ibcActor, _ := acc.(IBCActor)
ibcActor.OnChanOpen(ctx, portID, channelID, version)
ibcActor.OnChanClose(ctx, portID, channelID)
}
// cleanup state
k.deleteActor(ctx, portID, channelID)
}

func OnChanCloseInit(
ctx sdk.Context,
portID,
channelID,
actor string,
) error {
acc := k.getAccount(ctx, actor)
ibcActor, ok := acc.(IBCActor)
if ok {
k.setActor(ctx, portID, channelID, actor)
}

// continued logic
}

func OnChanCloseConfirm(
ctx sdk.Context,
portID,
Expand All @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 and returns an error
// then return an error ack to revert all packet data processing
// 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,
Expand All @@ -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
}

Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
}
}
Expand All @@ -245,14 +262,17 @@ func (im IBCModule) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
relayer string,
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
}

Expand All @@ -261,32 +281,42 @@ func (im IBCModule) OnAcknowledgementPacket(
unmarshal(acknowledgement, ack)

// send acknowledgement to original actor
acc := k.getAccount(ctx, cbPacketData.GetSrcCallbackAddress())
acc := k.getAccount(ctx, cbPacketData.GetSourceCallbackAddress())
ibcActor, ok := acc.(IBCActor)
if ok {
gasLimit := getGasLimit(ctx, cbPacketData)

// create cached context with gas limit
cacheCtx, writeFn := ctx.CacheContext()
cacheCtx = cacheCtx.WithGasLimit(gasLimit)

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 {
// only write callback state if we did not panic during execution
writeFn()
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 cacheCtx, a panic performed using the regular context should not be caught as we need to panic to inform baseapp that the transaction execution ran out of gas. For this reason I generate an anonymous function to perform the callback using the cacheCtx

}
}

err := ibcActor.OnAcknowledgementPacket(cacheCtx, packet, ack, relayer)
err := ibcActor.OnAcknowledgementPacket(cacheCtx, packet, ack, relayer)

// deduct consumed gas from original context
ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed())
// deduct consumed gas from original context
ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed())

return err
}

setAckCallbackError(ctx, packet, err)
emitAckCallbackErrorEvents(err)
if err := handleCallback(); err != nil {
setAckCallbackError(ctx, packet, err) // optional
emitAckCallbackErrorEvents(err)
}
}

return nil
}

// Call the IBCActor timeoutPacket callback after processing the packet
Expand All @@ -298,44 +328,56 @@ func (im IBCModule) OnAcknowledgementPacket(
func (im IBCModule) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer string,
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.GetSrcCallbackAddress())
acc := k.getAccount(ctx, cbPacketData.GetSourceCallbackAddress())
ibcActor, ok := acc.(IBCActor)
if ok {
gasLimit := getGasLimit(ctx, cbPacketData)

// create cached context with gas limit
cacheCtx, writeFn := ctx.CacheContext()
cacheCtx = cacheCtx.WithGasLimit(gasLimit)
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 {
// only write callback state if we did not panic during execution
writeFn()
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)
err := ibcActor.OnTimeoutPacket(ctx, packet, relayer)

// deduct consumed gas from original context
ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed())
// deduct consumed gas from original context
ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed())

setTimeoutCallbackError(ctx, packet, err)
emitTimeoutCallbackErrorEvents(err)
return err
}

if err := handleCallback(); err != nil {
setTimeoutCallbackError(ctx, packet, err) // optional
emitTimeoutCallbackErrorEvents(err)
}
}

return nil
}

func getGasLimit(ctx sdk.Context, cbPacketData CallbackPacketData) uint64 {
Expand Down Expand Up @@ -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.
- 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

### 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

- https://github.com/cosmos/ibc-go/issues/1660
- [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)