fix(compat): drop Buffer and TextDecoder for browser/React Native support - #199
Merged
Conversation
…port generateOnChainIdentifier used Buffer.from, and ABI string decoding used TextDecoder. Both are undefined in browsers without a polyfill and in React Native/Hermes. Replace Buffer with the existing pure-JS toUtf8Bytes, add a pure-JS fromUtf8Bytes decoder, and move the WebAuthn adapter and Calibur executor decode off TextEncoder/TextDecoder. Verified in headless Chrome, real Hermes, a real Android device, and a real browser passkey flow.
📝 WalkthroughWalkthroughExports pure‑JS UTF‑8 helpers (toUtf8Bytes, fromUtf8Bytes) in ethereUtils, switches ABI string decoding to fromUtf8Bytes, adds strict tests, updates Safe account/adapters to use these helpers, and updates Calibur7702Account to preserve ABI bytes by hexlifying them. ChangesPure-JS UTF-8 utilities and account integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37be9612f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/account/Calibur/Calibur7702Account.ts (1)
1-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve nested
bytescalldata as hex, not UTF-8 text.
call[2]is the inner ABIbytespayload. If this branch ever receives aUint8Array,fromUtf8Bytes(call[2])will reinterpret binary calldata as text and destroy selectors/arguments.SafeAccount.decodeAccountCallDataalready avoids the same corruption by hex-encoding raw bytes first. Usehexlify(call[2])here instead.Suggested fix
import { decodeAbiParameters, encodeAbiParameters, - fromUtf8Bytes, + hexlify, keccak256, privateKeyToAddress, signHash, } from "src/ethereUtils"; @@ const decodedTransactions: SimpleMetaTransaction[] = existingCalls.map((call) => ({ to: call[0], value: BigInt(call[1]), - data: typeof call[2] !== "string" ? fromUtf8Bytes(call[2]) : call[2], + data: typeof call[2] !== "string" ? hexlify(call[2]) : call[2], }));Also applies to: 1427-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/account/Calibur/Calibur7702Account.ts` around lines 1 - 8, The code is converting nested bytes calldata using fromUtf8Bytes(call[2]) which corrupts binary calldata; replace that conversion with a hex-preserving conversion (use hexlify(call[2]) or equivalent) wherever call[2] is treated as inner ABI bytes (including the branch in Calibur7702Account.ts and the similar logic around the referenced 1427-1431 block); ensure the decoded path matches SafeAccount.decodeAccountCallData by hex-encoding raw bytes first before passing into decodeAbiParameters/encodeAbiParameters so selectors/arguments remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ethereUtils.ts`:
- Around line 347-369: fromUtf8Bytes currently reads continuation bytes without
validating their presence or form and can read past the buffer; update
fromUtf8Bytes to validate sequence lengths and continuation-byte patterns before
consuming b1/b2/b3 (check i < bytes.length and (bN & 0xc0) === 0x80), reject
overlong encodings and surrogate halves and ensure codepoint <= 0x10FFFF, and on
any invalid/truncated sequence either append the Unicode replacement character
(U+FFFD) or throw (choose consistent behavior with TextDecoder). Refer to the
function fromUtf8Bytes and its local variables b0, b1, b2, b3 and the loop
consuming bytes when adding these checks so you never use undefined bytes or
accept malformed UTF‑8.
---
Outside diff comments:
In `@src/account/Calibur/Calibur7702Account.ts`:
- Around line 1-8: The code is converting nested bytes calldata using
fromUtf8Bytes(call[2]) which corrupts binary calldata; replace that conversion
with a hex-preserving conversion (use hexlify(call[2]) or equivalent) wherever
call[2] is treated as inner ABI bytes (including the branch in
Calibur7702Account.ts and the similar logic around the referenced 1427-1431
block); ensure the decoded path matches SafeAccount.decodeAccountCallData by
hex-encoding raw bytes first before passing into
decodeAbiParameters/encodeAbiParameters so selectors/arguments remain intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8340884-7985-4f0b-b945-baa0c8dc7439
📒 Files selected for processing (4)
src/account/Calibur/Calibur7702Account.tssrc/account/Safe/SafeAccount.tssrc/account/Safe/adapters.tssrc/ethereUtils.ts
…r inner calldata fromUtf8Bytes accepted overlong, surrogate, out-of-range and truncated sequences and could read past the buffer, diverging from TextDecoder (e.g. 0xC0AF decoded to '/'). Validate the lead and continuation-byte ranges per the WHATWG decoder so every malformed sequence maps to U+FFFD, byte-for-byte identical to TextDecoder (verified by a 5000-case fuzz). Also fix the Calibur executor decode to hexlify the inner bytes calldata instead of UTF-8 decoding it, which corrupted the selector and arguments. Adds regression tests for fromUtf8Bytes.
The pure-JS decoder shipped in this PR was permissive — silently producing junk on malformed input (no bounds checks on continuation bytes, no overlong / surrogate-codepoint validation, bytes 0xf8..0xff treated as 4-byte leads). Neither TextDecoder() default (which replaces with U+FFFD) nor the historical SDK behavior (which threw) match that. Rewrite to throw on every malformed sequence: lead-byte classification per RFC 3629, validated continuation bytes, and explicit errors for overlong encodings, surrogate code points, and out-of-range code points. Use String.fromCodePoint for the code-point → string step so the 4-byte path doesn't need hand-rolled surrogate-pair math. For well-formed input the output is byte-identical to the previous implementation and to TextDecoder — the existing ABI string and WebAuthn JSON call sites are unaffected. Tests: round-trip parity with TextDecoder (100 random valid UTF-8 strings) plus 8 strict-mode rejection cases (bad prefix, truncated sequences, missing continuation, overlong, surrogate code point). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds regression coverage for the calldata fix that landed in 36eb578 (same swap I proposed in this branch, hexlify instead of fromUtf8Bytes for the BatchedCall data field). - 25-iteration property test: random batches with random binary data payloads (0..79 bytes per call, 1..4 calls per batch), asserts every original call's data round-trips bit-for-bit through the prepend. - Explicit pinning vector: 32 stray-continuation bytes (0x80..0x9f) that the prior TextDecoder/fromUtf8Bytes implementation specifically corrupted. Seeded Mulberry32 PRNG so any failure is reproducible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/calibur/caliburEip712.test.js (1)
207-212: ⚡ Quick winAssert the prepended
approve(address,uint256)payload as well.You already verify the prepended call target/value; adding selector + decoded-arg checks will catch silent regressions in spender/amount encoding inside
src/account/Calibur/Calibur7702Account.ts:1395-1450.✅ Suggested test hardening
// First call is the prepended approve(token, paymaster, amount). expect(outCalls.length).toBe(originals.length + 1); expect(outCalls[0][0].toLowerCase()).toBe(TOKEN.toLowerCase()); expect(outCalls[0][1]).toBe(0n); + const approveData = outCalls[0][2]; + expect(approveData.slice(0, 10).toLowerCase()).toBe('0x095ea7b3'); // approve(address,uint256) + const [spender, amount] = coder.decode( + ['address', 'uint256'], + `0x${approveData.slice(10)}`, + ); + expect(spender.toLowerCase()).toBe(PAYMASTER.toLowerCase()); + expect(amount).toBe(approveAmount); expect(outRevert).toBe(revertOnFailure);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/calibur/caliburEip712.test.js` around lines 207 - 212, Add assertions to verify the prepended approve(address,uint256) calldata: compute the selector for "approve(address,uint256)" and assert that the first 4 bytes of outCalls[0][2] equal that selector, then ABI-decode the remaining calldata to extract (spender, amount) and assert spender matches the expected paymaster/spender and amount matches the expected approval amount used by Calibur7702Account (refer to the approval logic in Calibur7702Account.ts around the 1395-1450 region). Use the test harness' ABI coder/utils (e.g., ethers.utils) to decode and compare values alongside the existing target/value assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/calibur/caliburEip712.test.js`:
- Around line 207-212: Add assertions to verify the prepended
approve(address,uint256) calldata: compute the selector for
"approve(address,uint256)" and assert that the first 4 bytes of outCalls[0][2]
equal that selector, then ABI-decode the remaining calldata to extract (spender,
amount) and assert spender matches the expected paymaster/spender and amount
matches the expected approval amount used by Calibur7702Account (refer to the
approval logic in Calibur7702Account.ts around the 1395-1450 region). Use the
test harness' ABI coder/utils (e.g., ethers.utils) to decode and compare values
alongside the existing target/value assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75696cfe-c1d3-4fc4-a55f-ffce8cc904b0
📒 Files selected for processing (3)
src/ethereUtils.tstest/calibur/caliburEip712.test.jstest/ethereUtils.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
generateOnChainIdentifier used Buffer.from and ABI string decoding used TextDecoder, both undefined in browsers (no polyfill) and React Native/Hermes. Replaced with the existing pure-JS toUtf8Bytes plus a new fromUtf8Bytes decoder, and moved the WebAuthn adapter and Calibur executor decode off TextEncoder/TextDecoder. Verified in headless Chrome, real Hermes, a real Android device (passkeys + live sponsored tx), and a real browser passkey sponsored tx.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests