feat: add default WalletConnect and Magic keys with environment fallbacks#41
feat: add default WalletConnect and Magic keys with environment fallbacks#41
Conversation
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #41
PR: feat: add default WalletConnect and Magic keys with environment fallbacks
Files changed: 2 (+135 −60)
CI: ✅ All checks passing (build, test, audit, lint)
Findings Summary
- P1 CRITICAL: 0
- P2 IMPORTANT: 3
- P3 NICE-TO-HAVE: 4
P2 — Important
1. [API Design] Two independent ways to disable Magic creates ambiguity
magicApiKey: false and connectors: { magic: false } both disable Magic, but the interaction is non-obvious. A consumer passing magicApiKey: 'pk_live_CUSTOM' with connectors: { magic: false } hits a silent contradiction — key provided but connector disabled. No other connector has this dual-toggle pattern (WalletConnect/Coinbase/Injected use only connectors.*).
Suggestion: make connectors.magic the sole toggle (consistent with the other connectors), and magicApiKey a string | undefined override only — drop the false literal from the union. This simplifies the resolution logic and gives consumers one obvious way to disable Magic.
2. [Type Safety] DEFAULT_MAGIC_API_KEYS is typed Record<string, string> instead of the environment union
An invalid environment value (e.g. from a JS caller) silently produces undefined, and Magic is skipped without warning. Tighten to:
const DEFAULT_MAGIC_API_KEYS: Record<NonNullable<Web3CoreConfigOptions['environment']>, string> = { ... }This makes the compiler enforce exhaustiveness if a fourth environment is added.
3. [Testing] Missing test coverage for key scenarios
- No test for providing a custom
magicApiKeystring (onlyfalseand defaults are tested) - No test for the interaction of
magicApiKey: false+connectors: { magic: true }(should produce no Magic connector) - No test for
environment: 'stg'— shares the same key as'dev'today, but should be exercised to guard against future divergence
P3 — Nice-to-Have
4. [Docs] README is now outdated
The README still shows the old additionalConnectors: [magic({ apiKey: '...' })] pattern. Should be updated to show the new zero-config default and connectors.magic: false opt-out.
5. [Clarity] Dev and staging share the same Magic key — document why
A one-line comment (// dev and stg share the same Magic application) would prevent future confusion about whether this is intentional or a copy-paste error.
6. [Maintenance] Test file re-hardcodes key values
The tests duplicate the key strings ('61570c542c2d66c659492e5b24a41522', 'pk_live_...') rather than referencing the source constants. If keys are rotated, tests must be updated separately. Consider exporting the defaults for test use, or importing them directly.
7. [Dead code] Null-coalescing on resolved metadata fields
After resolveAppMetadata(), description, url, and icons are always defined. The ?? '' / ?? [] guards on lines 245-248 are dead code — removing them signals that the resolver is trustworthy.
Security
No security issues found. The hardcoded keys are publishable/client-side keys (pk_live_* and WalletConnect project IDs) — these are inherently public in browser bundles and cannot perform privileged operations. The Magic SDK is still lazy-loaded via dynamic import() inside the connector, so eagerly importing the factory does not bundle Magic for consumers who disable it. ✅
One operational recommendation: configure domain restrictions on the Magic and WalletConnect dashboards to limit key usage to *.decentraland.org, which mitigates third-party quota abuse.
Verdict
Good change overall — the zero-config DX improvement is solid and the implementation is clean. The main concern is the dual-disable mechanism for Magic, which breaks consistency with the other connectors and will confuse consumers. Addressing P2 items would make the API much cleaner.
Requested by Braian Mellor via Slack
Adds default Decentraland API keys to
createWeb3CoreConfig()so apps don't need to configure them manually. Apps can still override any key.environmentoption: dev/stg use the test key, prd uses the production keyadditionalConnectors), controllable viaconnectors.magicmagicApiKey: falseto disable Magic entirelyBefore:
After:
Test plan
environment: 'prd'uses production Magic keymagicApiKey: falsedisables Magic connectorconnectors: { magic: false }disables Magic connectorwalletConnectProjectIdoverrides the default