-
Notifications
You must be signed in to change notification settings - Fork 111
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
base: develop
Are you sure you want to change the base?
Changes from 5 commits
dd6aa99
7380d27
66170ae
542b448
9e20402
889a815
1948197
5af3f2c
3a26b7e
2ed02a9
acf9793
a72b3ef
2fb8d5d
2045d3d
afe7be1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,8 @@ | ||||||
package keeper | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
|
||||||
sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
|
||||||
"github.com/zeta-chain/node/x/crosschain/types" | ||||||
|
@@ -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", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to use universal contract , |
||||||
ErrorMessageOutbound: fmt.Sprintf("error from EVMDeposit: %s", err.Error()), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's be accurate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not specifically use |
||||||
}) | ||||||
return types.CctxStatus_Aborted, err | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
} | ||||||
|
@@ -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 | ||||||
|
@@ -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), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the failure of the revert, not the outbound There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||
|
@@ -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 | ||||||
} | ||||||
|
@@ -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"}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
default: | ||||||
return | ||||||
} | ||||||
|
@@ -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", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ErrorMessageOutbound: "outbound failed to external chain ,start revert", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a status message, error message should contains why it failed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, however, the 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()) | ||||||
|
@@ -305,7 +326,10 @@ | |||||
return fmt.Errorf("failed ZETARevertAndCallContract: %s", err.Error()) | ||||||
} | ||||||
|
||||||
cctx.SetReverted("", "outbound failed") | ||||||
cctx.SetReverted(types.StatusMessages{ | ||||||
StatusMessage: "reverted successfull", | ||||||
}) | ||||||
|
||||||
if len(ctx.TxBytes()) > 0 { | ||||||
// add event for tendermint transaction hash format | ||||||
hash := tmbytes.HexBytes(tmtypes.Tx(ctx.TxBytes()).Hash()) | ||||||
|
@@ -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( | ||||||
|
@@ -368,7 +395,9 @@ | |||||
} | ||||||
|
||||||
// tx is reverted | ||||||
cctx.SetReverted("", "outbound failed") | ||||||
cctx.SetReverted(types.StatusMessages{ | ||||||
StatusMessage: "revert successful", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
}) | ||||||
|
||||||
// add event for tendermint transaction hash format | ||||||
if len(ctx.TxBytes()) > 0 { | ||||||
|
@@ -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", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ErrorMessageRevert: "outbound and revert failed", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the status message, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above , resolving this to continue discussion there |
||||||
}) | ||||||
} | ||||||
return nil | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -38,6 +38,6 @@ func (k Keeper) InitiateOutbound(ctx sdk.Context, config InitiateOutboundConfig) | |||||
) | ||||||
} | ||||||
|
||||||
config.CCTX.SetPendingOutbound("") | ||||||
config.CCTX.SetPendingOutbound(types.StatusMessages{StatusMessage: "initiating outbound"}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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