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

ADR 008: IBC Actor Callbacks #1976

Merged
merged 15 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
374 changes: 374 additions & 0 deletions docs/architecture/adr-008-app-caller-cbs/adr-008-app-caller-cbs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,374 @@
# ADR 008: Callback to IBC Actors

## Changelog
* 2022-08-10: Initial Draft

## Status

Proposed

## Context

IBC was designed with callbacks between core IBC and IBC applications. IBC apps would send a packet to core IBC. When the result of the packet lifecycle eventually resolved into either an acknowledgement or a timeout, core IBC called a callback on the IBC application so that the IBC application could take action on the basis of the result (e.g. unescrow tokens for ICS-20).

This setup worked well for off-chain users interacting with IBC applications.

We are now seeing the desire for secondary applications (e.g. smart contracts, modules) to call into IBC apps as part of their state machine logic and then do some actions on the basis of the packet result. Or to receive a packet from IBC and do some logic upon receipt.

Example Usecases:
- Send an ICS-20 packet, and if it is successful, then send an ICA-packet to swap tokens on LP and return funds to sender
- Execute some logic upon receipt of token transfer to a smart contract address

This requires a second layer of callbacks. The IBC application already gets the result of the packet from core IBC, but currently there is no standardized way to pass this information on to an actor module/smart contract.

## Definitions

- Actor: an actor is an on-chain module (this may be a hardcoded module in the chain binary or a smart contract) that wishes to execute custom logic whenever IBC receives a packet flow that it has either sent or received. It **must** be addressable by a string value.

## Decision

Create a standardized callback interface that actors can implement. IBC applications (or middleware that wraps IBC applications) can now call this callback to route the result of the packet/channel handshake from core IBC to the IBC application to the original actor on the sending chain. IBC applications can route the packet receipt to the destination actor on the receiving chain.

IBC actors may implement the following interface:

```go
type IBCActor interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need OnChannelUpgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes eventually. The channel handshake callbacks are not going to be implemented in the short term

Copy link
Contributor

Choose a reason for hiding this comment

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

// OnChannelOpen will be called on the IBCActor when the channel opens
// this will happen either on ChanOpenAck or ChanOpenConfirm
OnChannelOpen(ctx sdk.Context, portID, channelID, version string)

// OnChannelClose will be called on the IBCActor if the channel closes
// this will be called on either ChanCloseInit or ChanCloseConfirm and if the channel handshake fails on our end
// NOTE: currently the channel does not automatically close if the counterparty fails the handhshake so actors must be prepared for an OpenInit to never return a callback for the time being
OnChannelClose(ctx sdk.Context, portID, channelID string)

// IBCActor must also implement PacketActor interface
PacketActor
}

// PacketActor is split out into its own separate interface since implementors may choose
// to only support callbacks for packet methods rather than supporting the full 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
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be relayer sdk.AccAddress to align with the IBCModule interface right?


// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

same

) 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
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

) error
}
Comment on lines +52 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are identical to the existing callbacks, wouldn't it make sense to split those out rather than duplicating them? So maybe:

type PacketCallbacks interface {
	// OnRecvPacket must return an acknowledgement that implements the Acknowledgement interface.
	// In the case of an asynchronous acknowledgement, nil should be returned.
	// If the acknowledgement returned is successful, the state changes on callback are written,
	// otherwise the application state changes are discarded. In either case the packet is received
	// and the acknowledgement is written (in synchronous cases).
	OnRecvPacket(
		ctx sdk.Context,
		packet channeltypes.Packet,
		relayer sdk.AccAddress,
	) exported.Acknowledgement

	OnAcknowledgementPacket(
		ctx sdk.Context,
		packet channeltypes.Packet,
		acknowledgement []byte,
		relayer sdk.AccAddress,
	) error

	OnTimeoutPacket(
		ctx sdk.Context,
		packet channeltypes.Packet,
		relayer sdk.AccAddress,
	) error
}

type PacketActor interface {
    PacketCallbacks
}

Just thinking in the future if these interfaces change, it'd be nice to ensure compile wise that they don't go out of sync

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, these are written down so that implementors understand what it is they're doing. But how exactly it gets implemented will depend on the execution environment in question.

For example with wasm, all these callbacks will be reduced to the single interface by which you call into a smart contract

Copy link
Member Author

Choose a reason for hiding this comment

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

These interfaces don't actually exist in #3287

Copy link
Member

Choose a reason for hiding this comment

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

I am all for reducing the IBCModule interface down into smaller composable interfaces! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Big fan of splitting up larger interfaces into smaller ones and accepting narrower scoped interfaces in method signatures where possible!

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #3317 to track this suggestion

```

