[SDK] Support full MetaMask disconnection on wallet disconnect #8039
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2daa563 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 |
WalkthroughAdds a changeset for a patch release, updates the injected wallet disconnect flow to attempt an experimental MetaMask permission revocation ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant InjectedWallet
participant Provider as EIP-1193 Provider (MetaMask)
User->>App: trigger disconnect
App->>InjectedWallet: disconnect()
note right of InjectedWallet #f9f7e8: New step — attempt permission revoke
par Revoke attempt (with 100ms timeout)
InjectedWallet->>Provider: request("wallet_revokePermissions", [{ eth_accounts: {} }])
and Timeout
InjectedWallet-->>InjectedWallet: abort if >100ms
end
note right of InjectedWallet #f0f7ff: Swallow errors (RPC/timeout)
InjectedWallet-->>App: disconnect complete
App-->>User: disconnected
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing touches
🧪 Generate unit tests
Comment |
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. |
joaquim-verges
changed the title
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 (2)
.changeset/petite-lizards-create.md (1)
1-6: Capitalize MetaMask and clarify the mechanism in the changeset noteSuggest tightening the note and naming the RPC for clearer release notes. Also fix “metamask” casing.
Support fully disconnecting from metamask on disconnect +Support fully disconnecting from MetaMask on disconnect by revoking the +eth_accounts permission via `wallet_revokePermissions` (experimental).packages/thirdweb/src/wallets/injected/index.ts (1)
405-417: Gate MetaMask-only revoke, raise timeout, and await disconnect flow
- Only MetaMask documents
wallet_revokePermissionsforeth_accounts; other wallets may error or hang. Gate on MetaMask (provider flag or wallet id). (docs.metamask.io)- 100ms is too aggressive; MetaMask’s confirmation can legitimately take longer. Recommend 500–1000ms to reduce false timeouts while still preventing hangs. (github.com)
- Since
disconnect()is now meaningfully async, considerawait disconnect()insideonDisconnect()to ensure listeners are removed and revoke is attempted before emitting.Proposed patch:
async function disconnect() { provider.removeListener("accountsChanged", onAccountsChanged); provider.removeListener("chainChanged", onChainChanged); provider.removeListener("disconnect", onDisconnect); // Experimental support for MetaMask disconnect // https://github.com/MetaMask/metamask-improvement-proposals/blob/main/MIPs/mip-2.md - try { - // Adding timeout as not all wallets support this method and can hang - await withTimeout( - () => - provider.request({ - method: "wallet_revokePermissions", - params: [{ eth_accounts: {} }], - }), - { timeout: 100 } - ); - } catch {} + try { + const isMetaMask = + // common provider flag + (provider as any)?.isMetaMask === true || + // wallet id hints (adjust to your generated ids) + id === "io.metamask" || id === "com.metamask" || id === "metamask"; + if (isMetaMask) { + // Adding timeout as not all wallets support this method and can hang + await withTimeout( + () => + provider.request({ + method: "wallet_revokePermissions", + params: [{ eth_accounts: {} }], + }), + { timeout: 750 } // 500–1000ms recommended + ); + } + } catch { + // swallow: best-effort revoke + } }Additionally (outside this hunk), in
onDisconnect():- async function onDisconnect() { - disconnect(); + async function onDisconnect() { + await disconnect(); emitter.emit("disconnect", undefined); }Docs for revoke shape (params
[{ eth_accounts: {} }]) confirm current usage. (docs.metamask.io)
📜 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/petite-lizards-create.md(1 hunks)packages/thirdweb/src/wallets/injected/index.ts(6 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/petite-lizards-create.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 (2)
📓 Common learnings
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
Learnt from: CR
PR: thirdweb-dev/js#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T15:37:38.513Z
Learning: Applies to .changeset/*.md : Version bump rules: patch for non‑API changes; minor for new/modified public API
📚 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/wallets/eip5792/send-calls.ts (1)
toProviderCallParams(157-218)
⏰ 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, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/thirdweb/src/wallets/injected/index.ts (6)
114-115: LGTM — clearer warning textPunctuation tweak improves readability, no behavior change.
308-314: LGTM — explicit retry suppressionKeeping
{ retryCount: 0 }is appropriate for wallet UX here.
319-323: LGTM — cleaner call sitePassing
accountdirectly aligns withtoProviderCallParamssignature.
332-336: LGTM — actionable error messageMessage now includes wallet id and normalized payload; good for support.
348-351: LGTM — consistent guidance for unsupported EIP-5792Clearer wording without trailing punctuation artifact.
9-10: Import is correct — keep withTimeout imported from 'viem'package.json pins [email protected] and viem re-exports withTimeout at the top level, so the current import is valid. (npmjs.com)
joaquim-verges
force-pushed
the
Support_full_MetaMask_disconnection_on_wallet_disconnect
branch
from
875ff8d to
9d4154c
Compare
joaquim-verges
force-pushed
the
Support_full_MetaMask_disconnection_on_wallet_disconnect
branch
from
9d4154c to
2daa563
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thirdweb/src/extensions/erc721/read/getNFT.test.ts (1)
54-55: Avoid permanent skip; gate with env/runIf and add a clear TODO trackerTest is still skipped at packages/thirdweb/src/extensions/erc721/read/getNFT.test.ts:54-55 — replace the unconditional it.skip with an env-gated run and add a TODO marker.
Apply this diff:
- // skip until indexer restores owner functionality - it.skip("with owner using indexer", async () => { + // TODO(indexer-owner): Remove when indexer restores owner lookups. Set TW_INDEXER_OWNERS_RESTORED=true locally to run. + it.runIf(process.env.TW_INDEXER_OWNERS_RESTORED === "true")("with owner using indexer", async () => {Fallback if it.runIf isn't available:
const maybeIt = process.env.TW_INDEXER_OWNERS_RESTORED === "true" ? it : it.skip; // ... maybeIt("with owner using indexer", async () => { // existing body });
📜 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 (3)
.changeset/petite-lizards-create.md(1 hunks)packages/thirdweb/src/extensions/erc721/read/getNFT.test.ts(1 hunks)packages/thirdweb/src/wallets/injected/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/petite-lizards-create.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thirdweb/src/wallets/injected/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/extensions/erc721/read/getNFT.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
**/*.test.{ts,tsx}: Co‑locate tests asfoo.test.ts(x)next to the implementation
Use real function invocations with stub data; avoid brittle mocks
Use MSW to intercept HTTP calls for network interactions; mock only hard‑to‑reproduce scenarios
Keep tests deterministic and side‑effect free; use Vitest
Files:
packages/thirdweb/src/extensions/erc721/read/getNFT.test.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/extensions/erc721/read/getNFT.test.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/extensions/erc721/read/getNFT.test.ts
⏰ 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). (8)
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Analyze (javascript)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8039 +/- ##
==========================================
+ Coverage 56.63% 56.64% +0.01%
==========================================
Files 904 904
Lines 58684 58694 +10
Branches 4162 4163 +1
==========================================
+ Hits 33233 33247 +14
+ Misses 25345 25342 -3
+ Partials 106 105 -1
🚀 New features to boost your workflow:
|
joaquim-verges
deleted the
Support_full_MetaMask_disconnection_on_wallet_disconnect
branch
PR-Codex overview
This PR focuses on enhancing the
thirdweblibrary by adding support for fully disconnecting from MetaMask and updating tests to skip a specific case until functionality is restored.Detailed summary
wallet_revokePermissions.getNFT.test.tsto skip until owner functionality is restored.Summary by CodeRabbit
New Features
Tests
Chores