From 45c051de7c816264387603e91b156d451d36a5ec Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 31 Aug 2023 14:13:53 +0200 Subject: [PATCH] docs(callbacks): improved godocs for ContractKeeper state reversion (backport #4516) (#4531) * docs(callbacks): improved godocs for ContractKeeper state reversion (#4516) * docs(callbacks): improved godocs for ContractKeeper state reversion * docs: updated docs/callbacks/interfaces.md (cherry picked from commit 7985bbd2aaa6fdab59a9f1e1577a3443e8b1395d) # Conflicts: # modules/apps/callbacks/types/expected_keepers.go * fix: resolved backport conflicts --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk --- docs/middleware/callbacks/interfaces.md | 121 ++++++++++-------- .../apps/callbacks/types/expected_keepers.go | 37 ++++-- 2 files changed, 98 insertions(+), 60 deletions(-) diff --git a/docs/middleware/callbacks/interfaces.md b/docs/middleware/callbacks/interfaces.md index a9af78ef394..1e5ae06b6af 100644 --- a/docs/middleware/callbacks/interfaces.md +++ b/docs/middleware/callbacks/interfaces.md @@ -68,57 +68,76 @@ The callbacks middleware requires the secondary application to implement the [`C ```go // ContractKeeper defines the entry points exposed to the VM module which invokes a smart contract type ContractKeeper interface { - // IBCSendPacketCallback is called in the source chain when a PacketSend is executed. The - // packetSenderAddress is determined by the underlying module, and may be empty if the sender is - // unknown or undefined. The contract is expected to handle the callback within the user defined - // gas limit, and handle any errors, or panics gracefully. - // If an error is returned, the transaction will be reverted by the callbacks middleware, and the - // packet will not be sent. - IBCSendPacketCallback( - ctx sdk.Context, - sourcePort string, - sourceChannel string, - timeoutHeight clienttypes.Height, - timeoutTimestamp uint64, - packetData []byte, - contractAddress, - packetSenderAddress string, - ) error - // IBCOnAcknowledgementPacketCallback is called in the source chain when a packet acknowledgement - // is received. The packetSenderAddress is determined by the underlying module, and may be empty if - // the sender is unknown or undefined. The contract is expected to handle the callback within the - // user defined gas limit, and handle any errors, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. - IBCOnAcknowledgementPacketCallback( - ctx sdk.Context, - packet channeltypes.Packet, - acknowledgement []byte, - relayer sdk.AccAddress, - contractAddress, - packetSenderAddress string, - ) error - // IBCOnTimeoutPacketCallback is called in the source chain when a packet is not received before - // the timeout height. The packetSenderAddress is determined by the underlying module, and may be - // empty if the sender is unknown or undefined. The contract is expected to handle the callback - // within the user defined gas limit, and handle any error, out of gas, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. - IBCOnTimeoutPacketCallback( - ctx sdk.Context, - packet channeltypes.Packet, - relayer sdk.AccAddress, - contractAddress, - packetSenderAddress string, - ) error - // IBCReceivePacketCallback is called in the destination chain when a packet acknowledgement is written. - // The contract is expected to handle the callback within the user defined gas limit, and handle any errors, - // out of gas, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. - IBCReceivePacketCallback( - ctx sdk.Context, - packet ibcexported.PacketI, - ack ibcexported.Acknowledgement, - contractAddress string, - ) error + // IBCSendPacketCallback is called in the source chain when a PacketSend is executed. The + // packetSenderAddress is determined by the underlying module, and may be empty if the sender is + // unknown or undefined. The contract is expected to handle the callback within the user defined + // gas limit, and handle any errors, or panics gracefully. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, and the error will be propagated to the underlying IBC + // application, resulting in a packet send failure. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. + IBCSendPacketCallback( + cachedCtx sdk.Context, + sourcePort string, + sourceChannel string, + timeoutHeight clienttypes.Height, + timeoutTimestamp uint64, + packetData []byte, + contractAddress, + packetSenderAddress string, + ) error + // IBCOnAcknowledgementPacketCallback is called in the source chain when a packet acknowledgement + // is received. The packetSenderAddress is determined by the underlying module, and may be empty if + // the sender is unknown or undefined. The contract is expected to handle the callback within the + // user defined gas limit, and handle any errors, or panics gracefully. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. + IBCOnAcknowledgementPacketCallback( + cachedCtx sdk.Context, + packet channeltypes.Packet, + acknowledgement []byte, + relayer sdk.AccAddress, + contractAddress, + packetSenderAddress string, + ) error + // IBCOnTimeoutPacketCallback is called in the source chain when a packet is not received before + // the timeout height. The packetSenderAddress is determined by the underlying module, and may be + // empty if the sender is unknown or undefined. The contract is expected to handle the callback + // within the user defined gas limit, and handle any error, out of gas, or panics gracefully. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. + IBCOnTimeoutPacketCallback( + cachedCtx sdk.Context, + packet channeltypes.Packet, + relayer sdk.AccAddress, + contractAddress, + packetSenderAddress string, + ) error + // IBCReceivePacketCallback is called in the destination chain when a packet acknowledgement is written. + // The contract is expected to handle the callback within the user defined gas limit, and handle any errors, + // out of gas, or panics gracefully. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. + IBCReceivePacketCallback( + cachedCtx sdk.Context, + packet ibcexported.PacketI, + ack ibcexported.Acknowledgement, + contractAddress string, + ) error } ``` diff --git a/modules/apps/callbacks/types/expected_keepers.go b/modules/apps/callbacks/types/expected_keepers.go index f21d3dbd923..ba168602dbe 100644 --- a/modules/apps/callbacks/types/expected_keepers.go +++ b/modules/apps/callbacks/types/expected_keepers.go @@ -14,10 +14,16 @@ type ContractKeeper interface { // packetSenderAddress is determined by the underlying module, and may be empty if the sender is // unknown or undefined. The contract is expected to handle the callback within the user defined // gas limit, and handle any errors, or panics gracefully. - // If an error is returned, the transaction will be reverted by the callbacks middleware, and the - // packet will not be sent. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, and the error will be propagated to the underlying IBC + // application, resulting in a packet send failure. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. IBCSendPacketCallback( - ctx sdk.Context, + cachedCtx sdk.Context, sourcePort string, sourceChannel string, timeoutHeight clienttypes.Height, @@ -30,9 +36,15 @@ type ContractKeeper interface { // is received. The packetSenderAddress is determined by the underlying module, and may be empty if // the sender is unknown or undefined. The contract is expected to handle the callback within the // user defined gas limit, and handle any errors, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. IBCOnAcknowledgementPacketCallback( - ctx sdk.Context, + cachedCtx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress, @@ -43,9 +55,15 @@ type ContractKeeper interface { // the timeout height. The packetSenderAddress is determined by the underlying module, and may be // empty if the sender is unknown or undefined. The contract is expected to handle the callback // within the user defined gas limit, and handle any error, out of gas, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. + // + // Implementations are provided with the packetSenderAddress and MAY choose to use this to perform + // validation on the origin of a given packet. It is recommended to perform the same validation + // on all source chain callbacks (SendPacket, AcknowledgementPacket, TimeoutPacket). This + // defensively guards against exploits due to incorrectly wired SendPacket ordering in IBC stacks. IBCOnTimeoutPacketCallback( - ctx sdk.Context, + cachedCtx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress, contractAddress, @@ -54,9 +72,10 @@ type ContractKeeper interface { // IBCReceivePacketCallback is called in the destination chain when a packet acknowledgement is written. // The contract is expected to handle the callback within the user defined gas limit, and handle any errors, // out of gas, or panics gracefully. - // If an error is returned, state will be reverted by the callbacks middleware. + // This entry point is called with a cached context. If an error is returned, then the changes in + // this context will not be persisted, but the packet lifecycle will not be blocked. IBCReceivePacketCallback( - ctx sdk.Context, + cachedCtx sdk.Context, packet ibcexported.PacketI, ack ibcexported.Acknowledgement, contractAddress string,