refactor: cctx validate outbound (#2317)

changelog.md

@@ -47,6 +47,7 @@
 * [2306](https://github.com/zeta-chain/node/pull/2306) - refactor zetaclient outbound transaction signing logic
 * [2296](https://github.com/zeta-chain/node/pull/2296) - move `testdata` package to `testutil` to organize test-related utilities
 * [2344](https://github.com/zeta-chain/node/pull/2344) - group common data of EVM/Bitcoin signer and observer using base structs
+* [2317](https://github.com/zeta-chain/node/pull/2317) - add ValidateOutbound method for cctx orchestrator
 
 ### Tests
 

testutil/keeper/crosschain.go

@@ -270,9 +270,23 @@ func GetCrosschainFungibleMock(t testing.TB, keeper *keeper.Keeper) *crosschainm
 }
 
 func MockGetSupportedChainFromChainID(m *crosschainmocks.CrosschainObserverKeeper, senderChain *chains.Chain) {
-	m.On("GetSupportedChainFromChainID", mock.Anything, senderChain.ChainId).
-		Return(senderChain).Once()
+	if senderChain != nil {
+		m.On("GetSupportedChainFromChainID", mock.Anything, senderChain.ChainId).
+			Return(senderChain).Once()
+	} else {
+		m.On("GetSupportedChainFromChainID", mock.Anything, mock.Anything).
+			Return(&chains.Chain{}).Once()
+	}
+}
 
+func MockFailedGetSupportedChainFromChainID(m *crosschainmocks.CrosschainObserverKeeper, senderChain *chains.Chain) {
+	if senderChain != nil {
+		m.On("GetSupportedChainFromChainID", mock.Anything, senderChain.ChainId).
+			Return(nil).Once()
+	} else {
+		m.On("GetSupportedChainFromChainID", mock.Anything, mock.Anything).
+			Return(nil).Once()
+	}
 }
 
 func MockGetRevertGasLimitForERC20(
@@ -329,7 +343,6 @@ func MockPayGasAndUpdateCCTX(
 		Once()
 	m.On("CallZRC20Burn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 		Return(nil).Once()
-
 }
 
 func MockUpdateNonce(m *crosschainmocks.CrosschainObserverKeeper, senderChain chains.Chain) (nonce uint64) {

x/crosschain/keeper/cctx_gateway_observers.go

@@ -55,6 +55,5 @@ func (c CCTXGatewayObservers) InitiateOutbound(
 		return types.CctxStatus_Aborted
 	}
 	commit()
-	cctx.SetPendingOutbound("")
 	return types.CctxStatus_PendingOutbound
 }

x/crosschain/keeper/cctx_gateway_zevm.go

@@ -1,8 +1,6 @@
 package keeper
 
 import (
-	"fmt"
-
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/zeta-chain/zetacore/x/crosschain/types"
@@ -20,81 +18,21 @@ func NewCCTXGatewayZEVM(crosschainKeeper Keeper) CCTXGatewayZEVM {
 	}
 }
 
-/*
-InitiateOutbound handles evm deposit and call ValidateOutbound.
-TODO (https://github.com/zeta-chain/node/issues/2278): move remaining of this comment to ValidateOutbound once it's added.
-
-  - If the deposit is successful, the CCTX status is changed to OutboundMined.
-
-  - If the deposit returns an internal error i.e if HandleEVMDeposit() returns an error, but isContractReverted is false, the CCTX status is changed to Aborted.
-
-  - If the deposit is reverted, the function tries to create a revert cctx with status PendingRevert.
-
-  - If the creation of revert tx also fails it changes the status to Aborted.
-
-Note : Aborted CCTXs are not refunded in this function. The refund is done using a separate refunding mechanism.
-We do not return an error from this function , as all changes need to be persisted to the state.
-Instead we use a temporary context to make changes and then commit the context on for the happy path ,i.e cctx is set to OutboundMined.
-New CCTX status after preprocessing is returned.
-*/
+// InitiateOutbound handles evm deposit and immediately validates pending outbound
 func (c CCTXGatewayZEVM) InitiateOutbound(ctx sdk.Context, cctx *types.CrossChainTx) (newCCTXStatus types.CctxStatus) {
 	tmpCtx, commit := ctx.CacheContext()
 	isContractReverted, err := c.crosschainKeeper.HandleEVMDeposit(tmpCtx, cctx)
 
-	// TODO (https://github.com/zeta-chain/node/issues/2278): further processing will be in validateOutbound(...), for now keeping it here
 	if err != nil && !isContractReverted {
 		// exceptional case; internal error; should abort CCTX
 		cctx.SetAbort(err.Error())
 		return types.CctxStatus_Aborted
-	} else if err != nil && isContractReverted {
-		// contract call reverted; should refund via a revert tx
-		revertMessage := err.Error()
-		senderChain := c.crosschainKeeper.zetaObserverKeeper.GetSupportedChainFromChainID(ctx, cctx.InboundParams.SenderChainId)
-		if senderChain == nil {
-			cctx.SetAbort(fmt.Sprintf("invalid sender chain id %d", cctx.InboundParams.SenderChainId))
-			return types.CctxStatus_Aborted
-		}
-		gasLimit, err := c.crosschainKeeper.GetRevertGasLimit(ctx, *cctx)
-		if err != nil {
-			cctx.SetAbort(fmt.Sprintf("revert gas limit error: %s", err.Error()))
-			return types.CctxStatus_Aborted
-		}
-		if gasLimit == 0 {
-			// use same gas limit of outbound as a fallback -- should not be required
-			gasLimit = cctx.GetCurrentOutboundParam().GasLimit
-		}
+	}
 
-		err = cctx.AddRevertOutbound(gasLimit)
-		if err != nil {
-			cctx.SetAbort(fmt.Sprintf("revert outbound error: %s", err.Error()))
-			return types.CctxStatus_Aborted
-		}
-		// we create a new cached context, and we don't commit the previous one with EVM deposit
-		tmpCtxRevert, commitRevert := ctx.CacheContext()
-		err = func() error {
-			err := c.crosschainKeeper.PayGasAndUpdateCctx(
-				tmpCtxRevert,
-				senderChain.ChainId,
-				cctx,
-				cctx.InboundParams.Amount,
-				false,
-			)
-			if err != nil {
-				return err
-			}
-			// Update nonce using senderchain id as this is a revert tx and would go back to the original sender
-			return c.crosschainKeeper.UpdateNonce(tmpCtxRevert, senderChain.ChainId, cctx)
-		}()
-		if err != nil {
-			cctx.SetAbort(fmt.Sprintf("deposit revert message: %s err : %s", revertMessage, err.Error()))
-			return types.CctxStatus_Aborted
-		}
-		commitRevert()
-		cctx.SetPendingRevert(revertMessage)
-		return types.CctxStatus_PendingRevert
+	newCCTXStatus = c.crosschainKeeper.ValidateOutboundZEVM(ctx, cctx, err, isContractReverted)
+	if newCCTXStatus == types.CctxStatus_OutboundMined {
+		commit()
 	}
-	// successful HandleEVMDeposit;
-	commit()
-	cctx.SetOutboundMined("Remote omnichain contract call completed")
-	return types.CctxStatus_OutboundMined
+
+	return newCCTXStatus
 }

x/crosschain/keeper/process_outbound.go → x/crosschain/keeper/cctx_orchestrator_validate_outbound.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	cosmoserrors "cosmossdk.io/errors"
+	"cosmossdk.io/math"
 	tmbytes "github.com/cometbft/cometbft/libs/bytes"
 	tmtypes "github.com/cometbft/cometbft/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -17,9 +18,54 @@ import (
 	observertypes "github.com/zeta-chain/zetacore/x/observer/types"
 )
 
-// ProcessOutbound processes the finalization of an outbound transaction based on the ballot status
-// The state is committed only if the individual steps are successful
-func (k Keeper) ProcessOutbound(
+/*
+ValidateOutboundZEVM processes the finalization of an outbound transaction if receiver is ZEVM.
+It takes deposit error and information if contract revert happened during deposit, to make a decision:
+
+- If the deposit was successful, the CCTX status is changed to OutboundMined.
+
+- If the deposit returned an internal error, but isContractReverted is false, the CCTX status is changed to Aborted.
+
+- If the deposit is reverted, the function tries to create a revert cctx with status PendingRevert.
+
+- If the creation of revert tx also fails it changes the status to Aborted.
+
+Note : Aborted CCTXs are not refunded in this function. The refund is done using a separate refunding mechanism.
+We do not return an error from this function, as all changes need to be persisted to the state.
+Instead we use a temporary context to make changes and then commit the context on for the happy path, i.e cctx is set to OutboundMined.
+New CCTX status after preprocessing is returned.
+*/
+func (k Keeper) ValidateOutboundZEVM(
+	ctx sdk.Context,
+	cctx *types.CrossChainTx,
+	depositErr error,
+	isContractReverted bool,
+) (newCCTXStatus types.CctxStatus) {
+	if depositErr != nil && isContractReverted {
+		tmpCtxRevert, commitRevert := ctx.CacheContext()
+		// contract call reverted; should refund via a revert tx
+		err := k.validateFailedOutbound(
+			tmpCtxRevert,
+			cctx,
+			types.CctxStatus_PendingOutbound,
+			depositErr.Error(),
+			cctx.InboundParams.Amount,
+		)
+		if err != nil {
+			cctx.SetAbort(err.Error())
+			return types.CctxStatus_Aborted
+		}
+
+		commitRevert()
+		return types.CctxStatus_PendingRevert
+	}
+	k.validateSuccessfulOutbound(ctx, cctx, "", false)
+	return types.CctxStatus_OutboundMined
+}
+
+// ValidateOutboundObservers processes the finalization of an outbound transaction based on the ballot status.
+// The state is committed only if the individual steps are successful.
+func (k Keeper) ValidateOutboundObservers(
 	ctx sdk.Context,
 	cctx *types.CrossChainTx,
 	ballotStatus observertypes.BallotStatus,
@@ -29,9 +75,9 @@ func (k Keeper) ProcessOutbound(
 	err := func() error {
 		switch ballotStatus {
 		case observertypes.BallotStatus_BallotFinalized_SuccessObservation:
-			k.ProcessSuccessfulOutbound(tmpCtx, cctx, valueReceived)
+			k.validateSuccessfulOutbound(tmpCtx, cctx, valueReceived, true)
 		case observertypes.BallotStatus_BallotFinalized_FailureObservation:
-			err := k.ProcessFailedOutbound(tmpCtx, cctx, valueReceived)
+			err := k.validateFailedOutboundObservers(tmpCtx, cctx, valueReceived)
 			if err != nil {
 				return err
 			}
@@ -49,35 +95,7 @@ func (k Keeper) ProcessOutbound(
 	return nil
 }
 
-// ProcessSuccessfulOutbound processes a successful outbound transaction. It does the following things in one function:
-//
-//  1. Change the status of the CCTX from
-//     - PendingRevert to Reverted
-//     - PendingOutbound to OutboundMined
-//
-//  2. Set the finalization status of the current outbound tx to executed
-//
-//  3. Emit an event for the successful outbound transaction
-//
-// This function sets CCTX status , in cases where the outbound tx is successful, but tx itself fails
-// This is done because SaveSuccessfulOutbound does not set the cctx status
-// For cases where the outbound tx is unsuccessful, the cctx status is automatically set to Aborted in the ProcessFailedOutbound function, so we can just return and error to trigger that
-func (k Keeper) ProcessSuccessfulOutbound(ctx sdk.Context, cctx *types.CrossChainTx, valueReceived string) {
-	oldStatus := cctx.CctxStatus.Status
-	switch oldStatus {
-	case types.CctxStatus_PendingRevert:
-		cctx.SetReverted("Outbound succeeded, revert executed")
-	case types.CctxStatus_PendingOutbound:
-		cctx.SetOutboundMined("Outbound succeeded, mined")
-	default:
-		return
-	}
-	cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
-	newStatus := cctx.CctxStatus.Status.String()
-	EmitOutboundSuccess(ctx, valueReceived, oldStatus.String(), newStatus, cctx.Index)
-}
-
-// ProcessFailedOutbound processes a failed outbound transaction. It does the following things in one function:
+// validateFailedOutboundObservers processes a failed outbound transaction for observers. It does the following things in one function:
 //
 // 1. For Admin Tx or a withdrawal from Zeta chain, it aborts the CCTX
 //
@@ -91,12 +109,12 @@ func (k Keeper) ProcessSuccessfulOutbound(ctx sdk.Context, cctx *types.CrossChai
 //
 // This function sets CCTX status , in cases where the outbound tx is successful, but tx itself fails
 // This is done because SaveSuccessfulOutbound does not set the cctx status
-// For cases where the outbound tx is unsuccessful, the cctx status is automatically set to Aborted in the ProcessFailedOutbound function, so we can just return and error to trigger that
-func (k Keeper) ProcessFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx, valueReceived string) error {
+// For cases where the outbound tx is unsuccessful, the cctx status is automatically set to Aborted in the validateFailedOutboundObservers function, so we can just return and error to trigger that
+func (k Keeper) validateFailedOutboundObservers(ctx sdk.Context, cctx *types.CrossChainTx, valueReceived string) error {
 	oldStatus := cctx.CctxStatus.Status
 	// The following logic is used to handler the mentioned conditions separately. The reason being
-	// All admin tx is created using a policy message , there is no associated inbound tx , therefore we do not need any revert logic
-	// For transactions which originated from ZEVM , we can process the outbound in the same block as there is no TSS signing required for the revert
+	// All admin tx is created using a policy message, there is no associated inbound tx, therefore we do not need any revert logic
+	// For transactions which originated from ZEVM, we can process the outbound in the same block as there is no TSS signing required for the revert
 	// For all other transactions we need to create a revert tx and set the status to pending revert
 
 	if cctx.InboundParams.CoinType == coin.CoinType_Cmd {
@@ -108,9 +126,9 @@ func (k Keeper) ProcessFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx,
 		// Try revert if the coin-type is ZETA
 		case coin.CoinType_Zeta:
 			{
-				err := k.processFailedOutboundForZEVM(ctx, cctx)
+				err := k.validateFailedOutboundObserversForZEVM(ctx, cctx)
 				if err != nil {
-					return cosmoserrors.Wrap(err, "ProcessFailedOutboundForZEVMTx")
+					return cosmoserrors.Wrap(err, "validateFailedOutboundObserversForZEVMTx")
 				}
 			}
 		// For all other coin-types, we do not revert, the cctx is aborted
@@ -121,24 +139,30 @@ func (k Keeper) ProcessFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx,
 			}
 		}
 	} else {
-		err := k.processFailedOutboundForExternalChainTx(ctx, cctx, oldStatus)
+		err := k.validateFailedOutbound(ctx, cctx, oldStatus, "Outbound failed, start revert", cctx.GetCurrentOutboundParam().Amount)
 		if err != nil {
-			return cosmoserrors.Wrap(err, "ProcessFailedOutboundForExternalChainTx")
+			return cosmoserrors.Wrap(err, "validateFailedOutbound")
 		}
 	}
 	newStatus := cctx.CctxStatus.Status.String()
 	EmitOutboundFailure(ctx, valueReceived, oldStatus.String(), newStatus, cctx.Index)
 	return nil
 }
 
-// processFailedOutboundForExternalChainTx processes the failed outbound transaction for external chain tx
-func (k Keeper) processFailedOutboundForExternalChainTx(
+// validateFailedOutbound processes the failed outbound transaction
+func (k Keeper) validateFailedOutbound(
 	ctx sdk.Context,
 	cctx *types.CrossChainTx,
 	oldStatus types.CctxStatus,
+	revertMsg string,
+	inputAmount math.Uint,
 ) error {
 	switch oldStatus {
 	case types.CctxStatus_PendingOutbound:
+		senderChain := k.zetaObserverKeeper.GetSupportedChainFromChainID(ctx, cctx.InboundParams.SenderChainId)
+		if senderChain == nil {
+			return observertypes.ErrSupportedChains
+		}
 
 		gasLimit, err := k.GetRevertGasLimit(ctx, *cctx)
 		if err != nil {
@@ -159,7 +183,7 @@ func (k Keeper) processFailedOutboundForExternalChainTx(
 			ctx,
 			cctx.InboundParams.SenderChainId,
 			cctx,
-			cctx.OutboundParams[0].Amount,
+			inputAmount,
 			false,
 		)
 		if err != nil {
@@ -170,19 +194,54 @@ func (k Keeper) processFailedOutboundForExternalChainTx(
 			return err
 		}
 		// Not setting the finalization status here, the required changes have been made while creating the revert tx
-		cctx.SetPendingRevert("Outbound failed, start revert")
+		cctx.SetPendingRevert(revertMsg)
 	case types.CctxStatus_PendingRevert:
 		cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
 		cctx.SetAbort("Outbound failed: revert failed; abort TX")
 	}
 	return nil
 }
 
-// processFailedOutboundForZEVM processes the failed outbound transaction for ZEVM
-func (k Keeper) processFailedOutboundForZEVM(ctx sdk.Context, cctx *types.CrossChainTx) error {
+// validateSuccessfulOutbound processes a successful outbound transaction. It does the following things in one function:
+//
+//  1. Change the status of the CCTX from
+//     - PendingRevert to Reverted
+//     - PendingOutbound to OutboundMined
+//
+//  2. Set the finalization status of the current outbound tx to executed
+//
+//  3. Emit an event for the successful outbound transaction if flag is provided
+//
+// This function sets CCTX status, in cases where the outbound tx is successful, but tx itself fails
+// This is done because SaveSuccessfulOutbound does not set the cctx status
+// For cases where the outbound tx is unsuccessful, the cctx status is automatically set to Aborted in the validateFailedOutboundObservers function, so we can just return and error to trigger that
+func (k Keeper) validateSuccessfulOutbound(
+	ctx sdk.Context,
+	cctx *types.CrossChainTx,
+	valueReceived string,
+	emitEvent bool,
+) {
+	oldStatus := cctx.CctxStatus.Status
+	switch oldStatus {
+	case types.CctxStatus_PendingRevert:
+		cctx.SetReverted("Outbound succeeded, revert executed")
+	case types.CctxStatus_PendingOutbound:
+		cctx.SetOutboundMined("Outbound succeeded, mined")
+	default:
+		return
+	}
+	cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
+	newStatus := cctx.CctxStatus.Status.String()
+	if emitEvent {
+		EmitOutboundSuccess(ctx, valueReceived, oldStatus.String(), newStatus, cctx.Index)
+	}
+}
+
+// validateFailedOutboundObserversForZEVM processes the failed outbound transaction for ZEVM
+func (k Keeper) validateFailedOutboundObserversForZEVM(ctx sdk.Context, cctx *types.CrossChainTx) error {
 	indexBytes, err := cctx.GetCCTXIndexBytes()
 	if err != nil {
-		// Return err to save the failed outbound ad set to aborted
+		// Return err to save the failed outbound and set to aborted
 		return fmt.Errorf("failed reverting GetCCTXIndexBytes: %s", err.Error())
 	}
 	// Finalize the older outbound tx