Repository navigation
fix(swap): prevent spurious nonce() revert modal before Approve/Swap on Celo Reserve (#635) - #647
ocakciomursel-pixel wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="258-259" />
<code_context>
+ { to: params[0], data: '0xaffed0e0' },
+ params[1] || 'latest',
+ ])
+ if (!nonceResult || nonceResult === '0x') {
+ return '0x'
+ }
+ } catch {
</code_context>
<issue_to_address>
**Non-Safe wallets follow the Safe path**
When a non-Safe contract wallet returns nonempty data for the `nonce()` probe, the handler preserves its bytecode, so `@usedapp/core` classifies the wallet as a contract wallet and follows its Safe-specific transaction path; the transaction fails.
Identify Safe compatibility more specifically or route known non-Safe wallet types explicitly.
</issue_to_address>
### Comment 2
<location path="src/pages/gd/Swap/SwapCore/mentoReserve.tsx" line_range="127" />
<code_context>
useEffect(() => {
- if (swap?.state?.status === 'Exception') {
+ if (swap?.state?.status === 'Mining' || swap?.state?.status === 'Success') {
+ setError(undefined)
+ } else if (
+ swap?.state?.status === 'Exception' &&
</code_context>
<issue_to_address>
**Swap failure details disappear**
When an approval has succeeded and a later swap fails while the approval state remains `Success`, the swap sets the shared error, then the retained approval success clears it. The user sees no swap failure details or modal.
Clear errors only for the operation that owns them, or reset the stale approval state before processing the swap failure.
Also at `src/pages/gd/Swap/SwapCore/mentoReserve.tsx:115-116`.
</issue_to_address>
### Comment 3
<location path="src/pages/gd/Swap/SwapConfirmModal/index.tsx" line_range="119" />
<code_context>
setStatus('SENT')
onSent(swap.state.transaction?.hash || '', swap.state.transaction?.from || '')
} else if (swap?.state?.status === 'Exception') {
+ if (isBenignWalletNonceProbeError(swap.state.errorMessage)) {
+ return
+ }
</code_context>
<issue_to_address>
**Modal closes before wallet confirmation**
When `swap.send()` rejects with the benign nonce-probe error while the wallet prompt is still open, the `handleSwap` catch calls `onClose()` without filtering the error, so the user loses the confirmation modal before the wallet transaction settles.
Apply the benign nonce-probe error check in the `handleSwap` catch and keep the modal open for that error.
Also at `src/pages/gd/Swap/SwapConfirmModal/index.tsx:120`.
</issue_to_address>
### Comment 4
<location path="src/pages/gd/Swap/SwapCore/mentoReserve.tsx" line_range="127-131" />
<code_context>
useEffect(() => {
- if (swap?.state?.status === 'Exception') {
+ if (swap?.state?.status === 'Mining' || swap?.state?.status === 'Success') {
+ setError(undefined)
+ } else if (
+ swap?.state?.status === 'Exception' &&
+ !isBenignWalletNonceProbeError(swap.state.errorMessage)
+ ) {
setError({
message: i18n._(t`Swap transaction failed, please try again.`),
</code_context>
<issue_to_address>
**Stale approval error reappears**
When an earlier approval has a non-benign Exception state and a later swap enters Mining without resetting that approval state, the effect clears the error when the swap enters Mining, then processes the approval state; a retained non-benign approval Exception sets the error again. The user sees an approval-failure modal while the swap is already mining.
Clear or reset stale approval errors before a swap, or ensure an error from an inactive transaction cannot overwrite the current transaction's state.
</issue_to_address>
### Comment 5
<location path="src/utils/transactionErrors.ts" line_range="26-28" />
<code_context>
+ const raw =
+ typeof error === 'string'
+ ? error
+ : (error as { shortMessage?: string; reason?: string; message?: string } | null)?.shortMessage ||
+ (error as { shortMessage?: string; reason?: string; message?: string } | null)?.reason ||
+ (error as { shortMessage?: string; reason?: string; message?: string } | null)?.message ||
+ ''
+
</code_context>
<issue_to_address>
**Nonce probe error escapes filtering**
When the caught ethers error object has a non-empty `reason` that omits `method="nonce()"`, while its `message` contains the nonce probe revert, when an ethers error object has a generic `reason` and the `nonce()` details in `message`, `raw` selects only `reason`; the detector then misses the nonce probe and `handleApprove` displays the failure modal. The same object is also not filtered by the catch path in the swap UI.
Inspect all available error text fields when detecting the nonce probe instead of choosing only the first non-empty field.
</issue_to_address>Sourcery assessment
Approval pending. 5 findings to address first.
Blocking findings: src/hooks/useWeb3.tsx:259, src/pages/gd/Swap/SwapCore/mentoReserve.tsx:127, src/pages/gd/Swap/SwapConfirmModal/index.tsx:119, src/pages/gd/Swap/SwapCore/mentoReserve.tsx:131, src/utils/transactionErrors.ts:28
69464e2 to
a51fa23
Compare
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="271" />
<code_context>
+ // the account actually implements the Gnosis Safe interface (`VERSION()`,
+ // `getThreshold()`, and `nonce()`).
+ if (code.toLowerCase().startsWith('0xef0100')) {
+ return '0x'
+ }
+ try {
</code_context>
<issue_to_address>
**Safe wallets use the wrong transaction path**
When a connected Gnosis Safe’s interface probe fails or returns an incomplete response, such as during an RPC error or rate limit, the failed-probe branch or `catch` returns `'0x'`, so `@usedapp/core` classifies the Safe as a non-contract wallet and bypasses Safe-specific transaction handling. Safe transactions then fail or are routed incorrectly.
Preserve the fetched bytecode when the probes fail or are incomplete, and return `'0x'` only when successful probes establish that the wallet is not a Safe.
Also at `src/hooks/useWeb3.tsx:274-275`.
</issue_to_address>
### Comment 2
<location path="src/pages/gd/Swap/SwapCore/mentoReserve.tsx" line_range="169-174" />
<code_context>
// )
} catch (e) {
- onClose()
+ if (!isBenignWalletNonceProbeError(e)) {
+ onClose()
+ }
}
}
</code_context>
<issue_to_address>
**Actions can be submitted twice**
When a nonce probe rejects `approve.send()` or `swap.send()` before the wallet prompt settles, the handlers suppress the probe error, and `handleApprove` clears `approving` in `finally` while the wallet request can remain open. The Approve or Swap controls become available before the first request resolves, allowing duplicate submissions.
Keep the relevant action controls disabled until the wallet request settles, even when the nonce-probe error is suppressed.
Also at `src/pages/gd/Swap/SwapConfirmModal/index.tsx:169-171`.
</issue_to_address>
### Comment 3
<location path="src/pages/gd/Swap/SwapCore/mentoReserve.tsx" line_range="320" />
<code_context>
const onSwap = useCallback(() => {
+ setError(undefined)
+ swap.resetState()
sendData({
event: 'swap',
</code_context>
<issue_to_address>
**Overlapping swaps share transaction state**
When the user dismisses the modal while a swap is still pending and starts another swap before the first transaction finishes, `swap.resetState()` resets the shared swap hook while the earlier transaction is still pending, and the late status update from that transaction is observed by the newly opened modal. `SwapConfirmModal` then reports the earlier transaction as the new swap, including recording its hash and clearing the current swap inputs.
Prevent a second swap from starting while the first is pending, or track each submission separately so late state updates cannot be attributed to a newer swap.
</issue_to_address>
### Comment 4
<location path="src/pages/gd/Swap/SwapConfirmModal/index.tsx" line_range="120" />
<code_context>
onSent(swap.state.transaction?.hash || '', swap.state.transaction?.from || '')
} else if (swap?.state?.status === 'Exception') {
+ if (isBenignWalletNonceProbeError(swap.state.errorMessage)) {
+ return
+ }
onClose()
</code_context>
<issue_to_address>
**Swap modal stays waiting after success**
When a benign nonce-probe exception still reaches the modal and leaves the transaction hook in `Exception` while the wallet request remains pending, `SwapConfirmModal` ignores the benign exception without changing its local `CONFIRM` status. If the transaction hook remains in terminal `Exception` while the wallet request is open, confirming that request does not trigger the modal's `Mining`/`Success` handlers, so the swap can mine on-chain while the modal remains on “Waiting for Confirmation” and the app never records it as submitted.
Continue tracking the pending wallet transaction after suppressing the probe error, and transition the modal when it is submitted or mined.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and the change alters how connected contract wallets are identified before approve and swap transactions, and suppresses a matching nonce-probe error. If a wallet is misclassified and a transaction is accepted through the wrong handling path, its on-chain effects cannot be undone by reverting this code.
Blocking findings: src/hooks/useWeb3.tsx:271, src/pages/gd/Swap/SwapCore/mentoReserve.tsx:174, src/pages/gd/Swap/SwapCore/mentoReserve.tsx:320, src/pages/gd/Swap/SwapConfirmModal/index.tsx:120
a51fa23 to
01780c9
Compare
Sourcery withdrew this approval because the latest commits introduced blocking findings.
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If a real Safe is misclassified because one of its probes reverts, the app may skip Safe-specific handling and route a later approve or swap through the wrong transaction path; a confirmed on-chain transaction cannot be undone. Reverting restores the previous detection behavior, but cannot reverse a transaction already submitted.
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="299-302" />
<code_context>
+ return '0x'
+ }
+ if (
+ versionRes.status === 'ok' &&
+ thresholdRes.status === 'ok' &&
+ nonceRes.status === 'ok'
+ ) {
+ const isValidUint256 = (hex: string) => /^0x[0-9a-fA-F]{64}$/.test(hex)
+ const isGnosisSafe =
</code_context>
<issue_to_address>
**RPC errors misclassify wallets as Safe**
When a non-Safe wallet has bytecode and an interface probe fails for a non-revert reason, including a timeout or unrecognized revert, `probeSelector` returns `error`, but the classification branches handle only `reverted` probes or three `ok` probes, so execution returns the original bytecode. `@usedapp/core` then selects its Safe `nonce()` path and can fail before wallet confirmation.
Handle `error` probe results so a non-Safe wallet is not passed through as Safe.
Also at `src/hooks/useWeb3.tsx:314`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if a wallet is misclassified, the app may take a non-Safe transaction path instead of the Safe-specific one, potentially sending an unintended on-chain transaction; reverting would not undo a transaction already executed. The probes reduce that risk, but the change directly alters wallet detection used by approve and swap flows.
Blocking findings: src/hooks/useWeb3.tsx:302
01780c9 to
f74d22a
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="294-295" />
<code_context>
+ try {
+ // If the provider responds to a lightweight liveness check, the eth_call
+ // failure was an unrecognized contract revert rather than a transport failure.
+ await webprovider.jsonRpcFetchFunc('eth_chainId', [])
+ return { status: 'reverted' }
+ } catch {
+ return { status: 'error', error: err }
</code_context>
<issue_to_address>
**Safe wallets use the wrong lifecycle**
When a Safe selector probe fails for a non-revert RPC reason while `eth_chainId` still succeeds, `probeSelector` marks the failure as a revert, so the any-reverted check returns `'0x'` for the Safe and `@usedapp/core` uses the non-Safe transaction lifecycle. The Safe transaction then fails or follows the wrong flow.
Distinguish contract reverts from transport and method-specific RPC failures instead of treating `eth_chainId` success as proof of a revert.
</issue_to_address>
### Comment 2
<location path="src/hooks/useWeb3.tsx" line_range="277-304" />
<code_context>
+ try {
</code_context>
<issue_to_address>
**Wallet detection can hang indefinitely**
When the connected account has non-empty non-EIP-7702 bytecode and the provider never resolves one of the probe calls, `probeSelector` awaits raw RPC calls without a timeout, and `Promise.all` waits for all three probes before `eth_getCode` returns. A provider that leaves one `eth_call` pending leaves wallet detection and the transaction lifecycle waiting, even if the user has already opened the wallet prompt.
Bound the probe calls with a timeout and handle timed-out detection without blocking the transaction lifecycle.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and returning 0x for a contract that fails these probes can make the wallet library use its non-Safe transaction path for an account that actually needs Safe handling. If that changes an approval or swap that gets submitted, the on-chain transaction cannot be undone by reverting this patch.
Blocking findings: src/hooks/useWeb3.tsx:295, src/hooks/useWeb3.tsx:304
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="295" />
<code_context>
+ return { status: 'ok', data: res }
+ } catch (err) {
+ if (isDeterministicRevert(err)) {
+ return { status: 'reverted' }
+ }
+ try {
</code_context>
<issue_to_address>
**RPC errors misclassify Safe probes**
When `eth_call` fails for a genuine Safe for a non-revert RPC reason while `eth_chainId` succeeds, `probeSelector` marks the failure as a revert, so the later probe check makes `eth_getCode` return `'0x'` for the Safe. `@usedapp/core` then selects the non-Safe transaction lifecycle, and Safe transactions fail or follow the wrong flow.
Distinguish contract reverts from other RPC failures instead of treating a successful `eth_chainId` call as proof of a revert.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if a real Safe fails one of these probes and is reported as having no code, the app may treat its address as an ordinary EOA and submit an approval or swap through the wrong account path; any resulting transaction cannot be undone by reverting this change. Reverting restores the previous Safe detection behavior, but would not reverse transactions already sent.
Blocking findings: src/hooks/useWeb3.tsx:295
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. For matching delegated accounts, this makes the wallet library treat the account as having no code, bypassing its contract-account nonce path and allowing signing to proceed. If that classification is wrong, a transaction could be broadcast and execute on-chain; reverting the change would not undo it.
Fixes #635
Root Cause
When executing
ApproveorSwapon the Celo Reserve with a MetaMask account that has EIP-7702 Smart Account delegation enabled (0xef0100+ 20-byte target address),@usedapp/core(usePromiseTransaction) checksisNonContractWallet(library, account)(await library.getCode(account) === "0x") concurrently while waiting for the wallet signature.Because
eth_getCodereturns the 23-byte EIP-7702 delegation designator (0xef0100...) instead of"0x",@usedapp/coremisclassifies the EOA as a Gnosis Safe contract and immediately invokesgnosisSafeContract.nonce()(0xaffed0e0) on the user's address. Thateth_callreverts withCALL_EXCEPTION(method="nonce()", data="0x"), settingstate.status = "Exception"and displaying an error in the UI before the user confirms the transaction in MetaMask.Solution
src/hooks/useWeb3.tsx, inspect theeth_getCoderesult for the connectedaddressand return"0x"when the bytecode matches an EIP-7702 delegation designator (/^0xef0100[0-9a-fA-F]{40}$/).0xef, so genuine contract wallets and Gnosis Safes are completely unaffected and require no extraeth_callnetwork probes.Summary by Sourcery
Bug Fixes: