Add analytics tracking to smart accounts kit#185
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
ed770dc to
b3b5e0c
Compare
|
@SocketSecurity ignore npm/nock@14.0.11 |
|
@SocketSecurity ignore npm/openapi-fetch@0.13.8 |
- fix openapi-fetch dependency issue - export DelegationStorageEnvironment
…te retries and bloating re-enqueues
9f35cd7 to
208dfa2
Compare
| trackSmartAccountsKitFunctionCall('createDelegation', { | ||
| hasCaveats: options.caveats !== undefined, | ||
| hasParentDelegation: options.parentDelegation !== undefined, |
There was a problem hiding this comment.
should also track which ScopeConfig was used.
There was a problem hiding this comment.
absolutely! I've added the scope name to the analytics parameters
MoMannn
left a comment
There was a problem hiding this comment.
Maybe also tracking: sign delegations actions - so we know how much got signed. Or can this be extracted from storage?
|
|
||
| ```sh | ||
| yarn add @metamask/smart-accounts-kit | ||
| yarn add @metamask/smart-accounts-kit viem openapi-fetch |
There was a problem hiding this comment.
Are we going to force users to install openapi-fetch? Could it rather be bundled with analytics package otherwise this is kind of weird from users perspective that it needs openapi?
There was a problem hiding this comment.
100%!
Analytics package is private and not distributed, so it cannot define strict dependencies. As such it defines a peer dependency - smart-accounts-kit though definitely should be defining a depdency on openapi-fetch, so I've fixed that.
| @@ -0,0 +1,140 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
| /* eslint-disable-next-line id-length */ | |||
| import * as t from 'vitest'; | |||
There was a problem hiding this comment.
nit: why not import like in other tests import { describe, it, expect } from 'vitest';?
There was a problem hiding this comment.
There's a couple of idiosyncrasies from the original package. I've tried to avoid making changes as much as possible, so that we can easily migrate to a distributed analytics package shared between this and mm connect.
| } | ||
| } | ||
|
|
||
| export default Sender; |
There was a problem hiding this comment.
Should this rather be: export { Sender }?
There was a problem hiding this comment.
As above:
There's a couple of idiosyncrasies from the original package. I've tried to avoid making changes as much as possible, so that we can easily migrate to a distributed analytics package shared between this and mm connect.
| } | ||
|
|
||
| this.isSending = true; | ||
| const current = [...this.batch.slice(0, this.batchSize)]; |
There was a problem hiding this comment.
nit: slice already returns a new array, so this could be: const current = this.batch.slice(0, this.batchSize)
There was a problem hiding this comment.
Same as above:
There's a couple of idiosyncrasies from the original package. I've tried to avoid making changes as much as possible, so that we can easily migrate to a distributed analytics package shared between this and mm connect.
| ): Delegation => { | ||
| trackSmartAccountsKitFunctionCall('createDelegation', { | ||
| hasCaveats: options.caveats !== undefined, | ||
| hasParentDelegation: options.parentDelegation !== undefined, |
There was a problem hiding this comment.
can maybe names of the caveats used so that we know which are most popular?
There was a problem hiding this comment.
👍 done!
We resolve the caveat names after building the caveats, so that includes the caveats that come from the scope, but we get the scope name too, so should be able to get a good idea of what they are doing.
| const options: Options = { | ||
| ...config, | ||
| // Bundle internal analytics into the published kit (not a separate install). | ||
| noExternal: ['@metamask/smart-accounts-kit-analytics'], |
There was a problem hiding this comment.
CJS bundles duplicate analytics singleton across entry points
Medium Severity
The base tsup config has splitting: true, but esbuild does not support code splitting for CJS output. With noExternal: ['@metamask/smart-accounts-kit-analytics'] and five entry points, each CJS bundle gets its own inlined copy of analytics.ts and the bundled environment.ts module state (session, hasBootstrapped, the analytics singleton). CJS consumers importing from multiple sub-paths (e.g., main + /actions) get separate analytics instances, causing duplicate smart_accounts_kit_initialized events, different anon_id values, and function call tracking using an uninitialized session.
Additional Locations (1)
7a72157 to
460383a
Compare
- Smart Accounts Kit package now declares a dependency on openapi-fetch - 'error' parameter in catch clause is removed - Disable analytics in CI - Add typecheck for global process to
460383a to
c82534b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| ): Delegation => { | ||
| const caveats = resolveCaveats(options); | ||
|
|
||
| trackSmartAccountsKitFunctionCall('createDelegation', { |
There was a problem hiding this comment.
Wrong function name tracked in createOpenDelegation analytics
Medium Severity
The createOpenDelegation function tracks itself as 'createDelegation' instead of 'createOpenDelegation'. This appears to be a copy-paste error from the createDelegation tracking call above. Analytics data will incorrectly conflate calls to these two distinct functions, making it impossible to distinguish between targeted and open delegations.


📝 Description
Adds (private) analytics package - duplicated and adapted from connect-monorepo, to send telemetry data to mm analytics service.
Standard do-not-track identifiers are respected:
DO_NOT_TRACKenvironment variablenavigator.doNotTrack(in browser)window.doNotTrack(in browser)🔄 What Changed?
Calls
ensureAnalyticsBootstrapped()in each of the top level exports included in @metamask/smart-accounts-kit to ensure that the analytics is set up whenever the package is included in an application.Metadata is resolved once per instantiation, and reused for all requests (static anonymous identifier), but not stored in any way. This means that events may be correlated between a user, but only within the lifecycle of the sdk instance - if the webpage is reloaded, or the user navigates to another page, there is no correlation of the user between those instances.
Two events are added:
smart_accounts_kit_initializedwhen the SDK is initializedsmart_accounts_kit_function_calledfor any function call of an "important" top level functionNote: the analytics package diverges slightly in style from the rest of the monorepo - for instance using nock for http testing. This is true to the original implementation in the mm-connect monorepo. I decided to keep as consistent as possible, to make migration to a published package in the future simplier.
🚀 Why?
Allows us to make informed decisions about the design and direction of the SDK, without tracking excessive information.
🧪 How to Test?
Describe how to test these changes:
List any breaking changes:
📋 Checklist
Check off completed items:
🔗 Related Issues
Link to related issues:
Closes #
Related to #
📚 Additional Notes
Any additional information, concerns, or context:
Note
Medium Risk
Adds outbound telemetry and import-time side effects across the kit (auto-bootstrap on top-level exports), which could impact privacy expectations, bundling/tree-shaking behavior, and runtime/network failure modes despite DNT/CI guards.
Overview
Adds a new private workspace package,
@metamask/smart-accounts-kit-analytics, implementing a batched analytics client (Senderwith backoff), session/base-property handling, and two events:smart_accounts_kit_initializedandsmart_accounts_kit_function_called.In
@metamask/smart-accounts-kit, introducessrc/analytics.tsto bootstrap analytics once (disabled byDO_NOT_TRACK/browser DNT and in CI), calls that bootstrap on import of top-level module entrypoints (index,actions,contracts,utils,experimental), and instruments key public functions to emit function-call telemetry with limited metadata (e.g.,chainId, counts, implementation flags, caveat names).Build/test packaging is updated to bundle the analytics package into the published kit (
tsupnoExternal), adjustsideEffectsto preserve import-time bootstrap, addopenapi-fetchdependency, and setDO_NOT_TRACK=truefor kit unit tests; README also clarifiesviempeer installation.Written by Cursor Bugbot for commit c82534b. This will update automatically on new commits. Configure here.