Skip to content

feat: event improvements#466

Merged
ignaciosantise merged 5 commits intofeat/multichainfrom
feat/events
Sep 30, 2025
Merged

feat: event improvements#466
ignaciosantise merged 5 commits intofeat/multichainfrom
feat/events

Conversation

@ignaciosantise
Copy link
Copy Markdown
Collaborator

@ignaciosantise ignaciosantise commented Sep 30, 2025

Summary

This pull request introduces several improvements and enhancements to event tracking, type safety, and wallet list handling in the AppKit package. The changes focus on providing richer analytics data, improving type consistency for network and namespace identifiers, and passing additional context when interacting with wallet items.

Event Tracking Enhancements:

  • Expanded analytics events to include more contextual properties, such as namespace, caipNetworkId, walletRank, displayIndex, and wallet name across various connection, disconnection, and wallet selection flows. This provides more granular insights into user actions and wallet interactions. [1] [2] [3] [4] [5] [6] [7] [8]

Type Safety and Consistency:

  • Updated the disconnect method and related hooks to use the ChainNamespace type instead of a plain string, ensuring better type safety and consistency when specifying namespaces. [1] [2] [3]
  • Standardized references to network identifiers to consistently use caipNetworkId rather than ambiguous or fallback string values. [1] [2] [3] [4] [5]

Wallet List and Selection Improvements:

  • Enhanced wallet list components (WalletItem, WalletList, and related props) to pass and handle a displayIndex and walletRank, providing more context for analytics and improving the ability to track user selection behavior. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Initialization and Controller Improvements:

  • Improved controller initialization logic to store and track the default network and emit an initialization event with relevant configuration details, such as theme, networks, and metadata. [1] [2] [3]
  • Ensured the WalletConnect connector is always initialized before restoring connected connectors, and added analytics for successful reconnections. [1] [2]

These changes collectively enhance the observability, reliability, and maintainability of the AppKit package, particularly in how user actions and wallet interactions are tracked and handled.


Note

Expands analytics events (including initialize, reconnect, and detailed wallet selection), standardizes caipNetworkId and ChainNamespace usage, passes wallet display index/rank, and improves connector init and error logging.

  • Analytics/Events:
    • Add INITIALIZE and richer CONNECT_SUCCESS/DISCONNECT_SUCCESS payloads (e.g., namespace, address, caipNetworkId, reconnect).
    • Standardize event properties: use explorerId (not explorer_id), optional network fields typed as CaipNetworkId.
    • Track wallet interactions with GET_WALLET, CLICK_GET_WALLET_HELP, and add walletRank, displayIndex, view on SELECT_WALLET.
    • Emit reconnect CONNECT_SUCCESS after session restore; include SWITCH_NETWORK network as caipNetworkId.
  • Types/Consistency:
    • Use ChainNamespace for disconnect; update hook signature.
    • Prefer caipNetworkId (remove empty-string fallbacks); update SIWX and send/swap event typings.
    • Update common types: remove polkadot from ChainNamespace; align NATIVE_TOKEN_ADDRESS.
  • Wallet Lists/UI:
    • Propagate displayIndex through WalletList/WalletItem and selection handlers; include walletRank in analytics.
  • Initialization/Connectors:
    • Always init WalletConnect before restoring sessions; send INITIALIZE event with config (theme, networks, features, adapters, etc.).
  • Logging:
    • Enhance LogController.sendError to handle error-like objects; improve cancel detection in send flow.

Written by Cursor Bugbot for commit b8cf55d. This will update automatically on new commits. Configure here.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 30, 2025

⚠️ No Changeset found

Latest commit: b8cf55d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

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

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude
Copy link
Copy Markdown

claude bot commented Sep 30, 2025

Claude finished @ignaciosantise's task —— View job


✅ Code Review Complete

This pull request introduces comprehensive event tracking improvements and type safety enhancements. Here's my detailed analysis:

Major Findings

