Conversation
- Use RPC_METHODS.* constants in ANALYTICS_TRACKED_RPC_METHODS and add case-sensitivity comment documenting intentional behaviour - Add transport_type/rpc_method properties assertion to analytics tests in connectToChannel, handleConnectionMessage, and handleSendMessage
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
jiexi
reviewed
Apr 2, 2026
Comment on lines
+57
to
+58
| // Comparison is intentionally case-sensitive: RPC method names are standardised | ||
| // and the SDK is expected to send them in canonical casing. |
Member
There was a problem hiding this comment.
Suggested change
| // Comparison is intentionally case-sensitive: RPC method names are standardised | |
| // and the SDK is expected to send them in canonical casing. |
jiexi
reviewed
Apr 2, 2026
| RPC_METHODS.METAMASK_CONNECTSIGN, | ||
| RPC_METHODS.METAMASK_CONNECTWITH, | ||
| RPC_METHODS.METAMASK_BATCH, | ||
| ]; |
Member
There was a problem hiding this comment.
we're missing some rpc methods here like wallet_revokePermissions, wallet_watchAsset i think
Align event names with segment-schema PR #512 review feedback: - wallet_connection_request_received → MMConnect Request Received - wallet_action_received → SDK RPC Request Received - wallet_action_user_approved → SDK RPC Request Approved - wallet_action_user_rejected → SDK RPC Request Rejected Uses Title Case "Object + Past-tense Verb" naming per repo conventions. Enum keys updated accordingly (MMCONNECT_REQUEST_RECEIVED, SDK_RPC_REQUEST_RECEIVED, etc.).
The event fires for all remote connection paths (SDK v1 socket relay, MetaMask Connect / MWP, and eventually WalletConnect), not just MMConnect. "Remote" correctly scopes it to connections that have a pre-permission lifecycle step distinct from the transport-agnostic Connect Request Started/Completed/Cancelled family.
Rename events to reflect cross-transport scope: - SDK RPC Request Received → Remote Connection RPC Request Received - SDK RPC Request Approved → Remote Connection RPC Request Approved - SDK RPC Request Rejected → Remote Connection RPC Request Rejected These events track post-connection RPC method calls from remotely connected dapps. Renaming from "SDK" to "Remote Connection" aligns with the segment-schema PR (Consensys/segment-schema#520) and sets up the schema for future MWP and WalletConnect instrumentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28322 +/- ##
==========================================
- Coverage 82.66% 82.12% -0.54%
==========================================
Files 4866 4911 +45
Lines 126134 129040 +2906
Branches 28268 28757 +489
==========================================
+ Hits 104273 105979 +1706
- Misses 14660 15790 +1130
- Partials 7201 7271 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ffmcgee725
reviewed
Apr 8, 2026
| if (anonId) { | ||
| DevLogger.log( | ||
| `[MM SDK Analytics] event=wallet_connection_request_received anonId=${anonId}`, | ||
| `[MM SDK Analytics] event=Remote Connect Request Received anonId=${anonId}`, |
Member
There was a problem hiding this comment.
Suggested change
| `[MM SDK Analytics] event=Remote Connect Request Received anonId=${anonId}`, | |
| `[MM SDK Analytics] event=${MetaMetricsEvents.REMOTE_CONNECT_REQUEST_RECEIVED} anonId=${anonId}`, |
is this a valid suggestion ? I'm assuming the value of MetaMetricsEvents.REMOTE_CONNECT_REQUEST_RECEIVED is what we want here
ffmcgee725
reviewed
Apr 8, 2026
| @@ -180,9 +188,17 @@ async function connectToChannel({ | |||
| DevLogger.log( | |||
| `[MM SDK Analytics] event=wallet_connection_user_approved anonId=${anonId}`, | |||
Member
There was a problem hiding this comment.
Suggested change
| `[MM SDK Analytics] event=wallet_connection_user_approved anonId=${anonId}`, | |
| `[MM SDK Analytics] event=${MetaMetricsEvents.WALLET_CONNECTION_USER_APPROVED} anonId=${anonId}`, |
same here if valid, even though you did not touch this piece of code, might as well do some cleaning (IF MetaMetricsEvents.WALLET_CONNECTION_USER_APPROVED is wallet_connection_user_approved which I'm not 100% sure of)
ffmcgee725
reviewed
Apr 8, 2026
| @@ -193,20 +209,21 @@ async function connectToChannel({ | |||
| DevLogger.log( | |||
| `[MM SDK Analytics] event=wallet_connection_user_rejected anonId=${anonId}`, | |||
ffmcgee725
reviewed
Apr 8, 2026
| if (anonId && ANALYTICS_TRACKED_RPC_METHODS.includes(message.method)) { | ||
| DevLogger.log( | ||
| `[MM SDK Analytics] event=wallet_action_received anonId=${anonId}`, | ||
| `[MM SDK Analytics] event=Remote Connection RPC Request Received anonId=${anonId}`, |
…1 events (#28390) ## Summary Same rationale as the V2 cleanup (#28358): V1 connections flow through the connection approval UI, which already fires `CONNECT_REQUEST_COMPLETED` / `CONNECT_REQUEST_CANCELLED`. Adding `wallet_connection_user_approved` / `wallet_connection_user_rejected` in the relay layer re-implements the same signal. **Removed:** * `wallet_connection_user_approved` tracking from `connectToChannel.ts` * `wallet_connection_user_rejected` tracking from `connectToChannel.ts` * Both event definitions from `MetaMetrics.events.ts` * Both test cases from `connectToChannel.test.ts` **Kept** (no existing MetaMetrics event covers these): * `Remote Connect Request Received` — fires before the permission check, needed to measure dapp-to-wallet drop-off * `Remote Connection RPC Request *` events — track per-RPC-method confirmation lifecycle, separate concern from connection approval **Renames in latest commit:** The `SDK RPC Request *` events have been renamed to `Remote Connection RPC Request *` to reflect their intended cross-transport scope. Currently only SDKv1 is instrumented but the schema supports `socket_relay`, `mwp`, and `walletconnect` transports. **Context:** The `useOriginSource` fix (WAPI-1380, #28290) ensures the `source` property is correct for both V1 and V2 connections on all permission events, so the existing events already carry the right attribution. ## Related schema PRs - [Consensys/segment-schema#519](Consensys/segment-schema#519) — `Remote Connect Request Received/Failed` schemas - [Consensys/segment-schema#520](Consensys/segment-schema#520) — `Remote Connection RPC Request Received/Approved/Rejected` schemas ## Test plan * `connectToChannel.test.ts` — all remaining tests pass * `handleConnectionMessage.test.ts` — updated for new event names * `handleSendMessage.test.ts` — updated for new event names <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: removes redundant MetaMetrics tracking and associated tests, with no changes to connection/permission behavior beyond analytics emission. > > **Overview** > Removes the SDK v1 relay-layer MetaMetrics events `wallet_connection_user_approved` and `wallet_connection_user_rejected`, including their definitions in `MetaMetrics.events.ts`. > > `connectToChannel.ts` no longer emits these events on `checkPermissions` success/failure, relying instead on the permission UI’s existing `CONNECT_REQUEST_COMPLETED` / `CONNECT_REQUEST_CANCELLED` tracking. The corresponding unit tests asserting those emissions are deleted and replaced with a clarifying comment. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1dc8ab6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
14 tasks
- Rename `Remote Connect Request Received` → `Remote Connection Request Received` to match the finalized naming convention in segment-schema PR #519 - Rename `anon_id` property → `remote_session_id` across all remote connection/RPC analytics events for clarity - Update corresponding test assertions
…rties Aligns with the decision in PR #28470 — remote_session_id is a dapp-generated UUID for cross-side analytics correlation, not sensitive data. Keeping it in sensitiveProperties triggered the anonymous event split which prevented linking dapp sessions to wallet user activity.
Contributor
🔍 Smart E2E Test Selection⏭️ Smart E2E selection skipped - draft PR All E2E tests pre-selected. |
|
Contributor
|
✅ E2E Fixture Validation — Schema is up to date |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SDKv1 (socket relay) was tracking wallet-side analytics through two channels:
@metamask/sdk-analytics— external package with its own telemetry backend; enabled inSDKConnect.init()viaanalytics.enable().SendAnalyticsfrom@metamask/sdk-communication-layer— posted events directly tosocketServerUrl/evtBoth bypassed the MetaMetrics opt-in/opt-out pipeline entirely.
This PR removes both and replaces all call sites with
analytics.trackEvent()throughAnalyticsController.Four events are added to
MetaMetrics.events.ts:Remote Connection Request Received— fired inconnectToChannelwhen a connection request arrives (pre-permission, measures dapp→wallet drop-off)Remote Connection RPC Request Received— fired inhandleConnectionMessagewhen an RPC message arrivesRemote Connection RPC Request Approved/Remote Connection RPC Request Rejected— fired inhandleSendMessageon RPC outcomewallet_connection_user_approvedandwallet_connection_user_rejectedare intentionally omitted — the permission UI already firesCONNECT_REQUEST_COMPLETED/CONNECT_REQUEST_CANCELLEDwhich cover the same signal.A new
ANALYTICS_TRACKED_RPC_METHODSconstant inSDKConnectConstants.tsreplaces both theisAnalyticsTrackedRpcMethodhelper fromsdk-communication-layerand a local duplicate array.All events include
transport_type: 'socket_relay'to identify the SDKv1 source,rpc_methodfor RPC events,sdk_versionfromoriginatorInfo.apiVersion, andremote_session_id(a dapp-generated UUID for cross-side session correlation, kept in regular properties so the MetaMetrics privacy plugin does not split it into a separate anonymous event).Changelog
CHANGELOG entry: null
Related issues
Fixes: WAPI-1375
Manual testing steps
Screenshots/Recordings
Before
N/A — analytics-only change, no UI changes.
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist