Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: update cctx status to object to split error message into error and revert_error #3326

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/openapi/openapi.swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58504,6 +58504,11 @@ definitions:
type: string
format: int64
description: when the CCTX was created. only populated on new transactions.
error_message_revert:
type: string
title: |-
error_message_revert carries information about the revert outbound tx ,
which is created if the first outbound tx fails
zetacoreemissionsParams:
type: object
properties:
Expand Down
71 changes: 71 additions & 0 deletions docs/zetacore/status_message.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# CCTX status message

The cctx object contains a field `cctx_status` , which has the following structure
```go
type Status struct {
Status CctxStatus `protobuf:"varint,1,opt,name=status,proto3,enum=zetachain.zetacore.crosschain.CctxStatus" json:"status,omitempty"`
StatusMessage string `protobuf:"bytes,2,opt,name=status_message,json=statusMessage,proto3" json:"status_message,omitempty"`
ErrorMessage string `protobuf:"bytes,6,opt,name=error_message,json=errorMessage,proto3" json:"error_message,omitempty"`
LastUpdateTimestamp int64 `protobuf:"varint,3,opt,name=lastUpdate_timestamp,json=lastUpdateTimestamp,proto3" json:"lastUpdate_timestamp,omitempty"`
IsAbortRefunded bool `protobuf:"varint,4,opt,name=isAbortRefunded,proto3" json:"isAbortRefunded,omitempty"`
CreatedTimestamp int64 `protobuf:"varint,5,opt,name=created_timestamp,json=createdTimestamp,proto3" json:"created_timestamp,omitempty"`
ErrorMessageRevert string `protobuf:"bytes,7,opt,name=error_message_revert,json=errorMessageRevert,proto3" json:"error_message_revert,omitempty"`
}
```

## Status
This is the most updated status for the cctx . This can be one of the following values
- `PendingInbound` : The cctx is pending for the inbound to be finalized , this is an intermediately status used by the protocol only
- `PendingOutbound` : This means that the inbound has been finalzied, and the outbound is pending
- `OutboundMined` : The outbound has been successfully mined. This is a terminal status
- `Aborted` : The cctx has been aborted. This is a terminal status
- `PendingRevert` : The the cctx failed at some step and is pending for the revert to be finalized
- `Reverted` : The cctx has been successfully reverted. This is a terminal status

### StatusMessage
The status message provides a some details about the current status.This is primiary meant for the user to quickly understand the status of the cctx.
### LastUpdateTimestamp
The last time the status was updated
### IsAbortRefunded
This is a boolean value which is true if the cctx has been refunded after being aborted or not .
### CreatedTimestamp
The time when the cctx was created
### ErrorMessage and ErrorMessageRevert
A cctx can have a maximum of two outbound params.
- A normal flow for a cctx is to go from `PendingOutbound` -> `OutboundMined` , which creates a single outbound
- A cctx where the outbound fails has the transition `PendingOutbound` -> `PendingRevert` -> `Reverted` , which creates two outbounds
- Any of the above two flows can abort the cctx at some point that can create either one or two outbounds

- The `ErrorMessage` field only contains a value if the original outbound failed. It contains details about the error that caused the outbound to fail
- The `ErrorMessageRevert` field only contains a value if the revert outbound failed. It contains details about the error that caused the revert outbound to fail.

### Example values for StatusMessage field and how to interpret them
- `initiating outbound` : The inbound votes have been successfully finalized, and the protocol is starting the outbound process
- `outbound mined successfully` : The outbound was successfully mined
- `revert successful` : The outbound failed , but the revert was successful
- `revert failed` : The revert failed. This message also means that the initial outbound has failed.

- `outbound failed` : The outbound failed, The protocol would try to create a revert either in the same block or schedule one to be picked up by zetaclient
- `outbound failed for non-ZETA cctx` : Special case of outbound failed where we do not try to revert the cctx
- `outbound failed for admin tx` : The outbound failed for an admin transaction, in this case we do not revert the cctx
- `outbound failed deposit to ZEVM failed but contract call did not revert` : Special case of outbound failed The outbound/deposit failed, but the contract did not revert,
this is most likely caused by an internal error in the protocol.The CCTX is this case is aborted. Users can try connecting with the zetachain team to get a refund

- `cctx aborted with admin cmd` : The cctx was aborted manually by an admin command


### Example values for ErrorMessage and ErrorMessageRevert fields and how to interpret them

- For a failed deposit, the ErrorMessage would contain the following fields. The protocol generates the fields tagged as internal.
```
- description[Internal]: This is message created by the protocol to provide a high level description of what caused the error
- method: The method that was called by the protocol,
- contract: The contract that his method was called on,
- args:The argumets that were used for this call,
- errorMessage[Internal]: Error message from the ZEVM call,
- revertReason: Revert reason from the smart contract,
```

- `outbound failed to external chain ,start revert` : withdraw failed to an external chain, and the protocol is starting the revert process back to ZEVM.
- `coin type [CoinType] not supported for revert when source chain is Zetachain` : The coin type is not supported for revert when the source chain is Zetachain.
- `error from EVMDeposit: [Error_String]` : Error returned by the protocol when trying to deposit tokens( and optionally call a contract) on ZEVM. The error string should explain the cause
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,8 @@ func TestBitcoinDepositAndCallRevertWithDust(r *runner.E2ERunner, args []string)
cctx := utils.WaitCctxAbortedByInboundHash(r.Ctx, r, txHash.String(), r.CctxClient)

require.True(r, cctx.GetCurrentOutboundParam().Amount.Uint64() < constant.BTCWithdrawalDustAmount)
require.True(r, strings.Contains(cctx.CctxStatus.ErrorMessage, crosschaintypes.ErrInvalidWithdrawalAmount.Error()))
require.True(
r,
strings.Contains(cctx.CctxStatus.ErrorMessageRevert, crosschaintypes.ErrInvalidWithdrawalAmount.Error()),
)
}
12 changes: 12 additions & 0 deletions proto/zetachain/zetacore/crosschain/cross_chain_tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,18 @@ message Status {
bool isAbortRefunded = 4;
// when the CCTX was created. only populated on new transactions.
int64 created_timestamp = 5;
// error_message_revert carries information about the revert outbound tx ,
// which is created if the first outbound tx fails
string error_message_revert = 7;
}

// StatusMessages is a message that carries status and error messages for a
// cctx Currently this is only used internally to pass messages when updating
// the status
message StatusMessages {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct is only used internally, and not saved to the state.
In my opinion, the code looks cleaner using this object rather than three strings as arguments. But it can be changed to that if needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's used internally we don't need a proto definition and it can be an internal type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defined this at the proto level as we we can refactor the Status object in the future to use this directly instead of defining three fields .
I wanted to get reviews on this pr before creating an issue.

I can refactor to defining a type, though, if its still needed

string status_message = 1;
string error_message_outbound = 2;
string error_message_revert = 3;
}

