feat: add better error decoding on set-root/execute steps, and skip z…#848
feat: add better error decoding on set-root/execute steps, and skip z…#848
Conversation
🦋 Changeset detectedLatest commit: 2bf49da The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There was a problem hiding this comment.
Pull request overview
Improves revert/error decoding for MCMS set-root / execute flows (including nested CallReverted(bytes) payloads) and adds logic to skip fork execution on zkSync-like chains where standard Anvil forking isn’t supported.
Changes:
- Add recursive formatting/decoding for ABI-unpacked error args (so nested revert payloads inside
bytesare decoded when possible). - Wrap
setRoot/executefailures with additional revert decoding attempts and skip fork execution for zkSync chains. - Add unit tests covering nested error decoding and the new helper behavior; add a patch changeset.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/cld/commands/mcms/cmd_execute_fork.go | Skips fork execution for zkSync, and adds addDecodedRevertReason to enrich failures with decoded revert data. |
| engine/cld/commands/mcms/execute_fork_test.go | Adds tests for addDecodedRevertReason nil/degenerate cases. |
| deployment/helpers.go | Enhances parseErrorFromABI output by recursively decoding []byte args that contain nested revert payloads. |
| deployment/helpers_test.go | Adds coverage for CallReverted(bytes) wrapping Error(string) and nested custom errors. |
| .changeset/happy-cows-know.md | Declares a patch release for improved decoding + zkSync skipping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // isZkSyncChain checks if a chain selector corresponds to a zkSync chain. | ||
| // zkSync chains use zkEVM/EraVM which requires anvil-zksync, not standard Anvil. | ||
| // Returns (true, nil) for zkSync chains, (false, nil) for non-zkSync, or (false, err) on lookup failure. | ||
| func isZkSyncChain(selector uint64) (bool, error) { | ||
| chainIDStr, err := chainsel.GetChainIDFromSelector(selector) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // zkSync Era mainnet = 324, zkSync Sepolia testnet = 300 | ||
| // See: https://docs.zksync.io/zksync-network/environment | ||
| return chainIDStr == "324" || chainIDStr == "300", nil | ||
| } |
There was a problem hiding this comment.
isZkSyncChain only treats chain IDs 324 and 300 as zkSync. Elsewhere in the codebase, several additional selectors are treated as zkSync VM chains (e.g. Lens + Cronos zkEVM) and will still attempt standard Anvil forking and likely fail, so this won't fully eliminate the false negatives this PR targets. Consider reusing the same selector allowlist as chainLoaderEVM.isZkSyncVM (engine/cld/chains/chains.go:570-582) or, if available here, derive the flag from the loaded chain (e.g. cldf_evm.Chain.IsZkSyncVM).
| } | ||
|
|
||
| if reason, err := abi.UnpackRevert(inner); err == nil { | ||
| return fmt.Sprintf("Error(\"%s\")", reason) |
There was a problem hiding this comment.
tryDecodeByteArg wraps any successful abi.UnpackRevert decode as Error("..."). abi.UnpackRevert can also decode Panic(uint256) revert payloads, so this will mislabel panics as Error(...) and produce misleading output. Suggest checking the 4-byte selector (0x08c379a0 vs 0x4e487b71) and formatting accordingly, or returning the decoded reason string without forcing the Error("...") wrapper.
| return fmt.Sprintf("Error(\"%s\")", reason) | |
| return reason |
| return "", errors.New("error not found in ABI") | ||
| } | ||
|
|
||
| // formatUnpackedArgs formats an ABI-unpacked value, recursively decoding any |
There was a problem hiding this comment.
| // formatUnpackedArgs formats an ABI-unpacked value, recursively decoding any | |
| // formatUnpackedArgs formats an ABI-unpacked value, recursively decode any |
ErrorDecoderobject so it uses the same logic as the timelock execution.