refactor: update cctx status to object to split error message into error and revert_error
#3326
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to error handling and status messaging across multiple components of the ZetaChain system. The primary focus is on introducing a structured approach to managing transaction statuses and error messages, specifically by implementing a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant CrossChainTx
participant Keeper
participant StatusHandler
Client->>CrossChainTx: Initiate Transaction
CrossChainTx->>Keeper: Process Transaction
alt Transaction Successful
Keeper->>StatusHandler: Update Status
StatusHandler-->>CrossChainTx: Set Status
else Transaction Failed
Keeper->>StatusHandler: Update Error Status
StatusHandler-->>CrossChainTx: Set Error Message
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
The modifications represent a significant improvement in error reporting granularity, providing more structured and informative feedback during cross-chain and fungible module transactions. The introduction of the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kingpinXD
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3326 +/- ##
===========================================
+ Coverage 61.91% 62.49% +0.58%
===========================================
Files 445 450 +5
Lines 31501 31771 +270
===========================================
+ Hits 19503 19855 +352
+ Misses 11127 11030 -97
- Partials 871 886 +15
|
| // 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 { |
There was a problem hiding this comment.
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.
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.
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
…factor-status-message # Conflicts: # docs/zetacore/status_message.md # x/crosschain/keeper/cctx_gateway_observers.go # x/crosschain/keeper/cctx_orchestrator_validate_outbound.go # x/crosschain/keeper/msg_server_vote_outbound_tx.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
x/crosschain/keeper/initiate_outbound_test.go (1)
Line range hint
79-79: Fix failing tests by updating expected error messages.The pipeline indicates test failures due to mismatched error messages. Update the test expectations to match the actual implementation:
- Line 79: Update to expect "error from EVMDeposit: deposit error"
- Line 430: Update to expect "unable to create outbound: chain not supported"
Apply these fixes to resolve the pipeline failures.
Also applies to: 430-430
🧹 Nitpick comments (14)
x/fungible/types/evm_error_message_test.go (1)
12-29: Enhance test coverage with table-driven tests and edge cases.The current test covers basic functionality but could be improved by:
- Using table-driven tests for better maintainability
- Adding edge cases (empty strings, special characters)
- Testing invalid inputs (nil address, nil args)
Consider refactoring to:
func TestEvmErrorMessage(t *testing.T) { - t.Run("TestEvmErrorMessage", func(t *testing.T) { - address := sample.EthAddress() - msg := types.EvmErrorMessage("errorMsg", "method", address, "args") - msg = types.EvmErrorMessageAddErrorString(msg, "error_cause") - msg = types.EvmErrorMessageAddRevertReason(msg, "revert_reason") - - require.Equal(t, fmt.Sprintf( - "message:%s,method:%s,contract:%s,args:%s,error:%s,revertReason:%s", - "errorMsg", - "method", - address.String(), - "args", - "error_cause", - "revert_reason", - ), msg) - }) + tests := []struct { + name string + errorMsg string + method string + address common.Address + args interface{} + errorStr string + revertReason interface{} + want string + }{ + { + name: "basic case", + errorMsg: "errorMsg", + method: "method", + address: sample.EthAddress(), + args: "args", + errorStr: "error_cause", + revertReason: "revert_reason", + }, + { + name: "empty strings", + errorMsg: "", + method: "", + address: sample.EthAddress(), + args: "", + errorStr: "", + revertReason: "", + }, + // Add more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg := types.EvmErrorMessage(tt.errorMsg, tt.method, tt.address, tt.args) + msg = types.EvmErrorMessageAddErrorString(msg, tt.errorStr) + msg = types.EvmErrorMessageAddRevertReason(msg, tt.revertReason) + + want := fmt.Sprintf( + "message:%s,method:%s,contract:%s,args:%v,error:%s,revertReason:%v", + tt.errorMsg, + tt.method, + tt.address.String(), + tt.args, + tt.errorStr, + tt.revertReason, + ) + require.Equal(t, want, msg) + }) + } }x/fungible/types/evm_error_message.go (1)
31-45: Consider using a structured error message type.The current string-based approach is error-prone and difficult to parse. A structured type would be more maintainable.
Consider creating a structured type:
type EvmErrorMessage struct { Message string Method string Contract common.Address Args interface{} Error string RevertReason interface{} } func (e *EvmErrorMessage) String() string { return fmt.Sprintf( "%s:%s,%s:%s,%s:%s,%s:%v,%s:%s,%s:%v", MessageKey, e.Message, MethodKey, e.Method, ContractKey, e.Contract.Hex(), ArgsKey, e.Args, ErrorKey, e.Error, RevertReasonKey, e.RevertReason, ) }x/crosschain/types/status.go (2)
17-28: Consider enhancing error context using error wrapping.While the status transition logic is sound, consider wrapping the error message in a custom error type for better error handling upstream.
func (m *Status) UpdateStatus(newStatus CctxStatus) { if m.ValidateTransition(newStatus) { m.Status = newStatus } else { - m.StatusMessage = fmt.Sprintf( - "Failed to transition status from %s to %s", - m.Status.String(), - newStatus.String(), - ) + errMsg := fmt.Sprintf("Failed to transition status from %s to %s", m.Status.String(), newStatus.String()) + m.StatusMessage = errMsg m.Status = CctxStatus_Aborted } }
31-40: Enhance method documentation for clarity.The implementation is clean, but consider expanding the documentation to detail each field's purpose and when they are updated.
-// UpdateErrorMessages updates cctx.status.error_message and cctx.status.error_message_revert. +// UpdateErrorMessages updates the status message fields: +// - StatusMessage: Contains the most recent status update +// - ErrorMessage: Stores outbound transaction errors +// - ErrorMessageRevert: Stores revert-specific error informationx/crosschain/keeper/cctx_gateway_observers.go (1)
78-81: LGTM! Enhanced error reporting structure.The use of StatusMessages provides better error context. Consider adding the chain ID to the error message for even better debugging context.
config.CCTX.SetAbort(types.StatusMessages{ StatusMessage: "outbound pre-processing failed", - ErrorMessageOutbound: fmt.Sprintf("unable to create outbound: %s", err.Error()), + ErrorMessageOutbound: fmt.Sprintf("unable to create outbound for chain %d: %s", outboundReceiverChainID, err.Error()), })x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (2)
48-64: Enhance error handling in ValidateOutboundZEVM.The error handling could be more descriptive and structured.
Consider enhancing the error messages:
cctx.SetAbort( types.StatusMessages{ - StatusMessage: fmt.Sprintf("revert failed"), + StatusMessage: fmt.Sprintf("outbound and revert operations failed"), ErrorMessageOutbound: depositErr.Error(), ErrorMessageRevert: reverErr.Error(), })
223-226: Improve error context in processFailedOutboundOnExternalChain.The error message could provide more context about the failure.
cctx.SetPendingRevert(types.StatusMessages{ - StatusMessage: "outbound failed", + StatusMessage: "outbound transaction failed, initiating revert", ErrorMessageOutbound: errorMsg, })x/fungible/keeper/evm.go (1)
Line range hint
719-729: Consider adding a gas buffer for reliability.The current gas estimation logic could be enhanced by adding a buffer to the estimated gas to handle unexpected variations in gas consumption. This would help prevent out-of-gas errors in edge cases.
if commit && gasLimit == nil { args, err := json.Marshal(evmtypes.TransactionArgs{ From: &from, To: contract, Data: (*hexutil.Bytes)(&data), }) if err != nil { return nil, cosmoserrors.Wrapf(sdkerrors.ErrJSONMarshal, "failed to marshal tx args: %s", err.Error()) } + // Add a 10% buffer to estimated gas + const gasBuffer = 1.1 gasRes, err := k.evmKeeper.EstimateGas(sdk.WrapSDKContext(ctx), &evmtypes.EthCallRequest{ Args: args, GasCap: config.DefaultGasCap, }) if err != nil { return nil, err } - gasCap = gasRes.Gas + gasCap = uint64(float64(gasRes.Gas) * gasBuffer) k.Logger(ctx).Info("call evm", "EstimateGas", gasCap) }docs/zetacore/status_message.md (3)
18-23: Fix typos and grammar in status descriptions.The status descriptions contain typos and grammatical errors:
- "intermediately" should be "intermediate"
- Remove duplicate "the" in "The the cctx failed"
- "finalzied" should be "finalized"
🧰 Tools
🪛 LanguageTool
[grammar] ~18-~18: You used an adverb (‘intermediately’) instead of an adjective, or a noun (‘status’) instead of another adjective.
Context: ...he inbound to be finalized , this is an intermediately status used by the protocol only - `PendingOut...(A_RB_NN)
[duplication] ~22-~22: Possible typo: you repeated a word.
Context: ...s a terminal status -PendingRevert: The the cctx failed at some step and is pending...(ENGLISH_WORD_REPEAT_RULE)
25-32: Improve documentation formatting and clarity.Several improvements needed:
- Remove "a" in "provides a some details"
- Add comma after "In this case"
- Consider using "whether" instead of "if" in boolean value description
🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: The article ‘a’ seems out of place here. Consider deleting it.
Context: ...atusMessage The status message provides a some details about the current status.T...(A_SOME)
[style] ~30-~30: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...d This is a boolean value which is true if the cctx has been refunded after being ...(IF_WHETHER)
60-67: Add language identifier to code block.Add "text" as the language identifier to the fenced code block for better rendering:
-``` +```text🧰 Tools
🪛 Markdownlint (0.37.0)
60-60: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
196-199: LGTM! Consider enhancing error context.The structured approach using
StatusMessagesimproves error handling clarity.Consider adding chain ID context to the status message for better debugging:
cctx.SetAbort(types.StatusMessages{ - StatusMessage: "outbound failed", + StatusMessage: fmt.Sprintf("outbound failed on chain %d", cctx.GetCurrentOutboundParam().ReceiverChainId), ErrorMessageOutbound: errMessage, })x/crosschain/types/cctx_test.go (2)
215-221: Consider adding negative test case.While the current test verifies successful status transition, consider adding a test case for invalid state transitions.
Add a test case to verify that
SetPendingRevertfails or behaves appropriately when called from an invalid initial state (e.g.,AbortedorReverted).
247-253: LGTM! Consider standardizing test structure.The test effectively verifies the reverted status transition and message handling.
For consistency with the
SetAborttests, consider structuring this test with sub-tests usingt.Run()to test different scenarios (e.g., different initial states).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
typescript/zetachain/zetacore/crosschain/cross_chain_tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/cross_chain_tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (25)
docs/openapi/openapi.swagger.yaml(1 hunks)docs/zetacore/status_message.md(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(1 hunks)proto/zetachain/zetacore/crosschain/cross_chain_tx.proto(1 hunks)x/crosschain/keeper/cctx_gateway_observers.go(1 hunks)x/crosschain/keeper/cctx_gateway_zevm.go(2 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(11 hunks)x/crosschain/keeper/evm_deposit.go(1 hunks)x/crosschain/keeper/initiate_outbound.go(1 hunks)x/crosschain/keeper/initiate_outbound_test.go(7 hunks)x/crosschain/keeper/msg_server_abort_stuck_cctx.go(2 hunks)x/crosschain/keeper/msg_server_vote_inbound_tx_test.go(1 hunks)x/crosschain/keeper/msg_server_vote_outbound_tx.go(1 hunks)x/crosschain/types/cctx.go(2 hunks)x/crosschain/types/cctx_test.go(1 hunks)x/crosschain/types/errors.go(1 hunks)x/crosschain/types/status.go(1 hunks)x/crosschain/types/status_test.go(2 hunks)x/fungible/keeper/evm.go(2 hunks)x/fungible/keeper/evm_test.go(1 hunks)x/fungible/keeper/zevm_message_passing_test.go(1 hunks)x/fungible/keeper/zevm_msg_passing.go(2 hunks)x/fungible/types/errors.go(1 hunks)x/fungible/types/evm_error_message.go(1 hunks)x/fungible/types/evm_error_message_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/crosschain/types/errors.go
- x/fungible/keeper/evm_test.go
🧰 Additional context used
📓 Path-based instructions (21)
x/fungible/keeper/zevm_message_passing_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/evm_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/types/evm_error_message_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/crosschain/keeper/cctx_gateway_zevm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/zevm_msg_passing.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/types/errors.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/initiate_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/status_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/evm.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/initiate_outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/cctx_gateway_observers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/types/evm_error_message.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (2)
x/crosschain/keeper/msg_server_vote_outbound_tx.go (1)
Learnt from: kingpinXD
PR: zeta-chain/node#2615
File: x/crosschain/keeper/msg_server_vote_outbound_tx_test.go:472-472
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `SaveFailedOutbound` function in `x/crosschain/keeper/msg_server_vote_outbound_tx.go` requires a string argument for the error message.
x/crosschain/types/status.go (1)
Learnt from: fbac
PR: zeta-chain/node#2952
File: x/crosschain/types/status.go:0-0
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `UpdateStatusMessage` method, it's acceptable to update `m.StatusMessage` before `m.Status`.
🪛 GitHub Actions: ci
x/crosschain/keeper/initiate_outbound_test.go
[error] 79-79: Test failure: Expected error message 'deposit error' but got 'error from EVMDeposit: deposit error'
[error] 430-430: Test failure: Expected error message 'chain not supported' but got 'unable to create outbound: chain not supported'
🪛 LanguageTool
docs/zetacore/status_message.md
[grammar] ~18-~18: You used an adverb (‘intermediately’) instead of an adjective, or a noun (‘status’) instead of another adjective.
Context: ...he inbound to be finalized , this is an intermediately status used by the protocol only - `PendingOut...
(A_RB_NN)
[duplication] ~22-~22: Possible typo: you repeated a word.
Context: ...s a terminal status - PendingRevert : The the cctx failed at some step and is pending...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~26-~26: The article ‘a’ seems out of place here. Consider deleting it.
Context: ...atusMessage The status message provides a some details about the current status.T...
(A_SOME)
[style] ~30-~30: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal).
Context: ...d This is a boolean value which is true if the cctx has been refunded after being ...
(IF_WHETHER)
[grammar] ~36-~36: It looks like you are using the wrong form of the noun or the verb. Did you mean “fail has” or “fails have”?
Context: ...le outbound - A cctx where the outbound fails has the transition PendingOutbound -> `Pe...
(NOUN_PLURAL_HAS)
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...tbound has failed. - outbound failed : The outbound failed, The protocol would...
(UNLIKELY_OPENING_PUNCTUATION)
[formatting] ~53-~53: Consider inserting a comma after an introductory phrase for better readability.
Context: ...is means the outbound is never created. In this case the cctx is aborted - `cctx aborted wit...
(IN_THAT_CASE_COMMA)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ...failed to external chain ,start revert` : withdraw failed to an external chain, a...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~70-~70: Loose punctuation mark.
Context: ... revert when source chain is Zetachain` : The coin type is not supported for reve...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~71-~71: Loose punctuation mark.
Context: ...error from EVMDeposit: [Error_String] : Error returned by the protocol when try...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
docs/zetacore/status_message.md
6-6: Column: 1
Hard tabs
(MD010, no-hard-tabs)
7-7: Column: 1
Hard tabs
(MD010, no-hard-tabs)
8-8: Column: 1
Hard tabs
(MD010, no-hard-tabs)
9-9: Column: 1
Hard tabs
(MD010, no-hard-tabs)
10-10: Column: 1
Hard tabs
(MD010, no-hard-tabs)
11-11: Column: 1
Hard tabs
(MD010, no-hard-tabs)
12-12: Column: 1
Hard tabs
(MD010, no-hard-tabs)
60-60: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (22)
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (2)
44-47: LGTM! Clean transition to structured status messages.The change to use
StatusMessagestype improves the structure and maintainability of status updates.
15-15: Verify consistency of message casing across the codebase.The change from "CCTX" to "cctx" in the message seems arbitrary. Ensure this aligns with the project's message formatting standards.
x/crosschain/keeper/initiate_outbound.go (2)
41-41: LGTM! Clear and descriptive status message.The status message "initiating outbound" clearly indicates the current state of the transaction.
Line range hint
9-16: Address TODO comment regarding gas payment flows.The TODO comment indicates a temporary solution for gas payment flows. This technical debt should be tracked and addressed.
✅ Verification successful
Technical debt is properly tracked and documented.
The TODO comment is linked to an open issue (#2345) that tracks the temporary solution for gas payment flows. The current implementation using
ShouldPayGasflag is actively used across multiple components, and the issue remains relevant for future improvement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's an existing issue for this TODO gh issue view 2345 || echo "Issue #2345 not found" # Search for related gas payment implementations rg "ShouldPayGas" --type go -A 5Length of output: 9834
Script:
#!/bin/bash # Search for related PRs gh pr list --search "2345 in:title,body" # Search for related TODOs about MsgVoteInbound or gas payment rg "TODO.*MsgVoteInbound|TODO.*gas payment" --type go # Search for MsgVoteInbound usage to understand the context ast-grep --pattern 'MsgVoteInbound'Length of output: 443
x/crosschain/types/status.go (1)
12-14: LGTM! Clean refactoring of status updates.The consolidation of status and error messages into a single
StatusMessagesstruct improves code organization and maintainability.x/fungible/keeper/zevm_msg_passing.go (1)
29-32: LGTM! Improved error context with proper wrapping.The enhanced error handling provides better context by including the destination address and amount, which will aid in debugging.
x/fungible/types/errors.go (1)
38-48: LGTM! Well-defined error types with clear messages.The new error types follow the established pattern and provide specific error contexts for ZETA deposit operations.
x/crosschain/keeper/evm_deposit.go (1)
51-51: LGTM! Enhanced error context.The error wrapping improvement provides better context for debugging CCTX index parsing failures.
x/crosschain/types/status_test.go (2)
142-157: LGTM! Test cases simplified.The test cases have been appropriately simplified to focus on status transitions rather than message content.
202-271: LGTM! Comprehensive test coverage for error messages.The new test cases thoroughly validate the error message update behavior:
- Empty error message handling
- Partial updates
- Multiple updates
- Message persistence
x/crosschain/types/cctx.go (2)
148-148: LGTM! Enhanced revert transaction parameters.The addition of CoinType ensures proper token type handling in revert transactions.
187-208: LGTM! Consistent status message handling.The status setter methods have been updated to use the new StatusMessages struct, providing a more structured approach to status and error handling.
x/fungible/keeper/evm.go (2)
370-370: Improved error handling with specific error types!The changes enhance error context by wrapping errors with specific error types (
types.ErrABIGetandtypes.ErrDepositZetaToFungibleAccount), making it easier to track and debug issues.Also applies to: 375-375
668-668: Enhanced EVM error reporting with structured messages!The changes introduce a more comprehensive error reporting system that:
- Provides detailed context including method, contract, and arguments
- Adds special handling for contract revert errors
- Uses structured error messages for consistency
This will significantly improve debugging and error tracking capabilities.
Also applies to: 675-686
x/crosschain/keeper/cctx_gateway_zevm.go (1)
33-36: LGTM! Enhanced error handling with structured messages.The change improves error reporting by using
StatusMessageswith clear separation between status and error messages.e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go (1)
55-58: LGTM! Improved test assertion readability.The multi-line format enhances readability while correctly validating the error message in the new structured format.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (2)
111-114: LGTM! Well-documented error message field.The addition of
error_message_revertwith clear documentation improves the protocol's error handling capabilities.
116-122: LGTM! Clean internal message structure.The
StatusMessagestype provides a clear structure for handling status and error messages internally.x/fungible/keeper/zevm_message_passing_test.go (1)
164-164: LGTM! More flexible error assertion.Using
ErrorContainsinstead ofErrorIsallows for more flexible error message validation while maintaining the test's intent.x/crosschain/types/cctx_test.go (3)
187-209: LGTM! Comprehensive test coverage.The test cases thoroughly verify abort status transitions and message handling for both revert and outbound scenarios.
227-231: LGTM! Clear and focused test case.The test appropriately verifies the basic status transition and message assignment.
237-241: LGTM! Appropriate test coverage.The test correctly verifies the transition to OutboundMined status.
kingpinXD
changed the title
kingpinXD
changed the title
kingpinXD
changed the title
error and revert_error
| // 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 { |
There was a problem hiding this comment.
If it's used internally we don't need a proto definition and it can be an internal type
| err.Error()) | ||
| config.CCTX.SetAbort(types.StatusMessages{ | ||
| StatusMessage: "outbound failed deposit to ZEVM failed but contract call did not revert", | ||
| ErrorMessageOutbound: fmt.Sprintf("error from EVMDeposit: %s", err.Error()), |
There was a problem hiding this comment.
| ErrorMessageOutbound: fmt.Sprintf("error from EVMDeposit: %s", err.Error()), | |
| ErrorMessageOutbound: fmt.Sprintf("error from HandleEVMDeposit: %s", err.Error()), |
Let's be accurate
There was a problem hiding this comment.
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
| "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.
| 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", |
There was a problem hiding this comment.
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
| @@ -139,7 +146,10 @@ func (k Keeper) processFailedOutboundObservers(ctx sdk.Context, cctx *types.Cros | |||
| 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", | |||
There was a problem hiding this comment.
| StatusMessage: "outbound failed for non-ZETA cctx", | |
| StatusMessage: "outbound couldn't be processed", |
There was a problem hiding this comment.
Similar to the comment above , I am planning to use outbound failed as the prefix for all outbound failed messages , so that the wording is consistent.
| @@ -193,7 +193,10 @@ SaveFailedOutbound saves a failed outbound transaction.It does the following thi | |||
| */ | |||
|
|
|||
| func (k Keeper) SaveFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx, errMessage string, tssPubkey string) { | |||
There was a problem hiding this comment.
We need to rename this, this is not the outbound that fails, where it would revert then but the observation of the outbound, meaning an issue at the protocol level
| @@ -193,7 +193,10 @@ SaveFailedOutbound saves a failed outbound transaction.It does the following thi | |||
| */ | |||
|
|
|||
| func (k Keeper) SaveFailedOutbound(ctx sdk.Context, cctx *types.CrossChainTx, errMessage string, tssPubkey string) { | |||
| cctx.SetAbort("", errMessage) | |||
| cctx.SetAbort(types.StatusMessages{ | |||
| StatusMessage: "outbound failed", | |||
There was a problem hiding this comment.
| StatusMessage: "outbound failed", | |
| StatusMessage: "outbound can't be observed", |
| // UpdateErrorMessages updates cctx.status.error_message and cctx.status.error_message_revert. | ||
| func (m *Status) UpdateErrorMessages(messages StatusMessages) { | ||
| // Always update the status message, status should contain only the most recent update | ||
| m.StatusMessage = messages.StatusMessage |
There was a problem hiding this comment.
should do
if messages.ErrorMessageOutbound != "" {
m.ErrorMessage = messages.ErrorMessageOutbound
}
as well
There was a problem hiding this comment.
do you mean
if m.StatusMessage != "" {}
I decided not to add the check to allow empty status messages if we ever need it ( However, we do not have empty messages now, as every update has some update for status )
| ContractKey = "contract" | ||
| ArgsKey = "args" | ||
| ErrorKey = "error" | ||
| RevertReasonKey = "revertReason" |
There was a problem hiding this comment.
| RevertReasonKey = "revertReason" | |
| RevertReasonKey = "revert_reason" |
There was a problem hiding this comment.
I used camel case here to be consistent with other constants defined in the core logic.
Is the reason you want to do more to snake case that this is going to be used by external users ? If so , I think moving to snake case makes sense , but lmk if there is a different reason for the change
Description
The PR refactors the
Statusobject to add a new fielderror_message_revert. This field contains error messages for the revert tx and the old fielderrorwould contain the error from outbound.It adds a new struct,
StatusMessages,which is used internally to pass values between functions.Introduces some error types to for errors which were previously not wrapped , this is done to make the error messages more informative.
Closes : #3194
How Has This Been Tested?
Summary by CodeRabbit
Based on the comprehensive review of the changes, here are the release notes:
New Features
Improvements
Bug Fixes
Documentation
The changes primarily focus on improving error handling, transaction status tracking, and providing more informative error messages across the system.