The CallbackPacketData interface will get extended 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.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

```go
// Implemented by any packet data type that wants to support
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// PacketActor callbacks
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
type CallbackPacketData interface {
// may return the empty string
GetSrcCallbackAddress() string

// may return the empty string
GetDestCallbackAddress() string
Comment on lines +84 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these might need more explanation? It seems like it might be valuable for an IBC application to allow a receiver address of the callback to be different than the receiver address for the packet data action

Comment on lines +85 to +88
Copy link
Member

Choose a reason for hiding this comment

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

mega nit on naming consistency here - the packet interface uses naming of GetSourcePort and GetDestPort rather than "src" abbreviation.

should we keep them consistent?

I don't feel particularly strongly tbh, just something I noticed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is worth doing


// UserDefinedGasLimit allows the sender of the packet to define inside the packet data
// a gas limit for how much the ADR-8 callbacks can consume. If defined, this will be passed
// in as the gas limit so that the callback is guaranteed to complete within a specific limit.
// On recvPacket, a gas-overflow will just fail the transaction allowing it to timeout on the sender side.
// On ackPacket and timeoutPacket, a gas-overflow will reject state changes made during callback but still
// commit the transaction. This ensures the packet lifecycle can always complete.
// If the packet data returns 0, the remaining gas limit will be passed in (modulo any chain-defined limit)
// Otherwise, we will set the gas limit passed into the callback to the `min(ctx.GasLimit, UserDefinedGasLimit())`
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
UserDefinedGasLimit() uint64
}
```

IBC Apps or middleware can then call the IBCActor callbacks like so in their own callbacks:

### 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.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

```go
func OnChanOpenInit(
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
portID string,
channelID string,
channelCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty,
version string,
actor string,
) (string, error) {
acc := k.getAccount(ctx, actor)
ibcActor, ok := acc.(IBCActor)
if ok {
k.setActor(ctx, portID, channelID, actor)
}

// continued logic
}

func OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
counterpartyChannelID string,
counterpartyVersion string,
) error {
// run any necessary logic first
// negotiate final version

actor := k.getActor(ctx, portID, channelID)
if actor != "" {
ibcActor, _ := acc.(IBCActor)
ibcActor.OnChanOpen(ctx, portID, channelID, version)
}
// cleanup state
k.deleteActor(ctx, portID, channelID)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

func OnChanOpenConfirm(
ctx sdk.Context,
portID,
channelID string,
) 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)
}
// 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)
}
Comment on lines +171 to +175
Copy link
Member

Choose a reason for hiding this comment

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

seems odd to set a different actor for the closing handshake steps rather than just reusing what's already there. i.e. not deleting the state entry in ACK/CONFIRM.


// continued logic
}

func OnChanCloseConfirm(
ctx sdk.Context,
portID,
channelID string,
) 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)
}
```

### 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
func OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
relayer sdk.AccAddress,
) (ack exported.Acknowledgement) {
// run any necesssary logic first
// IBCActor logic will postprocess
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// unmarshal packet data into expected interface
var cbPacketData callbackPacketData
unmarshalInterface(packet.GetData(), cbPacketData)
if cbPacketData == nil {
return
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +214 to +218
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this return rather be return app.OnRecvPacket(...)? maybe it's supposed to be implicit?


acc := k.getAccount(ctx, cbPacketData.GetDstCallbackAddress())
ibcActor, ok := acc.(IBCActor)
if ok {
// set gas limit for callback
gasLimit := getGasLimit(ctx, cbPacketData)
cbCtx = ctx.WithGasLimit(gasLimit)

err := ibcActor.OnRecvPacket(cbCtx, packet, relayer)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// deduct consumed gas from original context
ctx = ctx.WithGasLimit(ctx.GasMeter().RemainingGas() - cbCtx.GasMeter().GasConsumed())
if err != nil {
return AcknowledgementError(err)
}
}
return
}

// Call the IBCActor acknowledgementPacket callback after processing the packet
// if the ackPacket callback exists and returns an error
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// DO NOT return the error upstream. The acknowledgement must complete for the packet
// lifecycle to end, so the custom callback cannot block completion.
// Instead we emit error events and set the error in state
// so that users and on-chain logic can handle this appropriately
func (im IBCModule) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
relayer string,
) error {
// application-specific onAcknowledgmentPacket logic

// unmarshal packet data into expected interface
var cbPacketData callbackPacketData
unmarshalInterface(packet.GetData(), cbPacketData)
if cbPacketData == nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

ditto on implicit/explicit about passing through to app rather than silently returning

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that is what:
// application-specific onAcknowledgmentPacket logic
is

}

