Revert application state changes on failed acknowledgements (#107)

* make changes and start fixing tests

* add msg server test

* change to revert on SendCoins fail

* update interfaces and fix tests

* self review fixes

* Update modules/core/04-channel/types/acknowledgement.go

* Add Changelog

* update docs

* Update CHANGELOG.md

Co-authored-by: Aditya <[email protected]>

* add note for async acks

Co-authored-by: Aditya Sripal <[email protected]>

CHANGELOG.md

@@ -36,8 +36,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-## API Breaking
+### API Breaking 
 
+* (modules) [\#107](https://github.com/cosmos/ibc-go/pull/107) Modify OnRecvPacket callback to return an acknowledgement which indicates if it is successful or not. Callback state changes are discarded for unsuccessful acknowledgements only. 
 * (modules) [\#108](https://github.com/cosmos/ibc-go/pull/108) All message constructors take the signer as a string to prevent upstream bugs. The `String()` function for an SDK Acc Address relies on external context.
 
 ### State Machine Breaking

docs/custom.md

@@ -295,49 +295,46 @@ invoked by the IBC module after the packet has been proved valid and correctly p
 keepers. Thus, the `OnRecvPacket` callback only needs to worry about making the appropriate state
 changes given the packet data without worrying about whether the packet is valid or not.
 
-Modules may return an acknowledgement as a byte string and return it to the IBC handler.
+Modules may return to the IBC handler an acknowledgement which implements the Acknowledgement interface.
 The IBC handler will then commit this acknowledgement of the packet so that a relayer may relay the
 acknowledgement back to the sender module.
 
+The state changes that occurred during this callback will only be written if:
+- the acknowledgement was successful as indicated by the `Success()` function of the acknowledgement
+- if the acknowledgement returned is nil indicating that an asynchronous process is occurring
+
+NOTE: Applications which process asynchronous acknowledgements must handle reverting state changes
+when appropriate. Any state changes that occurred during the `OnRecvPacket` callback will be written 
+for asynchronous acknowledgements. 
+
 ```go
 OnRecvPacket(
     ctx sdk.Context,
     packet channeltypes.Packet,
-) (res *sdk.Result, ack []byte, abort error) {
+) ibcexported.Acknowledgement {
     // Decode the packet data
     packetData := DecodePacketData(packet.Data)
 
-    // do application state changes based on packet data
-    // and return result, acknowledgement and abortErr
-    // Note: abortErr is only not nil if we need to abort the entire receive packet, and allow a replay of the receive.
-    // If the application state change failed but we do not want to replay the packet,
-    // simply encode this failure with relevant information in ack and return nil error
-    res, ack, abortErr := processPacket(ctx, packet, packetData)
-
-    // if we need to abort the entire receive packet, return error
-    if abortErr != nil {
-        return nil, nil, abortErr
-    }
-
-    // Encode the ack since IBC expects acknowledgement bytes
-    ackBytes := EncodeAcknowledgement(ack)
+    // do application state changes based on packet data and return the acknowledgement
+    // NOTE: The acknowledgement will indicate to the IBC handler if the application 
+    // state changes should be written via the `Success()` function. Application state
+    // changes are only written if the acknowledgement is successful or the acknowledgement
+    // returned is nil indicating that an asynchronous acknowledgement will occur.
+    ack := processPacket(ctx, packet, packetData)
 
-    return res, ackBytes, nil
+    return ack
 }
 ```
 
-::: warning
-`OnRecvPacket` should **only** return an error if we want the entire receive packet execution
-(including the IBC handling) to be reverted. This will allow the packet to be replayed in the case
-that some mistake in the relaying caused the packet processing to fail.
-
-If some application-level error happened while processing the packet data, in most cases, we will
-not want the packet processing to revert. Instead, we may want to encode this failure into the
-acknowledgement and finish processing the packet. This will ensure the packet cannot be replayed,
-and will also allow the sender module to potentially remediate the situation upon receiving the
-acknowledgement. An example of this technique is in the `ibc-transfer` module's
-[`OnRecvPacket`](https://github.com/cosmos/ibc-go/tree/main/modules/apps/transfer/module.go).
-:::
+The Acknowledgement interface:
+```go
+// Acknowledgement defines the interface used to return
+// acknowledgements in the OnRecvPacket callback.
+type Acknowledgement interface {
+	Success() bool
+	Acknowledgement() []byte
+}
+```
 
 ### Acknowledgements
 

docs/migrations/ibc-migration-043.md

@@ -91,3 +91,11 @@ REST routes are not supported for these proposals.
 ## Proto file changes
 
 The gRPC querier service endpoints have changed slightly. The previous files used `v1beta1`, this has been updated to `v1`.
+
+## IBC callback changes
+
+### OnRecvPacket
+
+Application developers need to update their `OnRecvPacket` callback logic. 
+
+The `OnRecvPacket` callback has been modified to only return the acknowledgement. The acknowledgement returned must implement the `Acknowledgement` interface. The acknowledgement should indicate if it represents a successful processing of a packet by returning true on `Success()` and false in all other cases. A return value of false on `Success()` will result in all state changes which occurred in the callback being discarded. More information can be found in the [documentation](https://github.com/cosmos/ibc-go/blob/main/docs/custom.md#receiving-packets).

modules/apps/transfer/handler_test.go

@@ -52,7 +52,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	fungibleTokenPacket := types.NewFungibleTokenPacketData(coinToSendToB.Denom, coinToSendToB.Amount.Uint64(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String())
 	packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, 0)
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
-	err = suite.coordinator.RelayPacket(suite.chainA, suite.chainB, clientA, clientB, packet, ack.GetBytes())
+	err = suite.coordinator.RelayPacket(suite.chainA, suite.chainB, clientA, clientB, packet, ack.Acknowledgement())
 	suite.Require().NoError(err) // relay committed
 
 	// check that voucher exists on chain B
@@ -77,7 +77,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	fullDenomPath := types.GetPrefixedDenom(channelOnCForB.PortID, channelOnCForB.ID, voucherDenomTrace.GetFullDenomPath())
 	fungibleTokenPacket = types.NewFungibleTokenPacketData(voucherDenomTrace.GetFullDenomPath(), coinSentFromAToB.Amount.Uint64(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainC.SenderAccount.GetAddress().String())
 	packet = channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelOnBForC.PortID, channelOnBForC.ID, channelOnCForB.PortID, channelOnCForB.ID, timeoutHeight, 0)
-	err = suite.coordinator.RelayPacket(suite.chainB, suite.chainC, clientOnBForC, clientOnCForB, packet, ack.GetBytes())
+	err = suite.coordinator.RelayPacket(suite.chainB, suite.chainC, clientOnBForC, clientOnCForB, packet, ack.Acknowledgement())
 	suite.Require().NoError(err) // relay committed
 
 	coinSentFromBToC := sdk.NewInt64Coin(types.ParseDenomTrace(fullDenomPath).IBCDenom(), 100)
@@ -100,7 +100,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	// NOTE: fungible token is prefixed with the full trace in order to verify the packet commitment
 	fungibleTokenPacket = types.NewFungibleTokenPacketData(fullDenomPath, coinSentFromBToC.Amount.Uint64(), suite.chainC.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String())
 	packet = channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelOnCForB.PortID, channelOnCForB.ID, channelOnBForC.PortID, channelOnBForC.ID, timeoutHeight, 0)
-	err = suite.coordinator.RelayPacket(suite.chainC, suite.chainB, clientOnCForB, clientOnBForC, packet, ack.GetBytes())
+	err = suite.coordinator.RelayPacket(suite.chainC, suite.chainB, clientOnCForB, clientOnBForC, packet, ack.Acknowledgement())
 	suite.Require().NoError(err) // relay committed
 
 	balance = suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), coinSentFromAToB.Denom)

modules/apps/transfer/keeper/relay.go

@@ -296,7 +296,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 	if err := k.bankKeeper.SendCoinsFromModuleToAccount(
 		ctx, types.ModuleName, receiver, sdk.NewCoins(voucher),
 	); err != nil {
-		panic(fmt.Sprintf("unable to send coins from module to account despite previously minting coins to module account: %v", err))
+		return err
 	}
 
 	defer func() {

modules/apps/transfer/keeper/relay_test.go

@@ -198,7 +198,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 				fungibleTokenPacket := types.NewFungibleTokenPacketData(coinFromBToA.Denom, coinFromBToA.Amount.Uint64(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelB.PortID, channelB.ID, channelA.PortID, channelA.ID, clienttypes.NewHeight(0, 110), 0)
 				ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
-				err = suite.coordinator.RelayPacket(suite.chainB, suite.chainA, clientB, clientA, packet, ack.GetBytes())
+				err = suite.coordinator.RelayPacket(suite.chainB, suite.chainA, clientB, clientA, packet, ack.Acknowledgement())
 				suite.Require().NoError(err) // relay committed
 
 				seq++

modules/apps/transfer/module.go

@@ -7,13 +7,6 @@ import (
 	"math"
 	"math/rand"
 
-	"github.com/grpc-ecosystem/grpc-gateway/runtime"
-
-	"github.com/gorilla/mux"
-	"github.com/spf13/cobra"
-
-	abci "github.com/tendermint/tendermint/abci/types"
-
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/codec"
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
@@ -22,13 +15,19 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/module"
 	simtypes "github.com/cosmos/cosmos-sdk/types/simulation"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
+	"github.com/gorilla/mux"
+	"github.com/grpc-ecosystem/grpc-gateway/runtime"
+	"github.com/spf13/cobra"
+	abci "github.com/tendermint/tendermint/abci/types"
+
 	"github.com/cosmos/ibc-go/modules/apps/transfer/client/cli"
 	"github.com/cosmos/ibc-go/modules/apps/transfer/keeper"
 	"github.com/cosmos/ibc-go/modules/apps/transfer/simulation"
 	"github.com/cosmos/ibc-go/modules/apps/transfer/types"
 	channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/modules/core/05-port/types"
 	host "github.com/cosmos/ibc-go/modules/core/24-host"
+	ibcexported "github.com/cosmos/ibc-go/modules/core/exported"
 )
 
 var (
@@ -318,21 +317,27 @@ func (am AppModule) OnChanCloseConfirm(
 	return nil
 }
 
-// OnRecvPacket implements the IBCModule interface
+// OnRecvPacket implements the IBCModule interface. A successful acknowledgement
+// is returned if the packet data is succesfully decoded and the receive application
+// logic returns without error.
 func (am AppModule) OnRecvPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
-) (*sdk.Result, []byte, error) {
+) ibcexported.Acknowledgement {
+	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
+
 	var data types.FungibleTokenPacketData
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
+		ack = channeltypes.NewErrorAcknowledgement(fmt.Sprintf("cannot unmarshal ICS-20 transfer packet data: %s", err.Error()))
 	}
 
-	acknowledgement := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
-
-	err := am.keeper.OnRecvPacket(ctx, packet, data)
-	if err != nil {
-		acknowledgement = channeltypes.NewErrorAcknowledgement(err.Error())
+	// only attempt the application logic if the packet data
+	// was successfully decoded
+	if ack.Success() {
+		err := am.keeper.OnRecvPacket(ctx, packet, data)
+		if err != nil {
+			ack = channeltypes.NewErrorAcknowledgement(err.Error())
+		}
 	}
 
 	ctx.EventManager().EmitEvent(
@@ -342,14 +347,12 @@ func (am AppModule) OnRecvPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
-			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", err != nil)),
+			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
 		),
 	)
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
-	return &sdk.Result{
-		Events: ctx.EventManager().Events().ToABCIEvents(),
-	}, acknowledgement.GetBytes(), nil
+	return ack
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface

modules/core/03-connection/keeper/verify_test.go

@@ -283,7 +283,7 @@ func (suite *KeeperTestSuite) TestVerifyPacketCommitment() {
 				connection.ClientId = ibctesting.InvalidID
 			}
 
-			packet := channeltypes.NewPacket(ibctesting.TestHash, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
+			packet := channeltypes.NewPacket(ibctesting.MockPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)
 
@@ -344,7 +344,7 @@ func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgement() {
 			}
 
 			// send and receive packet
-			packet := channeltypes.NewPacket(ibctesting.TestHash, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
+			packet := channeltypes.NewPacket(ibctesting.MockPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)
 
@@ -360,12 +360,12 @@ func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgement() {
 
 			ack := ibcmock.MockAcknowledgement
 			if tc.changeAcknowledgement {
-				ack = []byte(ibctesting.InvalidID)
+				ack = ibcmock.MockFailAcknowledgement
 			}
 
 			err = suite.chainA.App.IBCKeeper.ConnectionKeeper.VerifyPacketAcknowledgement(
 				suite.chainA.GetContext(), connection, malleateHeight(proofHeight, tc.heightDiff), proof,
-				packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack,
+				packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack.Acknowledgement(),
 			)
 
 			if tc.expPass {
@@ -412,7 +412,7 @@ func (suite *KeeperTestSuite) TestVerifyPacketReceiptAbsence() {
 			}
 
 			// send, only receive if specified
-			packet := channeltypes.NewPacket(ibctesting.TestHash, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
+			packet := channeltypes.NewPacket(ibctesting.MockPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)
 
@@ -481,7 +481,7 @@ func (suite *KeeperTestSuite) TestVerifyNextSequenceRecv() {
 			}
 
 			// send and receive packet
-			packet := channeltypes.NewPacket(ibctesting.TestHash, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
+			packet := channeltypes.NewPacket(ibctesting.MockPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, defaultTimeoutHeight, 0)
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)