Ack callbacks for IBC transfers (#3589)

* added ack callbacks

* updated bad import in tests

* gofumpt

* remove unused file

* removed memo on callback packets and add tests for acks

* only removing the memo if the only key is the callback

* renamed ibc_hooks to ibchooks

* renaming helper method

* renamed callback key

* added missing store key to upgrade

* added apline debug image so that sh is available

* fixed callback param

* fixed readme

* Update x/ibc-hooks/wasm_hook.go

Co-authored-by: Adam Tucker <adam@osmosis.team>

* added test for empty memo

* renamed ibc hooks keeper

* removed unused import

* fofumpt

* deleting the key once it's no longer needed

* Update x/ibc-hooks/README.md

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* Update x/ibc-hooks/README.md

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* Update x/ibc-hooks/README.md

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* renamed packet without memo

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

Makefile

@@ -310,6 +310,10 @@ docker-build-debug:
 	@DOCKER_BUILDKIT=1 docker build -t osmosis:${COMMIT} --build-arg BASE_IMG_TAG=debug -f Dockerfile .
 	@DOCKER_BUILDKIT=1 docker tag osmosis:${COMMIT} osmosis:debug
 
+docker-build-debug-alpine:
+	@DOCKER_BUILDKIT=1 docker build -t osmosis:${COMMIT} --build-arg BASE_IMG_TAG=debug --build-arg RUNNER_IMAGE=$(RUNNER_BASE_IMAGE_ALPINE) -f Dockerfile .
+	@DOCKER_BUILDKIT=1 docker tag osmosis:${COMMIT} osmosis:debug
+
 docker-build-e2e-init-chain:
 	@DOCKER_BUILDKIT=1 docker build -t osmosis-e2e-init-chain:debug --build-arg E2E_SCRIPT_NAME=chain --platform=linux/x86_64 -f tests/e2e/initialization/init.Dockerfile .
 

app/keepers/keepers.go

@@ -37,6 +37,8 @@ import (
 	downtimedetector "github.com/osmosis-labs/osmosis/v13/x/downtime-detector"
 	downtimetypes "github.com/osmosis-labs/osmosis/v13/x/downtime-detector/types"
 	ibchooks "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks"
+	ibchookskeeper "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/keeper"
+	ibchookstypes "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/types"
 	ibcratelimit "github.com/osmosis-labs/osmosis/v13/x/ibc-rate-limit"
 	ibcratelimittypes "github.com/osmosis-labs/osmosis/v13/x/ibc-rate-limit/types"
 	"github.com/osmosis-labs/osmosis/v13/x/swaprouter"
@@ -110,6 +112,7 @@ type AppKeepers struct {
 	DowntimeKeeper               *downtimedetector.Keeper
 	SlashingKeeper               *slashingkeeper.Keeper
 	IBCKeeper                    *ibckeeper.Keeper
+	IBCHooksKeeper               *ibchookskeeper.Keeper
 	ICAHostKeeper                *icahostkeeper.Keeper
 	TransferKeeper               *ibctransferkeeper.Keeper
 	EvidenceKeeper               *evidencekeeper.Keeper
@@ -222,7 +225,13 @@ func (appKeepers *AppKeepers) InitNormalKeepers(
 		appKeepers.ScopedIBCKeeper,
 	)
 
-	appKeepers.WireICS20PreWasmKeeper(appCodec, bApp)
+	// Configure the hooks keeper
+	hooksKeeper := ibchookskeeper.NewKeeper(
+		appKeepers.keys[ibchookstypes.StoreKey],
+	)
+	appKeepers.IBCHooksKeeper = &hooksKeeper
+
+	appKeepers.WireICS20PreWasmKeeper(appCodec, bApp, appKeepers.IBCHooksKeeper)
 
 	icaHostKeeper := icahostkeeper.NewKeeper(
 		appCodec, appKeepers.keys[icahosttypes.StoreKey],
@@ -428,9 +437,11 @@ func (appKeepers *AppKeepers) InitNormalKeepers(
 // appkeepers.WasmHooks AND appKeepers.RateLimitingICS4Wrapper
 func (appKeepers *AppKeepers) WireICS20PreWasmKeeper(
 	appCodec codec.Codec,
-	bApp *baseapp.BaseApp) {
+	bApp *baseapp.BaseApp,
+	hooksKeeper *ibchookskeeper.Keeper,
+) {
 	// Setup the ICS4Wrapper used by the hooks middleware
-	wasmHooks := ibchooks.NewWasmHooks(nil) // The contract keeper needs to be set later
+	wasmHooks := ibchooks.NewWasmHooks(hooksKeeper, nil) // The contract keeper needs to be set later
 	appKeepers.Ics20WasmHooks = &wasmHooks
 	appKeepers.HooksICS4Wrapper = ibchooks.NewICS4Middleware(
 		appKeepers.IBCKeeper.ChannelKeeper,
@@ -640,5 +651,6 @@ func KVStoreKeys() []string {
 		tokenfactorytypes.StoreKey,
 		valsetpreftypes.StoreKey,
 		protorevtypes.StoreKey,
+		ibchookstypes.StoreKey,
 	}
 }

app/modules.go

@@ -9,6 +9,7 @@ import (
 	ibc "github.com/cosmos/ibc-go/v3/modules/core"
 	ibchost "github.com/cosmos/ibc-go/v3/modules/core/24-host"
 	ibckeeper "github.com/cosmos/ibc-go/v3/modules/core/keeper"
+	ibchookstypes "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/types"
 
 	ica "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts"
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
@@ -85,7 +86,7 @@ import (
 var moduleAccountPermissions = map[string][]string{
 	authtypes.FeeCollectorName:               nil,
 	distrtypes.ModuleName:                    nil,
-	ibc_hooks.ModuleName:                     nil,
+	ibchookstypes.ModuleName:                 nil,
 	icatypes.ModuleName:                      nil,
 	minttypes.ModuleName:                     {authtypes.Minter, authtypes.Burner},
 	minttypes.DeveloperVestingModuleAcctName: nil,
@@ -242,7 +243,7 @@ func OrderInitGenesis(allModuleNames []string) []string {
 		// wasm after ibc transfer
 		wasm.ModuleName,
 		// ibc_hooks after auth keeper
-		ibc_hooks.ModuleName,
+		ibchookstypes.ModuleName,
 	}
 }
 

app/upgrades/v13/upgrade_test.go

@@ -2,6 +2,7 @@ package v13_test
 
 import (
 	"fmt"
+	ibchookstypes "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/types"
 	"testing"
 
 	ibcratelimittypes "github.com/osmosis-labs/osmosis/v13/x/ibc-rate-limit/types"
@@ -58,14 +59,14 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 				suite.App.AccountKeeper.RemoveAccount(suite.Ctx, acc)
 				// Because of SDK version map bug, we can't do the following, and instaed do a massive hack
 				// vm := suite.App.UpgradeKeeper.GetModuleVersionMap(suite.Ctx)
-				// delete(vm, ibc_hooks.ModuleName)
+				// delete(vm, ibchookstypes.ModuleName)
 				// OR
-				// vm[ibc_hooks.ModuleName] = 0
+				// vm[ibchookstypes.ModuleName] = 0
 				// suite.App.UpgradeKeeper.SetModuleVersionMap(suite.Ctx, vm)
 				upgradeStoreKey := suite.App.AppKeepers.GetKey(upgradetypes.StoreKey)
 				store := suite.Ctx.KVStore(upgradeStoreKey)
 				versionStore := prefix.NewStore(store, []byte{upgradetypes.VersionMapByte})
-				versionStore.Delete([]byte(ibc_hooks.ModuleName))
+				versionStore.Delete([]byte(ibchookstypes.ModuleName))
 
 				hasAcc := suite.App.AccountKeeper.HasAccount(suite.Ctx, ibc_hooks.WasmHookModuleAccountAddr)
 				suite.Require().False(hasAcc)

app/upgrades/v14/constants.go

@@ -2,6 +2,7 @@ package v14
 
 import (
 	store "github.com/cosmos/cosmos-sdk/store/types"
+	ibchookstypes "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/types"
 
 	"github.com/osmosis-labs/osmosis/v13/app/upgrades"
 	downtimetypes "github.com/osmosis-labs/osmosis/v13/x/downtime-detector/types"
@@ -17,7 +18,7 @@ var Upgrade = upgrades.Upgrade{
 	UpgradeName:          UpgradeName,
 	CreateUpgradeHandler: CreateUpgradeHandler,
 	StoreUpgrades: store.StoreUpgrades{
-		Added:   []string{valsetpreftypes.StoreKey, protorevtypes.StoreKey, swaproutertypes.StoreKey, downtimetypes.StoreKey},
+		Added:   []string{valsetpreftypes.StoreKey, protorevtypes.StoreKey, swaproutertypes.StoreKey, downtimetypes.StoreKey, ibchookstypes.StoreKey},
 		Deleted: []string{},
 	},
 }

x/ibc-hooks/README.md

@@ -114,6 +114,42 @@ In wasm hooks, post packet execution:
 * if wasm message has error, return ErrAck
 * otherwise continue through middleware
 
+## Ack callbacks
 
+A contract that sends an IBC transfer, may need to listen for the ACK from that packet. To allow
+contracts to listen on the ack of specific packets, we provide Ack callbacks. 
 
-### Testing strategy
+### Design
+
+The sender of an IBC transfer packet may specify a callback for when the ack of that packet is received in the memo 
+field of the transfer packet. 
+
+Crucially, _only_ the IBC packet sender can set the callback.
+
+### Use case
+
+The crosschain swaps implementation sends an IBC transfer. If the transfer were to fail, we want to allow the sender
+to be able to retrieve their funds (which would otherwise be stuck in the contract). To do this, we allow users to 
+retrieve the funds after the timeout has passed, but without the ack information, we cannot guarantee that the send 
+hasn't failed (i.e.: returned an error ack notifying that the receiving change didn't accept it)
+
+### Implementation
+
+#### Callback information in memo
+
+For the callback to be processed, the transfer packet's memo should contain the following in its JSON:
+
+`{"ibc_callback": "osmo1contractAddr"}`
+
+The wasm hooks will keep the mapping from the packet's channel and sequence to the contract in storage. When an ack is
+received, it will notify the specified contract via a sudo message.
+
+#### Interface for receiving the Ack
+
+The contract that awaits the callback should implement the following interface for a sudo message:
+
+* `ReceiveAck { channel: String, sequence: u64, ack: String, success: bool }`
+
+# Testing strategy
+
+See go tests.

x/ibc-hooks/genesis.go

@@ -4,10 +4,12 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/address"
 
+	"github.com/osmosis-labs/osmosis/v13/x/ibc-hooks/types"
+
 	"github.com/osmosis-labs/osmosis/v13/osmoutils"
 )
 
-var WasmHookModuleAccountAddr sdk.AccAddress = address.Module(ModuleName, []byte("wasm-hook intermediary account"))
+var WasmHookModuleAccountAddr sdk.AccAddress = address.Module(types.ModuleName, []byte("wasm-hook intermediary account"))
 
 func IbcHooksInitGenesis(ctx sdk.Context, ak osmoutils.AccountKeeper) {
 	err := osmoutils.CreateModuleAccount(ctx, ak, WasmHookModuleAccountAddr)

x/ibc-hooks/ibc_middleware_test.go

@@ -5,7 +5,7 @@ import (
 	"fmt"
 	"testing"
 
-	ibc_hooks "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks"
+	ibchooks "github.com/osmosis-labs/osmosis/v13/x/ibc-hooks"
 
 	"github.com/osmosis-labs/osmosis/v13/osmoutils"
 
@@ -201,7 +201,7 @@ func (suite *HooksTestSuite) receivePacketWithSequence(receiver, memo string, pr
 func (suite *HooksTestSuite) TestRecvTransferWithMetadata() {
 	// Setup contract
 	suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/echo.wasm")
-	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}")
+	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}", 1)
 
 	ackBytes := suite.receivePacket(addr.String(), fmt.Sprintf(`{"wasm": {"contract": "%s", "msg": {"echo": {"msg": "test"} } } }`, addr))
 	ackStr := string(ackBytes)
@@ -217,7 +217,7 @@ func (suite *HooksTestSuite) TestRecvTransferWithMetadata() {
 func (suite *HooksTestSuite) TestFundsAreTransferredToTheContract() {
 	// Setup contract
 	suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/echo.wasm")
-	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}")
+	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}", 1)
 
 	// Check that the contract has no funds
 	localDenom := osmoutils.MustExtractDenomFromPacketOnRecv(suite.makeMockPacket("", "", 0))
@@ -243,7 +243,7 @@ func (suite *HooksTestSuite) TestFundsAreTransferredToTheContract() {
 func (suite *HooksTestSuite) TestFundsAreReturnedOnFailedContractExec() {
 	// Setup contract
 	suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/echo.wasm")
-	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}")
+	addr := suite.chainA.InstantiateContract(&suite.Suite, "{}", 1)
 
 	// Check that the contract has no funds
 	localDenom := osmoutils.MustExtractDenomFromPacketOnRecv(suite.makeMockPacket("", "", 0))
@@ -311,7 +311,7 @@ func (suite *HooksTestSuite) TestPacketsThatShouldBeSkipped() {
 func (suite *HooksTestSuite) TestFundTracking() {
 	// Setup contract
 	suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/counter.wasm")
-	addr := suite.chainA.InstantiateContract(&suite.Suite, `{"count": 0}`)
+	addr := suite.chainA.InstantiateContract(&suite.Suite, `{"count": 0}`, 1)
 
 	// Check that the contract has no funds
 	localDenom := osmoutils.MustExtractDenomFromPacketOnRecv(suite.makeMockPacket("", "", 0))
@@ -325,29 +325,140 @@ func (suite *HooksTestSuite) TestFundTracking() {
 
 	state := suite.chainA.QueryContract(
 		&suite.Suite, addr,
-		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, ibc_hooks.WasmHookModuleAccountAddr)))
+		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, ibchooks.WasmHookModuleAccountAddr)))
 	suite.Require().Equal(`{"count":0}`, state)
 
 	state = suite.chainA.QueryContract(
 		&suite.Suite, addr,
-		[]byte(fmt.Sprintf(`{"get_total_funds": {"addr": "%s"}}`, ibc_hooks.WasmHookModuleAccountAddr)))
-	suite.Require().Equal(`{"total_funds":[]}`, state)
+		[]byte(fmt.Sprintf(`{"get_total_funds": {"addr": "%s"}}`, ibchooks.WasmHookModuleAccountAddr)))
+	suite.Require().Equal(`{"total_funds":[{"denom":"ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878","amount":"1"}]}`, state)
 
 	suite.receivePacketWithSequence(
 		addr.String(),
 		fmt.Sprintf(`{"wasm": {"contract": "%s", "msg": {"increment": {} } } }`, addr), 1)
 
 	state = suite.chainA.QueryContract(
 		&suite.Suite, addr,
-		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, ibc_hooks.WasmHookModuleAccountAddr)))
+		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, ibchooks.WasmHookModuleAccountAddr)))
 	suite.Require().Equal(`{"count":1}`, state)
 
 	state = suite.chainA.QueryContract(
 		&suite.Suite, addr,
-		[]byte(fmt.Sprintf(`{"get_total_funds": {"addr": "%s"}}`, ibc_hooks.WasmHookModuleAccountAddr)))
-	suite.Require().Equal(`{"total_funds":[{"denom":"ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878","amount":"1"}]}`, state)
+		[]byte(fmt.Sprintf(`{"get_total_funds": {"addr": "%s"}}`, ibchooks.WasmHookModuleAccountAddr)))
+	suite.Require().Equal(`{"total_funds":[{"denom":"ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B77878","amount":"2"}]}`, state)
 
 	// Check that the token has now been transferred to the contract
 	balance = suite.chainA.GetOsmosisApp().BankKeeper.GetBalance(suite.chainA.GetContext(), addr, localDenom)
 	suite.Require().Equal(sdk.NewInt(2), balance.Amount)
 }
+
+// custom MsgTransfer constructor that supports Memo
+func NewMsgTransfer(
+	token sdk.Coin, sender, receiver string, memo string,
+) *transfertypes.MsgTransfer {
+	return &transfertypes.MsgTransfer{
+		SourcePort:       "transfer",
+		SourceChannel:    "channel-0",
+		Token:            token,
+		Sender:           sender,
+		Receiver:         receiver,
+		TimeoutHeight:    clienttypes.NewHeight(0, 100),
+		TimeoutTimestamp: 0,
+		Memo:             memo,
+	}
+}
+
+type Direction int64
+
+const (
+	AtoB Direction = iota
+	BtoA
+)
+
+func (suite *HooksTestSuite) GetEndpoints(direction Direction) (sender *ibctesting.Endpoint, receiver *ibctesting.Endpoint) {
+	switch direction {
+	case AtoB:
+		sender = suite.path.EndpointA
+		receiver = suite.path.EndpointB
+	case BtoA:
+		sender = suite.path.EndpointB
+		receiver = suite.path.EndpointA
+	}
+	return sender, receiver
+}
+
+func (suite *HooksTestSuite) RelayPacket(packet channeltypes.Packet, direction Direction) (*sdk.Result, []byte) {
+	sender, receiver := suite.GetEndpoints(direction)
+
+	err := receiver.UpdateClient()
+	suite.Require().NoError(err)
+
+	// receiver Receives
+	receiveResult, err := receiver.RecvPacketWithResult(packet)
+	suite.Require().NoError(err)
+
+	ack, err := ibctesting.ParseAckFromEvents(receiveResult.GetEvents())
+	suite.Require().NoError(err)
+
+	// sender Acknowledges
+	err = sender.AcknowledgePacket(packet, ack)
+	suite.Require().NoError(err)
+
+	err = sender.UpdateClient()
+	suite.Require().NoError(err)
+	err = receiver.UpdateClient()
+	suite.Require().NoError(err)
+
+	return receiveResult, ack
+}
+
+func (suite *HooksTestSuite) FullSend(msg sdk.Msg, direction Direction) (*sdk.Result, *sdk.Result, string, error) {
+	var sender *osmosisibctesting.TestChain
+	switch direction {
+	case AtoB:
+		sender = suite.chainA
+	case BtoA:
+		sender = suite.chainB
+	}
+	sendResult, err := sender.SendMsgsNoCheck(msg)
+	suite.Require().NoError(err)
+
+	packet, err := ibctesting.ParsePacketFromEvents(sendResult.GetEvents())
+	suite.Require().NoError(err)
+
+	receiveResult, ack := suite.RelayPacket(packet, direction)
+
+	return sendResult, receiveResult, string(ack), err
+}
+
+func (suite *HooksTestSuite) TestAcks() {
+	suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/counter.wasm")
+	addr := suite.chainA.InstantiateContract(&suite.Suite, `{"count": 0}`, 1)
+
+	// Generate swap instructions for the contract
+	callbackMemo := fmt.Sprintf(`{"ibc_callback":"%s"}`, addr)
+	// Send IBC transfer with the memo with crosschain-swap instructions
+	transferMsg := NewMsgTransfer(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000)), suite.chainA.SenderAccount.GetAddress().String(), addr.String(), callbackMemo)
+	suite.FullSend(transferMsg, AtoB)
+
+	// The test contract will increment the counter for itself every time it receives an ack
+	state := suite.chainA.QueryContract(
+		&suite.Suite, addr,
+		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, addr)))
+	suite.Require().Equal(`{"count":1}`, state)
+
+	suite.FullSend(transferMsg, AtoB)
+	state = suite.chainA.QueryContract(
+		&suite.Suite, addr,
+		[]byte(fmt.Sprintf(`{"get_count": {"addr": "%s"}}`, addr)))
+	suite.Require().Equal(`{"count":2}`, state)
+
+}
+
+func (suite *HooksTestSuite) TestSendWithoutMemo() {
+	// Sending a packet without memo to ensure that the ibc_callback middleware doesn't interfere with a regular send
+	transferMsg := NewMsgTransfer(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000)), suite.chainA.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), "")
+	_, _, ack, err := suite.FullSend(transferMsg, AtoB)
+	suite.Require().NoError(err)
+	suite.Require().Contains(ack, "result")
+}