🔒 Security Issues

  1. Potential Information Leakage in Events (packages/common/src/types/api/events.ts:198-435)

    • Events like ERROR_FETCH_TRANSACTIONS, LOAD_MORE_TRANSACTIONS, and SIWX_AUTH_ERROR include potentially sensitive properties like address, projectId, and message
    • These events are sent to analytics providers without clear data sanitization
    • Risk: User addresses and error messages could expose sensitive information
    • Recommendation: Implement data sanitization for all event properties before transmission
  2. Address Exposure in Success Events (AppKit.ts:413-423)

    • The CONNECT_SUCCESS event includes raw user addresses using CoreHelperUtil.getPlainAddress(address)
    • Multiple connection success events throughout the codebase expose addresses
    • Risk: Address correlation across analytics services
    • Recommendation: Hash or truncate addresses before including in analytics

🚨 Critical Code Quality Issues

  1. Memory Leak Risk in Event Properties (w3m-connecting-mobile/index.tsx:96-106, w3m-connecting-external-view/index.tsx:83-93)

    • Event tracking objects are created in render/effect cycles without proper cleanup
    • Complex nested objects in event properties could accumulate
    • Risk: Memory pressure on low-end devices
    • Recommendation: Consider object pooling or simpler event payloads
  2. Inconsistent Error Handling (w3m-connecting-external-view/index.tsx:95-109)

    • Error regex matching using /Wallet not found/i and /rejected/i is fragile
    • Different error types use different matching strategies
    • Risk: Unhandled error cases, inconsistent UX
    • Recommendation: Standardize error classification with explicit error codes

Performance Concerns

  1. Inefficient WalletItem Re-rendering (WalletItem.tsx:46-48)

    • memo only compares item.id but component receives displayIndex prop
    • Changes to displayIndex will cause unnecessary re-renders despite memoization
    • Impact: List performance degradation with many wallets
    • Fix: Include displayIndex in memo comparison or remove if not needed for render
  2. Potential Infinite useCallback Loop (w3m-connecting-mobile/index.tsx:83-116)

    • onConnect callback depends on data from RouterController.state.data
    • If data object reference changes frequently, could cause callback recreation
    • Risk: Effect re-execution and performance issues
    • Recommendation: Destructure specific properties or use refs for stable dependencies

📋 Type Safety & Best Practices

  1. Improved Type Safety

    • Good use of ChainNamespace type instead of strings in useAppKit.ts:34-35
    • Consistent caipNetworkId usage throughout the codebase
    • Event type definitions are comprehensive and well-structured
  2. Missing Changeset ⚠️

    • The changeset-bot correctly identified that this PR needs a changeset
    • These changes affect public APIs and should trigger version bumps
    • Action Required: Add changeset before merging

🔧 Minor Issues

  1. Console Logging in Production (LogController.ts:100-102)

    • Debug logging is properly gated behind debug flag ✅
    • Good sanitization of sensitive data in logs ✅
  2. Event Tracking Enhancement

    • Addition of walletRank, displayIndex, and contextual properties improves analytics
    • Proper namespace tracking in disconnect events
    • Well-structured event properties for business intelligence

Recommendations

Immediate Actions Required:

  1. Add data sanitization for event properties before analytics transmission
  2. Hash or truncate addresses in analytics events
  3. Fix WalletItem memoization to include displayIndex comparison
  4. Add changeset file for version management

Suggested Improvements:

  • Implement error code enum instead of regex-based error detection
  • Consider rate limiting for analytics events to prevent spam
  • Add event property validation schemas
  • Document the analytics data collection policy

Overall Assessment

This is a solid enhancement to event tracking and type safety. The code quality is generally good with proper error handling and TypeScript usage. However, the security concerns around data exposure in analytics events need immediate attention before merging.

Recommendation: Address security issues and add changeset before approval.


cursor[bot]

This comment was marked as outdated.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Memoization Ignores Index, Causing Analytics Issues

The MemoizedWalletItem's memo comparison only checks item.id, but the component now receives a displayIndex prop. This prevents re-renders when displayIndex changes, which can lead to stale displayIndex values being passed to onItemPress and inaccurate analytics.

packages/appkit/src/partials/w3m-all-wallets-list/components/WalletItem.tsx#L45-L48

export const MemoizedWalletItem = memo(WalletItem, (prevProps, nextProps) => {
return prevProps.item.id === nextProps.item.id;
});

Fix in Cursor Fix in Web


@ignaciosantise ignaciosantise merged commit ef9db7a into feat/multichain Sep 30, 2025
8 checks passed
@ignaciosantise ignaciosantise deleted the feat/events branch September 30, 2025 18:46
@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2025
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.

1 participant