[SDK] Improve SIWE chain management for external wallets #8432
Conversation
🦋 Changeset detectedLatest commit: 06d0538 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds a changeset documenting a patch release for improved SIWE chain management on external wallets. The primary code change introduces a NON_ETHEREUM_WALLETS constant in the SIWE authentication module with conditional logic branches that currently resolve to identical outcomes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Git: Failed to clone repository. Please run the 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
force-pushed
the
_SDK_Improve_SIWE_chain_management_for_external_wallets
branch
from
97f3a8e to
06d0538
Compare
![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)
packages/thirdweb/src/wallets/in-app/core/authentication/siwe.ts (2)
13-15: Align comment and naming with actual SIWE behaviorThe code makes sense (opt into
chainforNON_ETHEREUM_WALLETS, fallback to mainnet otherwise), but the comments/naming could better match behavior:
- Line 13: The comment says wallets "require a specific chain always", but the implementation still falls back to mainnet (
chain || getCachedChain(1)). Consider rephrasing to something like “wallets that prefer a specific chain for SIWE, but fall back to mainnet”.- Line 14: If
NON_ETHEREUM_WALLETSis meant to be a canonical list, you may want to add a short note or link indicating where wallet IDs like"xyz.abs"come from, so it’s easier to keep this list in sync with wallet metadata.- Lines 26–28: The branching is correct for the described behavior; if in the future you truly want to require a non‑mainnet chain for some wallets, you could tighten this by asserting that
chainis defined for those IDs instead of silently falling back.These are all clarity/maintainability points; behavior itself looks intentional.
Also applies to: 26-29
29-31: Fix small typo in commentLine 29:
alnready→already.- // only connect if the wallet doesn't alnready have an account + // only connect if the wallet doesn't already have an account
📜 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/sweet-lights-help.md(1 hunks)packages/thirdweb/src/wallets/in-app/core/authentication/siwe.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: joaquim-verges
Repo: thirdweb-dev/js PR: 7268
File: packages/thirdweb/src/wallets/in-app/core/wallet/in-app-core.ts:210-216
Timestamp: 2025-06-03T23:44:40.243Z
Learning: EIP7702 wallets do not need special handling for switching chains, unlike EIP4337 wallets which require reconnection when switching chains. In the switchChain method condition, EIP7702 should be intentionally excluded from the reconnection logic.
📚 Learning: 2025-06-03T23:44:40.243Z
Learnt from: joaquim-verges
Repo: thirdweb-dev/js PR: 7268
File: packages/thirdweb/src/wallets/in-app/core/wallet/in-app-core.ts:210-216
Timestamp: 2025-06-03T23:44:40.243Z
Learning: EIP7702 wallets do not need special handling for switching chains, unlike EIP4337 wallets which require reconnection when switching chains. In the switchChain method condition, EIP7702 should be intentionally excluded from the reconnection logic.
Applied to files:
.changeset/sweet-lights-help.mdpackages/thirdweb/src/wallets/in-app/core/authentication/siwe.ts
🔇 Additional comments (1)
.changeset/sweet-lights-help.md (1)
1-5: Changeset description looks goodPatch scope and message are clear and aligned with the SIWE chain-management change.
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8432 +/- ##
==========================================
- Coverage 54.83% 54.83% -0.01%
==========================================
Files 919 919
Lines 60865 60868 +3
Branches 4141 4141
==========================================
+ Hits 33373 33374 +1
- Misses 27390 27392 +2
Partials 102 102
🚀 New features to boost your workflow:
|
joaquim-verges
deleted the
_SDK_Improve_SIWE_chain_management_for_external_wallets
branch
PR-Codex overview
This PR focuses on improving the management of SIWE (Sign-In with Ethereum) chains for external wallets, particularly those that do not support the Ethereum mainnet.
Detailed summary
NON_ETHEREUM_WALLETSto identify wallets that cannot sign with Ethereum mainnet.siweAuthenticatefunction to conditionally setsiweChainbased on whether the wallet is inNON_ETHEREUM_WALLETS.Summary by CodeRabbit