Skip to content

Commit

Permalink
feat: Add RPC Method Middleware params transform and tracking (#23236)
Browse files Browse the repository at this point in the history
## **Description**

Currently we track a sample of most RPC method calls, but we do not
include params. This is for good reason of course since many param
values are sensitive and may identify an otherwise anonymous MetaMask
Metrics user. There are some param values that are not sensitive yet are
not tracked as well. Our goal is to glean additional insight into the
usage of our RPC methods by including non-sensitive params in the events
that track them.

This PR achieves that by including a map of method to transformation
functions which accepts a request's params object (or array) and
transforms it something that is acceptable for analytics. By default no
params are included.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23236?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2167

## **Manual testing steps**

1. Opt into MetaMetrics in the wallet settings
1. Open your the background console for extension
1. Add a breakpoint for trackEvent inside
`createRPCMethodTrackingMiddleware.js`
1. On any page with a provider injected, use `window.ethereum.request`
to make some rpc method calls
1. Check background console for breakpoint trigger
* Currently only `wallet_watchAsset` should include the `type` param.
All other methods should not include any params.
* [the wallet_watchAsset in API
playground](https://docs.metamask.io/wallet/reference/wallet_watchasset/)
may be helpful to use

NOTE: I recommend breakpoint and debugger over the network tab because I
don't believe the tracking requests actually fire on dev.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Alex Donesky <[email protected]>
  • Loading branch information
jiexi and adonesky1 authored May 22, 2024
1 parent 31826eb commit 7c36209
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 1 deletion.
16 changes: 15 additions & 1 deletion app/scripts/lib/createRPCMethodTrackingMiddleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,15 @@ const EVENT_NAME_MAP = {
},
};

/**
* This object maps a method name to a function that accept the method params and
* returns a non-sensitive version that can be included in tracked events.
* The default is to return undefined.
*/
const TRANSFORM_PARAMS_MAP = {
[MESSAGE_TYPE.WATCH_ASSET]: ({ type }) => ({ type }),
};

const rateLimitTimeoutsByMethod = {};
let globalRateLimitCount = 0;

Expand Down Expand Up @@ -170,7 +179,7 @@ export default function createRPCMethodTrackingMiddleware({
/** @type {any} */ res,
/** @type {Function} */ next,
) {
const { origin, method } = req;
const { origin, method, params } = req;

const rateLimitType =
RATE_LIMIT_MAP[method] ?? RATE_LIMIT_TYPES.RANDOM_SAMPLE;
Expand Down Expand Up @@ -302,6 +311,11 @@ export default function createRPCMethodTrackingMiddleware({
eventProperties.method = method;
}

const transformParams = TRANSFORM_PARAMS_MAP[method];
if (transformParams) {
eventProperties.params = transformParams(params);
}

trackEvent({
event,
category: MetaMetricsEventCategory.InpageProvider,
Expand Down
71 changes: 71 additions & 0 deletions app/scripts/lib/createRPCMethodTrackingMiddleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -561,5 +561,76 @@ describe('createRPCMethodTrackingMiddleware', () => {
});
});
});

it.each([
['no params', 'method_without_transform', {}, undefined],
[
'no params',
'eth_call',
[
{
accessList: [],
blobVersionedHashes: [],
blobs: [],
to: null,
},
null,
],
undefined,
],
['no params', 'eth_sandRawTransaction', ['0xdeadbeef'], undefined],
[
'no params',
'eth_sendTransaction',
{
to: '0x1',
from: '0x2',
gas: '0x3',
gasPrice: '0x4',
value: '0x5',
data: '0xdeadbeef',
maxPriorityFeePerGas: '0x6',
maxFeePerGas: '0x7',
},
undefined,
],
[
'only the type param',
'wallet_watchAsset',
{
type: 'ERC20',
options: {
address: '0xb60e8dd61c5d32be8058bb8eb970870f07233155',
symbol: 'FOO',
decimals: 18,
image: 'https://foo.io/token-image.svg',
},
},
{ type: 'ERC20' },
],
])(
`should include %s in the '%s' tracked events params property`,
async (_, method, params, expected) => {
const req = {
method,
origin: 'some.dapp',
params,
};

const res = {
error: null,
};

const { next } = getNext();
const handler = createHandler({ rateLimitSamplePercent: 1 });

await handler(req, res, next);

expect(trackEvent).toHaveBeenCalledTimes(1);
expect(trackEvent.mock.calls[0][0].properties.params).toStrictEqual(
expected,
);
},
);
});
});

0 comments on commit 7c36209

Please sign in to comment.