Skip to content

fix: give data-less batch reverts a readable error and pre-check the batch (WA-3267) - #8559

Open
Clóvis Neto (clovisdasilvaneto) wants to merge 1 commit into
devfrom
clovis/wa-3267-opaque-batch-revert
Open

fix: give data-less batch reverts a readable error and pre-check the batch (WA-3267)#8559
Clóvis Neto (clovisdasilvaneto) wants to merge 1 commit into
devfrom
clovis/wa-3267-opaque-batch-revert

Conversation

@clovisdasilvaneto

@clovisdasilvaneto Clóvis Neto (clovisdasilvaneto) commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

What it solves

Resolves: WA-3267

A user bulk-executed 2 queued transactions through MultiSendCallOnly 1.4.1. One inner execTransaction reverted. MultiSendCallOnly does revert(0, 0) — it destroys the child's revert reason — so the client got a data-less CALL_EXCEPTION with nothing to decode.

What they saw:

  • Toast: Execution failed. Require(false). followed by a ~4KB raw ethers dump — full calldata, contract addresses, version=6.17.0 — rendered verbatim in a <pre>.
  • Inline: Error submitting the transaction. Please try again.

Two separate defects behind that:

  1. Data-less reverts were never classified. useTxNotifications.ts built `${baseMessage} ${formatError(error)}`, and formatError just capitalizes error.reason → Require(false).. The override chain below it matched only REVERTED, guard errors, nonce-too-low and rate-limit, so a revert with no decodable payload fell straight through to the raw string. This breaks WA-3005 guideline Add SafeInfoService #1 (never show a technical string) and Refactor safe info service #6 (Details is a support reference, never a payload).

  2. The bulk flow never ran the GS026 pre-checks. runExecutionPreChecks (feat(errors): block execution with GS026 pre-checks before broadcast #8499) was wired only into the single-transaction path at components/tx/shared/hooks.ts:27. ExecuteBatch/ReviewBatch.tsx never called it, so the two most likely causes of this revert — the Safe nonce advanced since the queue loaded, or a collected signature no longer verifies — were caught before broadcast in a normal execution and completely invisible in bulk. The user paid gas for a failure that was detectable client-side.

The inline half was already fixed on dev by WA-3005 (d74c47cc8, #8484) → Could not submit the transaction. Note that commit is not in v1.99.0: release branched at 3c678cb48 on Aug 11, that commit landed Aug 13.

Out of scope: MultiSendCallOnly's revert(0, 0) (the reason is destroyed on-chain, no client fix exists), off-chain copy (WA-3006), the rest of WA-3005.

How this PR fixes it

Classify the failure. isOpaqueRevertError in transaction-errors.ts matches a revert with no GS code, no custom-error selector, and positive proof of an empty payload — data === '0x', ethers' "no data present", or viem's "for an unknown reason". Deliberately narrow: a GS revert, an Error(string) revert, a guard custom error, an RPC timeout and a rate-limit all stay on their existing paths.

Say something honest. OPAQUE_REVERT_MESSAGE lives in the shared code-keyed source that web and mobile both read:

This transaction fails on-chain without reporting a reason. If it bundles several transactions, execute them one at a time to find the one that fails.

Splitting the batch is the genuinely useful advice — a single execTransaction bubbles its GS code where MultiSendCallOnly discards it. The copy claims nothing about gas, because TxEvent.FAILED fires both pre-broadcast and from txMonitor; mined reverts go to TxEvent.REVERTED, which already says gas was spent. Both the toast and the inline alert render it, so one failure no longer tells two stories.

Sanitise Details once. A single shared getErrorReference drives both surfaces so they cannot drift: a non-revert keeps its message (usually copy we wrote — relay timeouts, "transaction not found"); a GS revert reduces to Error code GS0xx; a revert whose selector is exactly Error(string) yields the already-decoded reason; anything else yields nothing and the affordance is hidden rather than offered empty.

This is deliberately not a blanket suppression of every revert. ERC20: transfer amount exceeds balance is a sentence the chain wrote for a human to read — hiding it would satisfy the acceptance criteria while making the product worse. The gate is the structural 0x08c379a0 selector, not a regex over the message.

Prevent what we can. runBatchExecutionPreChecks checks that the batch's nonces still line up with the chain and that every collected signature still recovers to its signer, naming the offender by its position in the batch — the same number the review screen shows. Wired into ReviewBatch ahead of both the wallet and the relay path. This also covers the symptom reported in WA-49: a stale safe.nonce from the CGW cache produces a batch whose nonces no longer match the chain.

Three deliberate calls inside it:

  • Signatures are verified against the CGW-provided safeTxHash rather than a recomputed one. Cheaper and SDK-free, at the cost of missing a hash mismatch — a miss, never a false block, which is the right side to err on when on-chain validation is the authority.
  • Blobs longer than 65 bytes are skipped. CGW returns a contract signature's dynamic part, whose trailing byte is data rather than a v value; reading a signature type off it would recover a random address and falsely flag a nested-Safe signer.
  • sameAddress replaced a case-sensitive !== in validateTxSignatures. Incidental bugfix: ethers.recoverAddress returns checksummed, so a lowercase signer previously produced a false BAD_SIGNATURE block in the single-tx flow and in ComboSubmit too.

How to test it

Unit: yarn workspace @safe-global/web test src/utils/__tests__/transaction-errors.test.ts src/services/tx/__tests__/executionPreChecks.test.ts src/hooks/__tests__/useTxNotifications.test.ts src/components/tx-flow/flows/ExecuteBatch src/components/tx/ErrorMessage src/components/tx/TxSubmitError

Manually, the opaque revert: queue two transactions where the second is guaranteed to revert (e.g. an ERC-20 transfer exceeding the Safe's balance), fully sign both, and use Bulk execute. Estimation fails; the toast and the inline alert should both show the message above, with no Require(false), no hex, no version=, and no Details affordance.

Manually, the pre-check: build a batch, then execute one of its transactions from another tab (or have a co-signer do it) so the Safe nonce advances. Submit the batch. It should block before the wallet opens, naming the offending position — "Transaction 1 in this batch no longer matches the queue. Refresh to get the current one." — instead of burning gas on an opaque revert.

Regression: confirm a GS revert still shows its mapped copy and the code-only reference, an Error(string) revert still shows its reason behind Details, and an RPC failure still says "could not check" rather than predicting failure.

Affected flows

  • Bulk execute from the transaction queue — now blocked pre-broadcast on a stale nonce or an unverifiable signature, on both the wallet and relay paths
  • Any transaction failure toast — a data-less revert gets new copy; every revert loses its raw Details payload
  • Inline submit errors (TxSubmitError) — a data-less revert now matches the toast instead of saying "Could not submit the transaction."
  • Inline error Details (ErrorMessage, 41 call sites) — behaviour changes only for errors that pass isRevertError

Blast radius

  • packages/utils/.../contractErrors.ts — the shared code-keyed copy source, read by web and mobile. Additive only: OPAQUE_REVERT_MESSAGE, Gs026BatchReason, getGs026BatchMessage. No existing export changed; the mobile parity test passes untouched. New strings are covered by the existing content-rule test (no URLs, versions, "please", exclamation marks, hex blobs).
  • transaction-errors.ts — consumed by TxSubmitError, TxCheckError, ErrorMessage, useTxNotifications. isRevertError is unchanged; the new exports are additive.
  • ErrorMessage — 41 call sites. Non-transaction consumers (ImportDialog, spaces dialogs, proposers, NftCollections, PaginatedTxns) pass HTTP/validation errors, which are not reverts and keep their message unchanged. ExecuteThroughRoleForm's Zodiac Roles custom errors still take the GS013 + decoded-name path.
  • executionPreChecks.ts — validateTxSignatures is also consumed by useValidateTxData → ComboSubmit. Refactored to share one verification helper; all 16 pre-existing tests pass unchanged, and both consumers become strictly more permissive (the sameAddress fix and the over-long-blob skip only remove false blocks).
  • useTxNotifications — single consumer (_app.tsx:116), but it handles every transaction lifecycle event. formatError itself was left untouched, so the recovery, counterfactual and safe-message notification hooks are unaffected.
  • No RTK Query contract, feature flag, chain config, route, or persisted state is touched. getNonces is called once per batch submit, with forceRefetch.

Risks / not checked

  • No on-chain reproduction. I could not re-run the user's failed batch; the ethers error shape is reproduced from the reported dump and from ethers' own getBuiltinCallException, not from a live revert.
  • Not tested on mobile. packages/utils is shared and type-checks, and the parity test passes, but no mobile surface renders the new copy yet.
  • The batch nonce check is strict (nonce !== currentNonce + index) rather than tolerant. Justified by the useBatchedTxs invariant and the single entry point via BatchExecuteButton. If CGW's /nonces ever lags the cached safe.nonce, the user gets a hard block whose remedy ("Refresh") will not help.
  • Owner/threshold changes inside a batch invalidate the signature set for every subsequent inner execTransaction and land as the same opaque revert. Not detectable by these pre-checks; the fallback copy reaching both surfaces is the mitigation, not a fix.
  • SpeedUpModal still passes a raw error.message to detailedMessage — same defect class, deliberately out of scope.
  • Two follow-ups found and left alone, both recorded on the ticket:
    1. isRevertError classifies viem contract reverts backwards. viem/errors/contract.js:199 builds "…reverted with the following reason:" for the case that has a reason, which matches /reverted with/; :202 builds "The contract function "X" reverted." for the reasonless case, which matches nothing. So a viem revert with a reason is treated as "will most likely fail" and one without as "could not check" — backwards, since the reasonless case is the one we are most certain reverted. Fixing it flips a class of errors between two states WA-3005 deliberately separated, so it needs its own ticket rather than a side-effect change here.
    2. Panic(uint256) (0x4e487b71) is a standard selector, so it is neither decoded as a custom error nor matched as Error(string). An arithmetic overflow in a target contract surfaces with nothing behind Details. The panic codes are a finite documented set — the natural next candidate.

Visual summary

Before — the reason is destroyed on-chain and the raw dump reaches the user:

flowchart TB
  A["Bulk execute<br/>2 queued txs"] --> B["MultiSendCallOnly.multiSend"]
  B --> C["execTransaction #1"]
  B --> D["execTransaction #2 reverts<br/>(stale nonce / bad signature)"]
  D --> E["revert(0, 0)<br/>reason destroyed"]
  E --> F["ethers CALL_EXCEPTION<br/>data = 0x"]
  F --> G["formatError(error.reason)"]
  G --> H["Toast: 'Execution failed. Require(false).'<br/>+ 4KB hex dump in Details"]
  F --> I["Inline: 'Error submitting the transaction.'"]
Loading

After — detectable causes are stopped before broadcast, and what does get through is named:

flowchart TB
  A["Bulk execute<br/>2 queued txs"] --> P{"runBatchExecutionPreChecks"}
  P -->|"nonce moved"| Q["Blocked, no gas spent<br/>'Transaction 2 in this batch<br/>no longer matches the queue.'"]
  P -->|"signature invalid"| R["Blocked, no gas spent<br/>'Could not verify a signature<br/>on transaction 2 in this batch.'"]
  P -->|"all clear"| B["MultiSendCallOnly.multiSend"]
  B --> D["Inner tx reverts anyway<br/>revert(0, 0)"]
  D --> F["isOpaqueRevertError"]
  F --> G["OPAQUE_REVERT_MESSAGE<br/>(shared source)"]
  G --> H["Toast"]
  G --> I["Inline alert"]
  F --> J["getErrorReference → undefined<br/>no Details, no payload"]
Loading

How every revert class now resolves behind Details:

flowchart LR
  E["Error"] --> R{"isRevertError?"}
  R -->|no| M["keep our own message"]
  R -->|yes| G{"GS code?"}
  G -->|yes| C["'Error code GS0xx'"]
  G -->|no| S{"Error(string)<br/>selector?"}
  S -->|yes| D["decoded reason<br/>e.g. 'ERC20: transfer amount<br/>exceeds balance'"]
  S -->|no| N["nothing —<br/>affordance hidden"]
Loading

Checklist

  • I've tested the branch on mobile 📱 — not tested; packages/utils type-checks and the parity test passes, but no mobile surface renders the new copy yet
  • I've documented how it affects the analytics (if at all) 📊 — no analytics change. Pre-check blocks are still tracked as Errors._804, matching the single-tx flow; worth revisiting if WA-3005's error-rate KR should exclude prevented failures
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻 — yarn verify:changed:web exits 0: 643 suites, 5487 tests. Plus @safe-global/utils (110) and the mobile parity test
  • I've listed affected flows and blast radius, and named what I did not verify 🎯

CLA signature

With the submission of this Pull Request, I confirm that I have read and agree to the terms of the Contributor License Agreement.

🤖 Generated with Claude Code

@clovisdasilvaneto @claude
fix: give data-less batch reverts a readable error and pre-check the …
f6620b2 
…batch (WA-3267)

MultiSendCallOnly does `revert(0, 0)` when an inner execTransaction fails, so
a broken bulk execution reaches the client as a bare `require(false)` with no
payload to decode. The toast rendered that verbatim — "Execution failed.
Require(false)." — with a ~4KB ethers dump behind Details, and nothing in the
flow tried to prevent the revert in the first place.

Classify the case (`isOpaqueRevertError`) and give it honest copy in the shared
code-keyed source: it says something in the transaction fails on-chain and that
splitting the batch is what surfaces the real reason. Both the toast and the
inline alert render it, so one failure tells one story.

Sanitise Details through a single shared `getErrorReference`. A revert never
yields its raw dump; what survives is what we can name — the GS code, or a
decoded Error(string) reason, which the chain wrote for a human to read.
Anything else gets no Details rather than an empty one.

Run the GS026 pre-checks in the bulk flow too. They were wired only into the
single-transaction path, so a stale Safe nonce or a signature that no longer
verifies was invisible in bulk and cost the user gas. The batch variant checks
each queued transaction and names the offending one by its position.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Branch preview

✅ Deploy successful!

Website:
https://clovis-wa-3267-opaque-batch-revert--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://clovis-wa-3267-opaque-batch-revert--walletweb.review.5afe.dev/storybook/

@github-actions

github-actions Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

tx-builder Preview

✅ Deploy successful!

Preview URL:
https://clovis-wa-3267-opaque-batch-revert--tx-builder.review.5afe.dev/

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📦 Next.js Bundle Analysis for @safe-global/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1.53 MB (🟡 +312.45 KB)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

New Page Added

The following page was added to the bundle from the code in this PR:

Page Size (compressed) First Load
/spaces/activity 546 B 1.53 MB

Sixty-one Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/ 424 B (🟢 -290 B) 1.53 MB
/403 635 B (🟢 -86 B) 1.53 MB
/404 837 B (🟡 +19 B) 1.53 MB
/_offline 1.05 KB (🟢 -133 B) 1.53 MB
/addOwner 551 B (🟢 -1 B) 1.53 MB
/address-book 23.07 KB (🟢 -6.84 KB) 1.55 MB
/apps 44.98 KB (🔴 +9.28 KB) 1.57 MB
/apps/custom 42.42 KB (🔴 +10.01 KB) 1.57 MB
/apps/open 9.27 KB (🟢 -104.05 KB) 1.54 MB
/balances 25.5 KB (🟢 -109.6 KB) 1.55 MB
/balances/nfts 16.29 KB (🟢 -3.56 KB) 1.54 MB
/balances/positions 5.96 KB (🟢 -2.02 KB) 1.53 MB
/bridge 553 B (🟢 -66 B) 1.53 MB
/cookie 7.33 KB (🟢 -48 B) 1.53 MB
/dashboard/new 1.42 KB (🟢 -2.09 KB) 1.53 MB
/earn 575 B (🟢 -55 B) 1.53 MB
/home 195.68 KB (🟢 -72.31 KB) 1.72 MB
/hypernative/oauth-callback 2.07 KB (🟡 +58 B) 1.53 MB
/imprint 1.42 KB (🟢 -9 B) 1.53 MB
/licenses 1.96 KB (🟢 -94 B) 1.53 MB
/new-safe/advanced-create 30.99 KB (🟢 -202.55 KB) 1.56 MB
/new-safe/create 29.46 KB (🟢 -203.28 KB) 1.56 MB
/new-safe/load 9.32 KB (🟢 -4.02 KB) 1.54 MB
/privacy 13.21 KB (🟢 -507 B) 1.54 MB
/settings 770 B (🟢 -5.72 KB) 1.53 MB
/settings/appearance 6.17 KB (🟢 -2.25 KB) 1.53 MB
/settings/cookies 6 KB (🟢 -2.04 KB) 1.53 MB
/settings/data 32.36 KB (🟢 -2.66 KB) 1.56 MB
/settings/environment-variables 7.26 KB (🟢 -2.35 KB) 1.53 MB
/settings/modules 6.92 KB (🟢 -4.81 KB) 1.53 MB
/settings/notifications 11.42 KB (🟢 -2.5 KB) 1.54 MB
/settings/safe-apps 8.88 KB (🟢 -11.09 KB) 1.54 MB
/settings/security 6.49 KB (🟢 -2.2 KB) 1.53 MB
/settings/setup 37.76 KB (🟢 -19.4 KB) 1.56 MB
/share/safe-app 6.63 KB (🟢 -1.66 KB) 1.53 MB
/spaces 611 B (🟡 +2 B) 1.53 MB
/spaces/address-book 509 B (🟡 +2 B) 1.53 MB
/spaces/create-space 440 B (🟡 +3 B) 1.53 MB
/spaces/members 499 B (🟡 +3 B) 1.53 MB
/spaces/safe-accounts 507 B (🟡 +1 B) 1.53 MB
/spaces/security 6.75 KB (🟢 -121.95 KB) 1.53 MB
/spaces/settings/about 508 B (🟡 +4 B) 1.53 MB
/spaces/settings/account 509 B (🟡 +3 B) 1.53 MB
/spaces/settings/general 509 B (🟡 +3 B) 1.53 MB
/stake 1.22 KB (🟡 +124 B) 1.53 MB
/swap 744 B (🟢 -417 B) 1.53 MB
/terms 13.71 KB (🟢 -19 B) 1.54 MB
/transactions 35.4 KB (🟢 -128.78 KB) 1.56 MB
/transactions/history 35.36 KB (🟢 -128.78 KB) 1.56 MB
/transactions/messages 15.27 KB (🟢 -104.36 KB) 1.54 MB
/transactions/msg 8.08 KB (🟢 -105.13 KB) 1.54 MB
/transactions/queue 6.22 KB (🟢 -104 KB) 1.53 MB
/transactions/tx 1.01 KB (🟢 -105.67 KB) 1.53 MB
/wc 586 B (🟡 +1 B) 1.53 MB
/welcome 435 B (🟢 -13.87 KB) 1.53 MB
/welcome/accounts 610 B (🟢 -112 B) 1.53 MB
/welcome/create-space 444 B (🟡 +2 B) 1.53 MB
/welcome/invite-members 445 B (🟡 +4 B) 1.53 MB
/welcome/select-safes 448 B (🟡 +8 B) 1.53 MB
/welcome/spaces 308 B (🟢 -197 B) 1.53 MB
/welcome/survey 697 B (🟡 +1 B) 1.53 MB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Coverage report for apps/web

St.❔
 Category Percentage Covered / Total
🟢 Statements
84.93% (+0.06% 🔼)
 35292/41556
🟡 Branches
69.73% (-0.02% 🔻)
 11546/16557
🟡 Functions
73.19% (+0.09% 🔼)
 5368/7334
🟢 Lines
86.15% (+0.04% 🔼)
 31585/36662
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🟢
... / ReviewBatch.tsx
 92.79% 70.18% 88.89% 94.39%
🟢
... / ConfirmationTitle.tsx
 100% 50% 100% 100%
🟢
... / useTxNotifications.ts
 81.82% 53.23% 90% 83.13%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / transaction-errors.ts
98.73% (+0.52% 🔼)
94.29% (-1.55% 🔻)
 100% 100%
🟢
... / executionPreChecks.ts
97.53% (-2.47% 🔻)
93.55% (-6.45% 🔻)
 100%
98.55% (-1.45% 🔻)

Test suite run success

7624 tests passing in 876 suites.

Report generated by 🧪jest coverage report action from f6620b2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@clovisdasilvaneto