Support hooks in Transfer app

[thomas-nguy]

Summary

We would like to have the ability to define post-processing logic after an IBC transfer.

Problem Definition

Supporting hook would be quite beneficial for app developper that wants to include post-processing logic after an IBC transfer. For example,

• Transform the voucher denomination to a more friendly coin denomination
• Wrap the voucher to a non ICS20 standard token (ex ERC20 for Ethermint)
• Consume the voucher & performs additional operation
• Add more tracking and logs
• Etc..

The design should be flexible and should not impact the current implementation logic.

Proposal

Based on the hook implementation in GravityBridge, we propose a hook interface supporting those following methods

type TransferHooks interface {
	AfterSendTransfer(
		ctx sdk.Context,
		sourcePort, sourceChannel string,
		token sdk.Coin,
		sender sdk.AccAddress,
		receiver string,
		isSource bool)
	AfterRecvTransfer(
		ctx sdk.Context,
		destPort, destChannel string,
		token sdk.Coin,
		receiver string,
		isSource bool)
	AfterRefundTransfer(
		ctx sdk.Context,
		sourcePort, sourceChannel string,
		token sdk.Coin,
		sender string,
		isSource bool)
}

Those hooks will be triggered after a Send, Receive and Refund events respectively.

Submitting a draft in a moment, It is open to discussion

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

I think this would be a great improvement to the ICS 20 module! Post processing logic seems to be desirable for most standardized IBC applications (Interchain Accounts seems to be another good example)

I think it'd be great if we came upon a generalized solution which could be reused for any IBC application. The ICS 30 middleware does attempt to fulfill this role with one key distinction. The hooks attached via the middleware are capable of making the acknowledgement or timeout processing of a packet fail (or at least within the ibc-go implementation)

The proposed hooks above seem to only allow post processing logic which cannot fail (they would need to panic if they do). I could see good arguments for not allowing post processing logic to affect the behaviour of a packet, but maybe this is something we could parametrize in ICS30? The hooks could always panic to prevent a packet from being received, acknowledged or timed out regardless of this parameter.

One use case which comes to mind for ICS 20 which would benefit from the middleware design is the usage of an allow/deny list of tokens which may be received over ICS 20. It has been a common request from chains to be able to control exactly which channel id's may be used for ICS 20. In an ICS 30 middleware design, this could be a very simple module addition which uses governance parameters to allow/reject a transfer based on the channel id

Thoughts? Is there a good reason not to use ICS 30?

[thomas-nguy]

Thanks for your feedback @colin-axner

ICS 30 is a more generic approach which is not necessarily redundant with the hooks approach for ICS 20 module.

The way I see it is that hooks will be used to perform post-processing logic ONLY on ICS 20 tokens such as for wrapping, internal conversions, burning & minting etc...I think having a dedicated hook interface format would make it easier for developers to implement their custom logics. The result is also directly ties with the result of the ICS20 packet itself.

The middleware, in the other hand, would be use to perform pre/post processing logic related to the IBC packets in general such as applying a fee mechanism, swapping, piping to other apps etc...

The implementation is also harmless and quite simple, it might be a good starting point before ICS 30 become available.

[fedekunze]

I think the ICS30 middleware solves the use case of hooks for ICS20 transfers, although not sure what's the timeline for that

[colin-axner]

Thanks for your response @thomas-nguy. I see your point and I think examining the differences closer might help us come to consensus

My initial understanding tells me that the proposed hooks are a subset of the functionality enabled by ICS 30, but perhaps the important thing to note is that the proposed hooks do not change existing functionality and thus are safer changes to introduce into the codebase

Lets look at the interface for the hooks compared to the ICS 30 interfaces:

AfterSendTransfer(
		ctx sdk.Context,
		sourcePort, sourceChannel string,
		token sdk.Coin,
		sender sdk.AccAddress,
		receiver string,
		isSource bool,
)
SendPacket(
	ctx sdk.Context,
	channelCap *capabilitytypes.Capability,
	packet exported.PacketI,
) error

The primary difference between the two (besides function naming and the exact arguments provided) is that these hooks proposed are specific to ICS 20 (which is intentional)

AfterRecvTransfer(
		ctx sdk.Context,
		destPort, destChannel string,
		token sdk.Coin,
		receiver string,
		isSource bool,
)

OnRecvPacket(
	ctx sdk.Context,
	packet channeltypes.Packet,
	relayer sdk.AccAddress,
) ibcexported.Acknowledgement 

The same is true for these functions.

AfterRefundTransfer(
		ctx sdk.Context,
		sourcePort, sourceChannel string,
		token sdk.Coin,
		sender string,
		isSource bool,
)

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

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

The same is true except the base application in the middleware stack would need to understand that timeouts indicate a refund and a failed acknowledgement also indicates a refund as well.

---

Since the proposed hooks are taking the ICS 30 callbacks and translating them into ICS 20 language, I don't see how they might provide any additional functionality?

My initial reaction is to propose implementing the proposed hooks via the corresponding ICS 30 callbacks. That is, use the already agreed upon API so if we decided to enable full ICS 30 functionality (beyond just post processing), we could do this without breaking existing API or needing to maintain old interfaces. My primary hesitation with regards to the proposed transfer hooks is coming from the perspective of a maintainer. I want to ensure we aren't adding interfaces which will be deprecated fairly quickly

The only problem with implementing a portion of the ICS 30 interfaces is that our code will want to rely on the entire interface. However, the post processing apps could easily return nil errors for the unused channel handshake callbacks, but at this point I think it'd make sense to deliver full ICS 30 functionality for ICS 20

The actual implementation of ICS 30 for ICS 20 should actually be quite small. It would primarily just be adding the app callbacks into each app module callback like such. Very similar to the amount of changes in your draft pr. We could start this now, no ICS 30 module is needed to implement ICS 30.

I think most folks will be in favor of making ICS 20 extensible. I'm happy to help push this through as well

---

I think I should point out the pros/cons of using ICS 30 over the TransferHooks:

Pros:

• uses established interface
• uses already agreed upon spec
• allows custom logic to wrap an ICS 20 transfer
• allows reusability of post processing logic modules for other similar apps (NFT transfers come to mind)
• easier to maintain generalized interface across all our IBC apps
• developers only need to become familiar with one interface to add their custom logic

Cons:

• Post processing logic requires understanding when refunds occur in ICS 20
• The custom logic may affect packet cycles

One important detail to note is that new ports may need to be binded to in order to support multiple different types of custom logic on the same chain in the same way the incentivized version of transfer will be fee-transfer, but the base post processing logic a chain would like to use could take advantage of the already established channels using the transfer port

What do you think?

[thomas-nguy]

Ok fair enough,

I will open another draft to implement ICS30 support in ICS20, I will probably wait #307 #331 to be finalised to avoid code duplication

[colin-axner]

Closing this issue since I believe this can be supported via middleware post processing. Please let us know if this is not the case