Skip to content

Add WalletConnect logger option#558

Merged
ignaciosantise merged 3 commits into
developfrom
disable-walletconnect-core-logs
May 20, 2026
Merged

Add WalletConnect logger option#558
ignaciosantise merged 3 commits into
developfrom
disable-walletconnect-core-logs

Conversation

@ignaciosantise
Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise commented May 15, 2026

This adds a logger option to createAppKit and forwards it through connector initialization into @walletconnect/universal-provider, with a changeset for the release.
The public types now accept WalletConnect logger levels including fatal, so consumers can disable or tune WalletConnect core logs without passing invalid values.
This gives SDK integrators explicit control over WalletConnect logging while preserving the default behavior when logger is omitted.
Validation: yarn format, yarn lint, and yarn test.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 15, 2026

🦋 Changeset detected

Latest commit: d56c6db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@reown/appkit-react-native Patch
@reown/appkit-bitcoin-react-native Patch
@reown/appkit-coinbase-react-native Patch
@reown/appkit-common-react-native Patch
@reown/appkit-core-react-native Patch
@reown/appkit-ethers-react-native Patch
@reown/appkit-solana-react-native Patch
@reown/appkit-ui-react-native Patch
@reown/appkit-wagmi-react-native Patch

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

@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

Claude finished @ignaciosantise's task in 2m 4s —— View job


PR Review

  • Gather context
  • Read changed files
  • Run data classification subagent — no issues found
  • Run breaking changes subagent — no issues found
  • Post consolidated review

Found 2 issue(s)

Issue 1: Type duplication — WalletConnectLoggerLevel and WalletConnectLogger redefined locally instead of imported

ID: types-logger-type-duplication-a3f1
File: packages/appkit/src/types.ts:16-26
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: WalletConnectLoggerLevel (line 16) and WalletConnectLogger (lines 18–26) are defined as local private types in types.ts, byte-for-byte copies of the types exported from packages/common/src/types/wallet/connector.ts (lines 35–53), which are re-exported through the @reown/appkit-common-react-native package already imported at the top of types.ts.
  • Risk: Two canonical definitions of the same type in the same repo. If one is updated (e.g., a new log level added to WalletConnectLoggerLevel), the other silently falls out of sync without a compile error, because the local types in types.ts are private and not structurally checked against the exported ones.
  • Impact: Silent drift between the public API type (AppKitConfig.logger) and the internal connector type (ConnectorInitOptions.logger), leading to consumers being able to pass values that the connector rejects at runtime.
  • Trigger: Any future change to WalletConnectLoggerLevel or WalletConnectLogger in connector.ts that isn't mirrored in types.ts.

Recommendation: Import from common instead of redefining:

import {
  // ...existing imports...
  type WalletConnectLoggerLevel,
  type WalletConnectLogger,
} from '@reown/appkit-common-react-native';

Fix this →


Issue 2: Unsafe type cast bridges structural mismatch silently

ID: wc-connector-logger-cast-b2e7
File: packages/appkit/src/connectors/WalletConnectConnector.ts:37
Severity: LOW
Category: code_quality

Context:

  • Pattern: ops.logger as UniversalProviderOpts['logger'] asserts the custom WalletConnectLogger shape into WalletConnect's internal logger type without structural verification. UniversalProviderOpts['logger'] is pino's Logger, which has additional members beyond the subset defined in WalletConnectLogger (e.g., level, silent, levelVal, isLevelEnabled, serializers, etc.).
  • Risk: A consumer passing a minimal custom logger object (matching only WalletConnectLogger) will pass TypeScript compilation but may cause a runtime error if WalletConnect internally accesses pino-specific fields like .level or .isLevelEnabled.
  • Impact: Runtime crash or silent no-op when a consumer passes a custom logger object and WalletConnect tries to introspect it.
  • Trigger: Consumer passes a custom logger object (not a string level); WalletConnect internally reads pino-specific fields on it.

Recommendation: Narrow the cast or validate at the boundary:

// Option A: use `unknown` cast chain to make the assertion explicit
logger: ops.logger as unknown as UniversalProviderOpts['logger']

// Option B: accept only string levels for now if full pino Logger is too broad for the type
logger?: WalletConnectLoggerLevel  // drop custom logger object support until types align

