Supply app version to contract keeper (#7000)

* chore: adding version to UnmarshalPacketData interface * chore: addressing PR feedback * chore: adding application version to callback data * chore: adding version as additional field in contract keeper interface functions * docs: updating docs to include version argument * docs: updating changelog to include note about callbacks version argument * chore: adding application version to emmited events * docs: fix spacing * docs: fix spacing * docs: fix spacing * chore: move changelog entry to callbacks module * chore: update docs * Apply suggestions from code review * chore: add migration docs --------- Co-authored-by: Colin Axnér <[email protected]>
cosmos · Aug 5, 2024 · 4ace83d · 4ace83d
1 parent 3111025
commit 4ace83d
Show file tree

Hide file tree

Showing 14 changed files with 304 additions and 134 deletions.
diff --git a/docs/architecture/adr-008-app-caller-cbs.md b/docs/architecture/adr-008-app-caller-cbs.md
@@ -185,6 +185,9 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCSendPacketCallback(
 		cachedCtx sdk.Context,
 		sourcePort string,
@@ -194,6 +197,7 @@ type ContractKeeper interface {
 		packetData []byte,
 		contractAddress,
 		packetSenderAddress string,
+		version 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
@@ -206,13 +210,17 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCOnAcknowledgementPacketCallback(
 		cachedCtx sdk.Context,
 		packet channeltypes.Packet,
 		acknowledgement []byte,
 		relayer sdk.AccAddress,
 		contractAddress,
 		packetSenderAddress string,
+		version 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
@@ -225,23 +233,31 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCOnTimeoutPacketCallback(
 		cachedCtx sdk.Context,
 		packet channeltypes.Packet,
 		relayer sdk.AccAddress,
 		contractAddress,
 		packetSenderAddress string,
+		version 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCReceivePacketCallback(
 		cachedCtx sdk.Context,
 		packet ibcexported.PacketI,
 		ack ibcexported.Acknowledgement,
 		contractAddress string,
+		version string,
 	) error
 }
 ```

diff --git a/docs/docs/04-middleware/02-callbacks/03-interfaces.md b/docs/docs/04-middleware/02-callbacks/03-interfaces.md
@@ -86,6 +86,9 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCSendPacketCallback(
 		cachedCtx sdk.Context,
 		sourcePort string,
@@ -95,6 +98,7 @@ type ContractKeeper interface {
 		packetData []byte,
 		contractAddress,
 		packetSenderAddress string,
+		version 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
@@ -107,13 +111,17 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCOnAcknowledgementPacketCallback(
 		cachedCtx sdk.Context,
 		packet channeltypes.Packet,
 		acknowledgement []byte,
 		relayer sdk.AccAddress,
 		contractAddress,
 		packetSenderAddress string,
+		version 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
@@ -126,23 +134,31 @@ type ContractKeeper interface {
 	// 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCOnTimeoutPacketCallback(
 		cachedCtx sdk.Context,
 		packet channeltypes.Packet,
 		relayer sdk.AccAddress,
 		contractAddress,
 		packetSenderAddress string,
+		version 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.
+	//
+	// The version provided is the base application version for the given packet send. This allows
+	// contracts to determine how to unmarshal the packetData.
 	IBCReceivePacketCallback(
 		cachedCtx sdk.Context,
 		packet ibcexported.PacketI,
 		ack ibcexported.Acknowledgement,
 		contractAddress string,
+		version string,
 	) error
 }
 ```

diff --git a/docs/docs/05-migrations/13-v8-to-v9.md b/docs/docs/05-migrations/13-v8-to-v9.md
@@ -26,6 +26,7 @@ There are four sections based on the four potential user groups of this document
             - [ICS20 v2](#ics20-v2)
             - [`DenomTrace` type refactoring](#denomtrace-type-refactoring)
         - [ICS27 - Interchain Accounts](#ics27---interchain-accounts)
+        - [Callbacks](#callbacks)
         - [IBC testing package](#ibc-testing-package)
         - [API deprecation notice](#api-deprecation-notice)
     - [Relayers](#relayers)
@@ -289,6 +290,63 @@ func NewIBCMiddleware(
 - The [`InitModule` function](https://github.com/cosmos/ibc-go/blob/v8.0.0/modules/apps/27-interchain-accounts/module.go#L124-L143) has been removed. When adding the interchain accounts module to the chain, please set the desired params for controller and host submodules directly after calling `RunMigrations` in the upgrade handler.
 - The [`GetBytes()` function](https://github.com/cosmos/ibc-go/blob/v8.0.0/modules/apps/27-interchain-accounts/types/packet.go#L65-L68) of the `CosmosTx` type has been removed.
 
+### Callbacks
+
+The `ContractKeeper` interface has been extended with the base application version. The base application version will be required by contracts to unmarshal the packet data. An example of this is unmarshaling ics20v2 packets which requires knowing the base version of a transfer stack (either v1 or v2). 
+
+```diff
+       IBCSendPacketCallback(
+                cachedCtx sdk.Context,
+                sourcePort string,
+@@ -31,6 +34,7 @@ type ContractKeeper interface {
+                packetData []byte,
+                contractAddress,
+                packetSenderAddress string,
++               version 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
+@@ -43,6 +47,9 @@ type ContractKeeper interface {
+        // 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,
+@@ -50,6 +57,7 @@ type ContractKeeper interface {
+                relayer sdk.AccAddress,
+                contractAddress,
+                packetSenderAddress string,
++               version 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
+@@ -62,22 +70,30 @@ type ContractKeeper interface {
+        // 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,
++               version 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,
++               version string,
+        ) error
+```
+
 ### IBC testing package
 
 - The `mock.PV` type has been removed in favour of [`cmttypes.MockPV`](https://github.com/cometbft/cometbft/blob/v0.38.5/types/priv_validator.go#L50) in [#5709](https://github.com/cosmos/ibc-go/pull/5709).

diff --git a/modules/apps/callbacks/CHANGELOG.md b/modules/apps/callbacks/CHANGELOG.md
@@ -44,6 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (apps/callbacks)  [\#7000](https://github.com/cosmos/ibc-go/pull/7000) Add base application version to contract keeper callbacks.
+
 ### State Machine Breaking
 
 ### Improvements

diff --git a/modules/apps/callbacks/fee_transfer_test.go b/modules/apps/callbacks/fee_transfer_test.go
@@ -115,6 +115,7 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() {
 					_ sdk.AccAddress,
 					contractAddress,
 					_ string,
+					_ string,
 				) error {
 					expAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
 					s.Require().Equal(expAck, acknowledgement)

diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go
@@ -110,7 +110,7 @@ func (im IBCMiddleware) SendPacket(
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCSendPacketCallback(
-			cachedCtx, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data, callbackData.CallbackAddress, callbackData.SenderAddress,
+			cachedCtx, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data, callbackData.CallbackAddress, callbackData.SenderAddress, callbackData.ApplicationVersion,
 		)
 	}
 
@@ -151,7 +151,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCOnAcknowledgementPacketCallback(
-			cachedCtx, packet, acknowledgement, relayer, callbackData.CallbackAddress, callbackData.SenderAddress,
+			cachedCtx, packet, acknowledgement, relayer, callbackData.CallbackAddress, callbackData.SenderAddress, callbackData.ApplicationVersion,
 		)
 	}
 
@@ -184,7 +184,7 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, channelVersion string,
 	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
-		return im.contractKeeper.IBCOnTimeoutPacketCallback(cachedCtx, packet, relayer, callbackData.CallbackAddress, callbackData.SenderAddress)
+		return im.contractKeeper.IBCOnTimeoutPacketCallback(cachedCtx, packet, relayer, callbackData.CallbackAddress, callbackData.SenderAddress, callbackData.ApplicationVersion)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
@@ -220,7 +220,7 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pac
 	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
-		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress)
+		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
@@ -263,7 +263,7 @@ func (im IBCMiddleware) WriteAcknowledgement(
 	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
-		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress)
+		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions

diff --git a/modules/apps/callbacks/replay_test.go b/modules/apps/callbacks/replay_test.go
@@ -37,7 +37,7 @@ func (s *CallbacksTestSuite) TestTransferTimeoutReplayProtection() {
 				cachedCtx sdk.Context,
 				packet channeltypes.Packet,
 				_ sdk.AccAddress,
-				_, _ string,
+				_, _, _ string,
 			) error {
 				// only replay the timeout packet twice. We could replay it more times
 				callbackCount++
@@ -119,7 +119,7 @@ func (s *CallbacksTestSuite) TestTransferErrorAcknowledgementReplayProtection()
 				packet channeltypes.Packet,
 				ack []byte,
 				_ sdk.AccAddress,
-				_, _ string,
+				_, _, _ string,
 			) error {
 				// only replay the ack packet twice. We could replay it more times
 				callbackCount++
@@ -197,7 +197,7 @@ func (s *CallbacksTestSuite) TestTransferSuccessAcknowledgementReplayProtection(
 				packet channeltypes.Packet,
 				ack []byte,
 				_ sdk.AccAddress,
-				_, _ string,
+				_, _, _ string,
 			) error {
 				// only replay the ack packet twice. We could replay it more times
 				callbackCount++
@@ -265,7 +265,7 @@ func (s *CallbacksTestSuite) TestTransferRecvPacketReplayProtection() {
 				cachedCtx sdk.Context,
 				packet ibcexported.PacketI,
 				_ ibcexported.Acknowledgement,
-				_ string,
+				_, _ string,
 			) error {
 				callbackCount++
 				if callbackCount == 2 {