// unmarshal ack bytes into the acknowledgment interface
var ack exported.Acknowledgement
unmarshal(acknowledgement, ack)

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

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

defer func() {
if e := recover(); e != nil {
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be a recover from panic? is it not possible to just do error checking?

Copy link
Contributor

Choose a reason for hiding this comment

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

will panicking revert state in a way that returning an error won't?

Copy link
Contributor

@colin-axner colin-axner Mar 21, 2023

Choose a reason for hiding this comment

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

I guess the idea is that there is a possibility the cacheCtx runs out of gas, but the regular context still has some execution left. I think we need a bit more logic to handle this scenario as you need to propogate the panic if the regular context is also out of gas. State should only be written if err == nil && cacheCtx does not panic. If it panics, it should only continue to panic if the regular context is also out of gas

log("ran out of gas in callback. reverting callback state")
} else {
// only write callback state if we did not panic during execution
writeFn()
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
}

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

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

setAckCallbackError(ctx, packet, err)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
emitAckCallbackErrorEvents(err)
}
}

// Call the IBCActor timeoutPacket callback after processing the packet
// if the timeoutPacket callback exists and returns an error
// DO NOT return the error upstream. The timeout must complete for the packet
// lifecycle to end, so the custom callback cannot block completion.
// Instead we emit error events and set the error in state
// so that users and on-chain logic can handle this appropriately
func (im IBCModule) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
relayer string,
) error {
// application-specific onTimeoutPacket logic

// unmarshal packet data into expected interface
var cbPacketData callbackPacketData
unmarshalInterface(packet.GetData(), cbPacketData)
if cbPacketData == nil {
return
}

// call timeout callback on original actor
acc := k.getAccount(ctx, cbPacketData.GetSrcCallbackAddress())
ibcActor, ok := acc.(IBCActor)
if ok {
gasLimit := getGasLimit(ctx, cbPacketData)

// 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()
}
}

err := ibcActor.OnTimeoutPacket(ctx, packet, relayer)

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

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

func getGasLimit(ctx sdk.Context, cbPacketData CallbackPacketData) uint64 {
// getGasLimit returns the gas limit to pass into the actor callback
// this will be the minimum of the remaining gas limit in the tx
// and the config defined gas limit. The config limit is itself
// the minimum of a user defined gas limit and the chain-defined gas limit
// for actor callbacks
var configLimit uint64
if cbPacketData == 0 {
configLimit = chainDefinedActorCallbackLimit
} else {
configLimit = min(chainDefinedActorCallbackLimit, cbPacketData.UserDefinedGasLimit())
}
return min(ctx.GasMeter().GasRemaining(), configLimit)
}
```

Chains are expected to specify a `chainDefinedActorCallbackLimit` to ensure that callbacks do not consume an arbitrary amount of gas. Thus, it should always be possible for a relayer to complete the packet lifecycle even if the actor callbacks cannot run successfully.

## Consequences

### Positive

- IBC Actors can now programatically execute logic that involves sending a packet and then performing some additional logic once the packet lifecycle is complete
- Middleware implementing ADR-8 can be generally used for any application
- Leverages the same callback architecture used between core IBC and IBC applications

### 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
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd move this to the Neutral section 🤷


### Neutral

## References

- https://github.com/cosmos/ibc-go/issues/1660
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Loading