[SDK] Fix signAuthorization for 1193 providers #8042
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 8c6378c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughAdds a changeset and updates the injected wallet implementation to include authorizationList in eth_sendTransaction, revise signAuthorization to try eth_sign then fall back to secp256k1_sign with signature parsing/merging, and simplify gas-fee handling. No exported/public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor D as Dapp
participant IW as InjectedWallet
participant P as EIP-1193 Provider
rect rgba(200,230,255,0.25)
Note over D,IW: signAuthorization flow (new)
D->>IW: signAuthorization(authorization)
IW->>P: eth_sign(address, payload)
alt eth_sign succeeds
P-->>IW: signature (hex)
else eth_sign fails
IW->>P: secp256k1_sign(payload)
P-->>IW: signature (hex)
end
IW->>IW: parse signature via ox__Signature.fromHex
IW-->>D: authorization + signature
end
sequenceDiagram
autonumber
actor D as Dapp
participant IW as InjectedWallet
participant P as EIP-1193 Provider
rect rgba(220,255,220,0.25)
Note over D,P: eth_sendTransaction with optional authorizationList
D->>IW: sendTransaction(tx)
IW->>IW: build tx<br/>- gasPrice or EIP-1559 fields<br/>- authorizationList (if provided)
IW->>P: eth_sendTransaction(txWithAuthList)
P-->>IW: txHash
IW-->>D: txHash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
✨ Finishing touches
🧪 Generate unit tests
Comment |
joaquim-verges
changed the title
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
joaquim-verges
force-pushed
the
joaquim/auth-1193
branch
from
5af0e1e to
8c6378c
Compare
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/shaggy-fans-live.md (1)
2-5: Changeset is scoped and leveled correctly (patch).Looks good. Consider expanding the note to explicitly mention the new eth_sendTransaction authorizationList support and the eth_sign → secp256k1_sign fallback behavior for traceability in release notes.
packages/thirdweb/src/wallets/injected/index.ts (3)
2-3: Imports: type-only for EIP1193 is good; consider lazy-loading ox modules.ox/Signature and ox/Authorization are only needed in signAuthorization/sendTransaction. To keep initial bundle lean, prefer dynamic imports inside those methods.
Example (outside selected lines):
// in signAuthorization const ox__Signature = await import("ox/Signature"); // in sendTransaction (only when tx.authorizationList is provided) const ox__Authorization = await import("ox/Authorization");
203-214: authorizationList transformation is correct; small ergonomics tweak optional.toRpcList on tx.authorizationList before spreading gas/other fields is correct for EIP-7702 RPC shape. Optional: only include the key when there’s at least one authorization to avoid serializing undefined.
- authorizationList: tx.authorizationList ? ox__Authorization.toRpcList(tx.authorizationList) : undefined, + ...(tx.authorizationList && tx.authorizationList.length > 0 + ? { authorizationList: ox__Authorization.toRpcList(tx.authorizationList) } + : {})
272-293: Sign flow improvement is solid; add a second secp256k1_sign param shape and better error surface.Some 1193 providers expect params as [address, payload] for secp256k1_sign. Add a guarded second attempt and preserve causes when all attempts fail. Also consider a short timeout to avoid hanging UIs.
async signAuthorization(authorization: AuthorizationRequest) { const payload = ox__Authorization.getSignPayload(authorization); - let signature: Hex | undefined; - try { - signature = await provider.request({ - method: "eth_sign", - params: [getAddress(account.address), payload], - }); - } catch { - // fallback to secp256k1_sign, some providers don't support eth_sign - signature = await provider.request({ - // @ts-expect-error - overriding types here - method: "secp256k1_sign", - params: [payload], - }); - } - if (!signature) { - throw new Error("Failed to sign authorization"); - } + let signature: Hex | undefined; + let err1: unknown; + try { + signature = await provider.request({ + method: "eth_sign", + params: [getAddress(account.address), payload], + }); + } catch (e) { + err1 = e; + } + if (!signature) { + try { + signature = await provider.request({ + // @ts-expect-error - provider-specific + method: "secp256k1_sign", + params: [payload], + }); + } catch (e2) { + try { + signature = await provider.request({ + // @ts-expect-error - provider-specific + method: "secp256k1_sign", + params: [getAddress(account.address), payload], + }); + } catch (e3) { + throw new Error( + `Failed to sign authorization via eth_sign or secp256k1_sign`, + ); + } + } + } const parsedSignature = ox__Signature.fromHex(signature as Hex); return { ...authorization, ...parsedSignature }; },Optional safety: recover and verify signer address matches account.address before returning to catch provider quirks.
Would you like me to add a quick check using viem’s recover to assert the signature corresponds to account.address?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/shaggy-fans-live.md(1 hunks)packages/thirdweb/src/wallets/injected/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/*.md: Each change inpackages/*must include a changeset for the appropriate package
Version bump rules: patch for non‑API changes; minor for new/modified public API
Files:
.changeset/shaggy-fans-live.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/wallets/injected/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/injected/index.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/injected/index.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/wallets/injected/index.ts
🧠 Learnings (1)
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Applied to files:
packages/thirdweb/src/wallets/injected/index.ts
🧬 Code graph analysis (1)
packages/thirdweb/src/wallets/injected/index.ts (1)
packages/thirdweb/src/exports/utils.ts (2)
numberToHex(82-82)getAddress(147-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Build Packages
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/wallets/injected/index.ts (1)
190-194: Gas fee handling LGTM.Using gasPrice branch with numberToHex is correct and cleanly overrides EIP-1559 fields via spread order.
PR-Codex overview
This PR focuses on fixing the
signAuthorizationimplementation for the1193provider in thethirdwebwallet, enhancing error handling and support for different signing methods.Detailed summary
sendTransactionto includeauthorizationListin the transaction parameters.signAuthorizationto:eth_signfirst.secp256k1_signif the first method fails.ox__Signature.fromHex.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores