-
-
Notifications
You must be signed in to change notification settings - Fork 267
Feat: new analytics privacy controller #7643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ad of dates - Changed `deleteRegulationDate` to `deleteRegulationTimestamp` in the state and related methods to store timestamps in milliseconds since epoch. - Updated relevant methods and tests to reflect the new timestamp format. - Removed date formatting logic and adjusted selectors accordingly. - Added new dependencies for testing and updated the test suite to ensure proper functionality with the new timestamp format. This change enhances consistency in handling date-related data within the analytics privacy controller.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
…e PascalCase - Refactored enum values in `DataDeleteResponseStatus` and `DataDeleteStatus` to follow PascalCase naming convention. - Updated all references in the codebase and tests to ensure consistency with the new enum values. - This change enhances code readability and aligns with common TypeScript practices.
…st title - Rename Error to Failure (and Ok to Success) for clearer naming - Fix duplicate test title in AnalyticsPrivacyController.test.ts - Update all references across the codebase
…tead of type-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new @metamask/analytics-privacy-controller package that provides GDPR/CCPA data deletion functionality for analytics data, extracting logic from the mobile app for reuse across MetaMask clients.
Changes:
- New controller package with
AnalyticsPrivacyControllerfor managing data deletion state (tracksdataRecorded,deleteRegulationId, anddeleteRegulationTimestamp) AnalyticsPrivacyServicefor communicating with Segment's Regulations API via proxy endpoint with retry/circuit-breaker logic- Comprehensive test coverage (100% branch, function, line, and statement coverage) and selector utilities for state access
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/analytics-privacy-controller/src/AnalyticsPrivacyController.ts | Main controller implementation managing deletion request lifecycle and state |
| packages/analytics-privacy-controller/src/AnalyticsPrivacyService.ts | Service for HTTP communication with Segment Regulations API via proxy |
| packages/analytics-privacy-controller/src/types.ts | Type definitions for deletion statuses and API responses |
| packages/analytics-privacy-controller/src/selectors.ts | Reusable state selectors |
| packages/analytics-privacy-controller/src/constants.ts | Segment API constants |
| packages/analytics-privacy-controller/package.json | Package configuration with dependencies |
| packages/analytics-privacy-controller/*.test.ts | Comprehensive test suites for controller and service |
| yarn.lock | Lockfile updates for new package dependencies |
| tsconfig.json, tsconfig.build.json | TypeScript project references |
| teams.json, .github/CODEOWNERS | Team ownership configuration |
| README.md | Documentation updates including package in list and dependency graph |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
# Conflicts: # yarn.lock
mcmire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @NicolasMassart, sorry for the late review, I had some questions/comments below.
packages/analytics-privacy-controller/src/AnalyticsPrivacyController.ts
Outdated
Show resolved
Hide resolved
packages/analytics-privacy-controller/src/AnalyticsPrivacyController.ts
Outdated
Show resolved
Hide resolved
|
|
||
| if ( | ||
| response.status === DataDeleteResponseStatus.Success && | ||
| response.regulateId && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be validating the response data. Is there a reason why we are doing that here? Ideally, the service should return data that is usable, and if not, should throw an error.
packages/analytics-privacy-controller/src/AnalyticsPrivacyService.ts
Outdated
Show resolved
Hide resolved
packages/analytics-privacy-controller/src/AnalyticsPrivacyService.ts
Outdated
Show resolved
Hide resolved
… data recording handling
…sController and streamline analytics ID handling - Eliminated the dependency on `@metamask/analytics-controller` from the `AnalyticsPrivacyController` and its tests. - Updated the `AnalyticsPrivacyController` to accept an `analyticsId` directly, improving clarity and reducing coupling. - Adjusted tests to reflect the removal of the `AnalyticsController:getState` action, ensuring they now validate the presence of a valid `analyticsId`. - Cleaned up related code to enhance maintainability and readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
|
|
||
| if (!this.state.deleteRegulationId) { | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return ignores actual hasCollectedDataSinceDeletionRequest state value
Medium Severity
The checkDataDeleteStatus method initializes hasCollectedDataSinceDeletionRequest to false in the status object and returns early when deleteRegulationId is null, without reading the actual state value. This means if data has been collected (via updateDataRecordingFlag(true)) but no deletion request has been made yet, the method incorrectly reports hasCollectedDataSinceDeletionRequest: false instead of true. For GDPR/CCPA compliance tracking, this could lead users to incorrectly believe no data has been collected.
Explanation
This PR introduces a new
@metamask/analytics-privacy-controllerpackage that provides GDPR/CCPA data deletion functionality for analytics data. The package allows to extract the logic from the mobile app (and will be compatible with extension too)Current state: MetaMask mobile app currently has a dedicated mechanism to handle user data deletion requests for analytics data in compliance with GDPR and CCPA regulations.
Solution: This package introduces:
dataRecordedflag,deleteRegulationId, anddeleteRegulationTimestampto support compliance workflowsImplementation details:
AnalyticsController:getStateto retrieve the user's analytics IDAnalyticsPrivacyServiceto make HTTP requests to Segment's Regulations APIcreateServicePolicyfrom@metamask/controller-utilsfor retry logic and error handlingReferences
see also MetaMask/metamask-mobile#22016
Fixes #7618
Checklist
Note
Introduces
@metamask/analytics-privacy-controlleras a new package to manage analytics data deletion workflows.AnalyticsPrivacyControllerwith actions to create/check deletion tasks, expose timestamps/IDs, and trackhasCollectedDataSinceDeletionRequest; publishes related events and selectorsAnalyticsPrivacyServiceto call a proxy to Segment Regulations API (POST create, GET status) with retry/circuit-breaker policy via@metamask/controller-utilsREADME,CODEOWNERS,teams.json, tsconfig, yarn.lock)Written by Cursor Bugbot for commit 6dd80c0. This will update automatically on new commits. Configure here.