// ProtocolContractVersion represents the version of the protocol contract used
Expand Down
46 changes: 46 additions & 0 deletions typescript/zetachain/zetacore/crosschain/cross_chain_tx_pb.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,14 @@ export declare class Status extends Message<Status> {
*/
createdTimestamp: bigint;

/**
* error_message_revert carries information about the revert outbound tx ,
* which is created if the first outbound tx fails
*
* @generated from field: string error_message_revert = 7;
*/
errorMessageRevert: string;

constructor(data?: PartialMessage<Status>);

static readonly runtime: typeof proto3;
Expand All @@ -410,6 +418,44 @@ export declare class Status extends Message<Status> {
static equals(a: Status | PlainMessage<Status> | undefined, b: Status | PlainMessage<Status> | undefined): boolean;
}

/**
* StatusMessages is a message that carries status and error messages for a
* cctx Currently this is only used internally to pass messages when updating
* the status
*
* @generated from message zetachain.zetacore.crosschain.StatusMessages
*/
export declare class StatusMessages extends Message<StatusMessages> {
/**
* @generated from field: string status_message = 1;
*/
statusMessage: string;

/**
* @generated from field: string error_message_outbound = 2;
*/
errorMessageOutbound: string;

/**
* @generated from field: string error_message_revert = 3;
*/
errorMessageRevert: string;

constructor(data?: PartialMessage<StatusMessages>);

static readonly runtime: typeof proto3;
static readonly typeName = "zetachain.zetacore.crosschain.StatusMessages";
static readonly fields: FieldList;

static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): StatusMessages;

static fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>): StatusMessages;

static fromJsonString(jsonString: string, options?: Partial<JsonReadOptions>): StatusMessages;

static equals(a: StatusMessages | PlainMessage<StatusMessages> | undefined, b: StatusMessages | PlainMessage<StatusMessages> | undefined): boolean;
}

/**
* RevertOptions represents the options for reverting a cctx
*
Expand Down
5 changes: 4 additions & 1 deletion x/crosschain/keeper/cctx_gateway_observers.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ func (c CCTXGatewayObservers) InitiateOutbound(
}()
if err != nil {
// do not commit anything here as the CCTX should be aborted
config.CCTX.SetAbort("internal error", err.Error())
config.CCTX.SetAbort(types.StatusMessages{
StatusMessage: "Outbound pre-processing failed",
ErrorMessageOutbound: fmt.Sprintf("unable to create outbound: %s", err.Error()),
})
return types.CctxStatus_Aborted, err
}
commit()
Expand Down
9 changes: 6 additions & 3 deletions x/crosschain/keeper/cctx_gateway_zevm.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/zeta-chain/node/x/crosschain/types"
Expand Down Expand Up @@ -28,9 +30,10 @@ func (c CCTXGatewayZEVM) InitiateOutbound(

if err != nil && !isContractReverted {
// exceptional case; internal error; should abort CCTX
config.CCTX.SetAbort(
"error during deposit that is not smart contract revert",
err.Error())
config.CCTX.SetAbort(types.StatusMessages{
StatusMessage: "outbound failed deposit to ZEVM failed but contract call did not revert",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StatusMessage: "outbound failed deposit to ZEVM failed but contract call did not revert",
StatusMessage: "universal contract call didn't revert but outbound failed to be processed",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to use universal contract ,
But I would rather structure it so that outbound failed comes first , to make sure its a special case of outbound failed as mentioned int he document
outbound failed but the universal contract did not revert

ErrorMessageOutbound: fmt.Sprintf("error from EVMDeposit: %s", err.Error()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ErrorMessageOutbound: fmt.Sprintf("error from EVMDeposit: %s", err.Error()),
ErrorMessageOutbound: fmt.Sprintf("error from HandleEVMDeposit: %s", err.Error()),

Let's be accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not specifically use HandleEVMDeposit here as this error message might be used by people not familiar with the protocol , and EvmDeposit might be easier to understand

})
return types.CctxStatus_Aborted, err
}

Expand Down
64 changes: 48 additions & 16 deletions x/crosschain/keeper/cctx_orchestrator_validate_outbound.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,27 @@
if depositErr != nil && isContractReverted {
tmpCtxRevert, commitRevert := ctx.CacheContext()
// contract call reverted; should refund via a revert tx
err := k.processFailedOutboundOnExternalChain(
reverErr := k.processFailedOutboundOnExternalChain(
kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
tmpCtxRevert,
cctx,
types.CctxStatus_PendingOutbound,
depositErr.Error(),
cctx.InboundParams.Amount,
)
if err != nil {
// In this case the outbound and revert both fail in the same block ,so we should update status for both together.
// A status revert failed indicates that the outbound has already failed before this.

if reverErr != nil {
// Error here would mean the outbound tx failed and we also failed to create a revert tx.
// This is the only case where we set outbound and revert messages, as both the outbound and the revert failed in the same block
cctx.SetAbort(
"revert failed",
fmt.Sprintf("deposit error: %s, processing error: %s", depositErr.Error(), err.Error()))
types.StatusMessages{
StatusMessage: fmt.Sprintf("revert failed"),
ErrorMessageOutbound: depositErr.Error(),
ErrorMessageRevert: reverErr.Error(),
})
return types.CctxStatus_Aborted
}

commitRevert()
return types.CctxStatus_PendingRevert
}
Expand Down Expand Up @@ -124,7 +131,9 @@
if cctx.InboundParams.CoinType == coin.CoinType_Cmd {
// if the cctx is of coin type cmd or the sender chain is zeta chain, then we do not revert, the cctx is aborted
cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
cctx.SetAbort("", "outbound failed for admin tx")
cctx.SetAbort(types.StatusMessages{
StatusMessage: "outbound failed for admin tx",
})
} else if chains.IsZetaChain(cctx.InboundParams.SenderChainId, k.GetAuthorityKeeper().GetAdditionalChainList(ctx)) {
switch cctx.InboundParams.CoinType {
// Try revert if the coin-type is ZETA
Expand All @@ -139,7 +148,10 @@
default:
{
cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
cctx.SetAbort("", "outbound failed for non-ZETA cctx")
cctx.SetAbort(types.StatusMessages{
StatusMessage: "Outbound failed for non-ZETA cctx",
ErrorMessageOutbound: fmt.Sprintf("coin type %s not supported for revert when source chain is Zetachain", cctx.InboundParams.CoinType),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the failure of the revert, not the outbound

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The revert is never created for this particular flow , I think adding a ErrMessageRevert would be confusing since this flow has only 1 outbound params

kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
} else {
Expand Down Expand Up @@ -210,10 +222,16 @@
return err
}
// Not setting the finalization status here, the required changes have been made while creating the revert tx
cctx.SetPendingRevert("", revertMsg)
cctx.SetPendingRevert(types.StatusMessages{
StatusMessage: "outbound failed",
kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
ErrorMessageOutbound: revertMsg,
})
case types.CctxStatus_PendingRevert:
cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
cctx.SetAbort("aborted while processing failed outbound", "outbound and revert failed")
cctx.SetAbort(types.StatusMessages{
StatusMessage: "Revert failed",
ErrorMessageRevert: revertMsg,
})
}
return nil
}
Expand All @@ -240,9 +258,9 @@
oldStatus := cctx.CctxStatus.Status
switch oldStatus {
case types.CctxStatus_PendingRevert:
cctx.SetReverted("", "")
cctx.SetReverted(types.StatusMessages{StatusMessage: "revert successful"})
kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
case types.CctxStatus_PendingOutbound:
cctx.SetOutboundMined("")
cctx.SetOutboundMined(types.StatusMessages{StatusMessage: "outbound mined successfully"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cctx.SetOutboundMined(types.StatusMessages{StatusMessage: "outbound mined successfully"})
cctx.SetOutboundMined(types.StatusMessages{StatusMessage: "outbound successfully mined"})

default:
return
}
Expand Down Expand Up @@ -271,7 +289,10 @@
}

// Trying to revert the transaction this would get set to a finalized status in the same block as this does not need a TSS singing
cctx.SetPendingRevert("", "outbound failed")
cctx.SetPendingRevert(types.StatusMessages{
StatusMessage: "outbound failed",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StatusMessage: "outbound failed",
StatusMessage: "outbound failed to be processed on connected chain",

ErrorMessageOutbound: "outbound failed to external chain ,start revert",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a status message, error message should contains why it failed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, however, the why is not available to us right now.
I thought of adding Observers voted this outbound as failed but I think that information would be confusing .

We can in the future , add a field for observers to add a message as to why they are voting this tx as failed . I can create an issue for that if needed

kingpinXD marked this conversation as resolved.
Show resolved Hide resolved
})
data, err := base64.StdEncoding.DecodeString(cctx.RelayedMessage)
if err != nil {
return fmt.Errorf("failed decoding relayed message: %s", err.Error())
Expand Down Expand Up @@ -305,7 +326,10 @@
return fmt.Errorf("failed ZETARevertAndCallContract: %s", err.Error())
}

cctx.SetReverted("", "outbound failed")
cctx.SetReverted(types.StatusMessages{
StatusMessage: "reverted successfull",

Check failure on line 330 in x/crosschain/keeper/cctx_orchestrator_validate_outbound.go

View workflow job for this annotation

GitHub Actions / lint

`successfull` is a misspelling of `successful` (misspell)
})

if len(ctx.TxBytes()) > 0 {
// add event for tendermint transaction hash format
hash := tmbytes.HexBytes(tmtypes.Tx(ctx.TxBytes()).Hash())
Expand Down Expand Up @@ -350,7 +374,10 @@
}

// update status
cctx.SetPendingRevert("", "outbound failed")
cctx.SetPendingRevert(types.StatusMessages{
StatusMessage: "outbound failed",
ErrorMessageOutbound: "outbound failed to external chain ,start revert",
})
kingpinXD marked this conversation as resolved.
Show resolved Hide resolved

// process the revert on ZEVM
if err := k.fungibleKeeper.ProcessV2RevertDeposit(
Expand All @@ -368,7 +395,9 @@
}

// tx is reverted
cctx.SetReverted("", "outbound failed")
cctx.SetReverted(types.StatusMessages{
StatusMessage: "revert successful",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StatusMessage: "revert successful",
StatusMessage: "revert successfully processed",

})

// add event for tendermint transaction hash format
if len(ctx.TxBytes()) > 0 {
Expand All @@ -381,7 +410,10 @@
cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
case types.CctxStatus_PendingRevert:
cctx.GetCurrentOutboundParam().TxFinalizationStatus = types.TxFinalizationStatus_Executed
cctx.SetAbort("aborted while processing failed outbound", "outbound and revert failed")
cctx.SetAbort(types.StatusMessages{
StatusMessage: "revert failed",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StatusMessage: "revert failed",
StatusMessage: "revert failed to be processed",

ErrorMessageRevert: "outbound and revert failed",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the status message, ErrorMessageRevert should give reason why revert failed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above , resolving this to continue discussion there

})
}
return nil
}
2 changes: 1 addition & 1 deletion x/crosschain/keeper/evm_deposit.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (k Keeper) HandleEVMDeposit(ctx sdk.Context, cctx *types.CrossChainTx) (boo
// - Return false will abort the cctx
indexBytes, err := cctx.GetCCTXIndexBytes()
if err != nil {
return false, err
return false, errors.Wrap(types.ErrUnableToParseCCTXIndexBytes, err.Error())
}
data, err := base64.StdEncoding.DecodeString(cctx.RelayedMessage)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion x/crosschain/keeper/initiate_outbound.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ func (k Keeper) InitiateOutbound(ctx sdk.Context, config InitiateOutboundConfig)
)
}

config.CCTX.SetPendingOutbound("")
config.CCTX.SetPendingOutbound(types.StatusMessages{StatusMessage: "initiating outbound"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config.CCTX.SetPendingOutbound(types.StatusMessages{StatusMessage: "initiating outbound"})
config.CCTX.SetPendingOutbound(types.StatusMessages{StatusMessage: "processing outbound"})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose to use initiate , as that's the function called right after this line

return cctxGateway.InitiateOutbound(ctx, config)
}
Loading
Loading