MetaMask / MetaMask/metamask-extension
MetaMetrics event fragment timeout check mixes milliseconds and seconds
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
I think the event fragment timeout check in MetaMetrics is using the wrong unit conversion.
Current code in `app/scripts/controllers/metametrics-controller.ts`:
```ts
Date.now() - fragment.lastUpdated / 1000 > fragment.timeout
```
But the fragment contract says:
- `timeout` is in seconds
- `lastUpdated` is `Date.now()`
From `shared/constants/metametrics.ts`:
```ts
/**
* Time in seconds the event should be persisted for.
*/
timeout?: number;
/**
* `Date.now()` when the fragment was last updated.
*/
lastUpdated?: number;
```
And `lastUpdated` is written with `Date.now()` when fragments are created/updated.
Because `/` has higher precedence than `-`, the current expression is evaluated as:
```ts
Date.now() - (fragment.lastUpdated / 1000)
```
not:
```ts
(Date.now() - fragment.lastUpdated) / 1000
```
So any fragment with both `timeout` and `lastUpdated` would become eligible for abandonment much earlier than intended.
I think the check should be one of these instead:
```ts
Date.now() - fragment.lastUpdated > fragment.timeout * 1000
```
or
```ts
(Date.now() - fragment.lastUpdated) / 1000 > fragment.timeout
```
A bit of history in case it helps:
- introduced with event fragments in commit `b820ef131b` (`Implement event fragments`, PR #12251)
- carried forward through later refactors/migrations including `29c2b136b8`, `317d2ef12e`, and `6ae02e8e8a`
I didn’t find a current in-tree caller that sets `timeout`, so this may be latent today rather than something widely hit in production, but the timeout path itself looks incorrect as implemented.
Contributor guide
Research direction
Read the timeout check in app/scripts/controllers/metametrics-controller.ts alongside the fragment contract in shared/constants/metametrics.ts. Confirm how timeout and lastUpdated units are represented, then verify that the abandonment check compares elapsed time using matching units. Done means the check no longer treats a millisecond timestamp as seconds and preserves the documented timeout behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- analytics
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100