@ignaciosantise ignaciosantise changed the title [codex] Add WalletConnect logger option Add WalletConnect logger option May 15, 2026
@ignaciosantise ignaciosantise marked this pull request as ready for review May 19, 2026 11:34
Copilot AI review requested due to automatic review settings May 19, 2026 11:34
@ignaciosantise
Copy link
Copy Markdown
Collaborator Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Claude finished @ignaciosantise's task in 1m 28s —— View job


PR Review

  • Gather context
  • Read changed files
  • Check previous issues (types-logger-type-duplication-a3f1, wc-connector-logger-cast-b2e7)
  • Post consolidated review

🔄 Incremental Review: checking current code state against the two issues flagged in the previous auto-review.

Resolved since last review:

  • types-logger-type-duplication-a3f1 — Local WalletConnectLoggerLevel / WalletConnectLogger definitions in types.ts are gone. AppKitConfig.logger now uses ConnectorInitOptions['logger'] which resolves through the single canonical definition in connector.ts.
  • wc-connector-logger-cast-b2e7 — The as UniversalProviderOpts['logger'] cast is gone. ConnectorInitOptions['logger'] is now narrowed to a string-only union (WalletConnectLoggerLevel), and the connector guards with typeof ops.logger === 'string' before passing it through.
Found 1 issue(s)

Issue 1: Private helper loses type precision — logger?: string instead of logger?: WalletConnectLoggerLevel

ID: wc-connector-logger-private-sig-9c2e
File: packages/appkit/src/connectors/WalletConnectConnector.ts:93
Severity: LOW
Category: code_quality

Context:

  • Pattern: getUniversalProvider's destructured parameter is typed as logger?: string, losing the narrowed WalletConnectLoggerLevel union from the public ConnectorInitOptions.
  • Risk: Any string is accepted inside the private method, so a future refactor that reads or re-passes logger within this method won't get autocomplete or compile-time exhaustiveness checking.
  • Impact: No runtime impact today; potential silent breakage if a caller within the class passes an invalid level string.
  • Trigger: Internal refactoring or copy-paste of the private helper.

Recommendation: Use the specific type in the private signature:

import { type WalletConnectLoggerLevel } from '@reown/appkit-common-react-native';

// in getUniversalProvider signature:
logger?: WalletConnectLoggerLevel;

Also: the typeof ops.logger === 'string' guard at line 33 is redundant given the type is already WalletConnectLoggerLevel | undefined (a string union). It can be simplified to const logger = ops.logger; — though it's harmless as a runtime defence.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an optional logger configuration to AppKit so SDK integrators can control WalletConnect (@walletconnect/universal-provider) log verbosity, while preserving default behavior when omitted.

Changes:

  • Extend public types to include a WalletConnect logger level and expose it via AppKitConfig.logger.
  • Thread logger through connector initialization and into UniversalProvider.init(...).
  • Add a changeset describing the new option.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/common/src/types/wallet/connector.ts Introduces WalletConnectLoggerLevel and adds logger? to ConnectorInitOptions.
packages/appkit/src/types.ts Exposes logger? on AppKitConfig (public API surface).
packages/appkit/src/connectors/WalletConnectConnector.ts Forwards logger into UniversalProvider.init during connector initialization.
packages/appkit/src/AppKit.ts Passes logger from AppKit config into connector init(...) calls.
.changeset/walletconnect-logger-option.md Adds release notes/versions for the new logger option.
Comments suppressed due to low confidence (1)

packages/appkit/src/connectors/WalletConnectConnector.ts:94

  • getUniversalProvider types logger as a plain string, which weakens internal type-safety and allows accidental invalid values. Consider typing this parameter as WalletConnectLoggerLevel (or ConnectorInitOptions['logger']) to keep it consistent with the public config type.
  }: {
    projectId: string;
    metadata: Metadata;
    logger?: string;
  }): Promise<UniversalProvider> {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .changeset/walletconnect-logger-option.md
Comment thread packages/common/src/types/wallet/connector.ts
Comment thread packages/appkit/src/connectors/WalletConnectConnector.ts
@sonarqubecloud
Copy link
Copy Markdown

@ignaciosantise ignaciosantise merged commit f38aada into develop May 20, 2026
11 checks passed
@ignaciosantise ignaciosantise deleted the disable-walletconnect-core-logs branch May 20, 2026 13:13
@github-actions github-actions Bot locked and limited conversation to collaborators May 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants