From b256fcaceab6ea21387dd761847d0976ee5729c9 Mon Sep 17 00:00:00 2001 From: sophieqgu <37032128+sophieqgu@users.noreply.github.com> Date: Wed, 25 Mar 2026 03:27:58 -0400 Subject: [PATCH 1/8] chore(rewards): add image and remove status label (#27890) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Use updated image field from CampaignDto to render campaign tile background. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. --- > [!NOTE] > **Medium Risk** > Medium risk because it changes the `CampaignDto` shape (removing `statusLabel`, moving `details.image` to top-level `image`) and updates UI rendering to depend on the new field, which could break if upstream data isn’t migrated consistently. > > **Overview** > Rewards campaign data is updated to **remove `statusLabel`** and introduce an optional top-level **`image`** (theme-aware URLs), with `details.image` removed from the type/state shapes. > > UI components (`CampaignTile`, `CampaignStatus`) now render their background images from `campaign.image` instead of `campaign.details.image`, and tests across rewards views/controller/reducer are updated to match the new DTO shape. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5cf9e9feedfcbaa3c312e438a29ec41e66632b96. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../Views/CampaignMechanicsView.test.tsx | 25 ------------------ .../UI/Rewards/Views/CampaignsView.test.tsx | 1 - .../Views/OndoCampaignDetailsView.test.tsx | 5 ---- .../SeasonOneCampaignDetailsView.test.tsx | 1 - .../Campaigns/CampaignOptInSheet.test.tsx | 1 - .../Campaigns/CampaignStatus.test.tsx | 26 +++---------------- .../components/Campaigns/CampaignStatus.tsx | 4 +-- .../Campaigns/CampaignTile.test.tsx | 14 +++------- .../components/Campaigns/CampaignTile.tsx | 4 +-- .../Campaigns/CampaignTile.utils.test.ts | 1 - .../Campaigns/CampaignsGroup.test.tsx | 1 - .../Campaigns/CampaignsPreview.test.tsx | 1 - .../Rewards/hooks/useRewardCampaigns.test.ts | 1 - .../RewardsController.test.ts | 2 -- .../services/rewards-data-service.test.ts | 1 - .../controllers/rewards-controller/types.ts | 17 +++++------- app/reducers/rewards/index.test.ts | 1 - app/reducers/rewards/selectors.test.ts | 1 - 18 files changed, 17 insertions(+), 90 deletions(-) diff --git a/app/components/UI/Rewards/Views/CampaignMechanicsView.test.tsx b/app/components/UI/Rewards/Views/CampaignMechanicsView.test.tsx index 2af0dd41c57..68cded2bef7 100644 --- a/app/components/UI/Rewards/Views/CampaignMechanicsView.test.tsx +++ b/app/components/UI/Rewards/Views/CampaignMechanicsView.test.tsx @@ -140,7 +140,6 @@ const createTestCampaign = ( endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, @@ -185,10 +184,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', @@ -246,10 +241,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', @@ -272,10 +263,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', @@ -298,10 +285,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', @@ -323,10 +306,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', @@ -349,10 +328,6 @@ describe('CampaignMechanicsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Earn rewards', diff --git a/app/components/UI/Rewards/Views/CampaignsView.test.tsx b/app/components/UI/Rewards/Views/CampaignsView.test.tsx index 625c5c7360b..6e37c8fa434 100644 --- a/app/components/UI/Rewards/Views/CampaignsView.test.tsx +++ b/app/components/UI/Rewards/Views/CampaignsView.test.tsx @@ -143,7 +143,6 @@ const createTestCampaign = ( endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/Views/OndoCampaignDetailsView.test.tsx b/app/components/UI/Rewards/Views/OndoCampaignDetailsView.test.tsx index 101a99d9f74..7d4fcd088e2 100644 --- a/app/components/UI/Rewards/Views/OndoCampaignDetailsView.test.tsx +++ b/app/components/UI/Rewards/Views/OndoCampaignDetailsView.test.tsx @@ -206,7 +206,6 @@ const createTestCampaign = ( endDate: nextMonth.toISOString(), termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, @@ -317,10 +316,6 @@ describe('OndoCampaignDetailsView', () => { campaigns: [ createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Description', diff --git a/app/components/UI/Rewards/Views/SeasonOneCampaignDetailsView.test.tsx b/app/components/UI/Rewards/Views/SeasonOneCampaignDetailsView.test.tsx index b2be68e4816..5714e553e60 100644 --- a/app/components/UI/Rewards/Views/SeasonOneCampaignDetailsView.test.tsx +++ b/app/components/UI/Rewards/Views/SeasonOneCampaignDetailsView.test.tsx @@ -148,7 +148,6 @@ const createTestCampaign = ( endDate: nextMonth.toISOString(), termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignOptInSheet.test.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignOptInSheet.test.tsx index 4b30daf03c9..c5a5f1d241a 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignOptInSheet.test.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignOptInSheet.test.tsx @@ -165,7 +165,6 @@ const createTestCampaign = ( endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignStatus.test.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignStatus.test.tsx index e779437e910..65bf77634df 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignStatus.test.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignStatus.test.tsx @@ -33,7 +33,6 @@ const createTestCampaign = (overrides = {}): CampaignDto => ({ endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, @@ -58,12 +57,9 @@ describe('CampaignStatus', () => { it('renders campaign image', () => { const campaign = createTestCampaign({ - details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, - howItWorks: { title: '', description: '', phases: [] }, + image: { + lightModeUrl: 'https://example.com/light.png', + darkModeUrl: 'https://example.com/dark.png', }, }); const { getByTestId } = render(); @@ -89,10 +85,6 @@ describe('CampaignStatus', () => { it('renders howItWorks title when available', () => { const campaign = createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Description', @@ -117,10 +109,6 @@ describe('CampaignStatus', () => { it('does not render howItWorks title when title is empty', () => { const campaign = createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: '', description: '', phases: [] }, }, }); @@ -133,10 +121,6 @@ describe('CampaignStatus', () => { it('renders howItWorks description when available', () => { const campaign = createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'How it works', description: 'Hold ONDO tokens to earn rewards', @@ -161,10 +145,6 @@ describe('CampaignStatus', () => { it('does not render howItWorks description when description is empty', () => { const campaign = createTestCampaign({ details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, howItWorks: { title: 'Title', description: '', phases: [] }, }, }); diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignStatus.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignStatus.tsx index 2ca2e5ac7c9..b1fdeaa75af 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignStatus.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignStatus.tsx @@ -37,8 +37,8 @@ const CampaignStatus: React.FC = ({ campaign }) => { const backgroundImageUrl = colorScheme === 'dark' - ? campaign.details?.image?.darkModeUrl - : campaign.details?.image?.lightModeUrl; + ? campaign.image?.darkModeUrl + : campaign.image?.lightModeUrl; const howItWorksTitle = campaign.details?.howItWorks?.title; const howItWorksDescription = campaign.details?.howItWorks?.description; diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignTile.test.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignTile.test.tsx index 2b23a5b5e41..541d9db5cc3 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignTile.test.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignTile.test.tsx @@ -88,7 +88,6 @@ const createTestCampaign = (overrides = {}): CampaignDto => ({ endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, @@ -161,16 +160,9 @@ describe('CampaignTile', () => { it('renders background image via campaign-tile-background testID', () => { const campaign = createTestCampaign({ - details: { - image: { - lightModeUrl: 'https://example.com/light.png', - darkModeUrl: 'https://example.com/dark.png', - }, - howItWorks: { - title: '', - description: '', - phases: [], - }, + image: { + lightModeUrl: 'https://example.com/light.png', + darkModeUrl: 'https://example.com/dark.png', }, }); diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignTile.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignTile.tsx index 4950e938f80..79fd04a03a1 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignTile.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignTile.tsx @@ -74,8 +74,8 @@ const CampaignTile: React.FC = ({ campaign, onPress }) => { const backgroundImageUrl = colorScheme === 'dark' - ? campaign.details?.image?.darkModeUrl - : campaign.details?.image?.lightModeUrl; + ? campaign.image?.darkModeUrl + : campaign.image?.lightModeUrl; const handlePress = () => { if (!isInteractive) return; diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignTile.utils.test.ts b/app/components/UI/Rewards/components/Campaigns/CampaignTile.utils.test.ts index 663855acaaa..abec4ec03e0 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignTile.utils.test.ts +++ b/app/components/UI/Rewards/components/Campaigns/CampaignTile.utils.test.ts @@ -42,7 +42,6 @@ function buildCampaignDto(overrides: Partial = {}): CampaignDto { endDate: '2025-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignsGroup.test.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignsGroup.test.tsx index 267f1c86f0a..235f30603d8 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignsGroup.test.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignsGroup.test.tsx @@ -16,7 +16,6 @@ const createTestCampaign = ( endDate: '2027-12-31T23:59:59.999Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/components/Campaigns/CampaignsPreview.test.tsx b/app/components/UI/Rewards/components/Campaigns/CampaignsPreview.test.tsx index 8adae8d1a65..dc1eb911605 100644 --- a/app/components/UI/Rewards/components/Campaigns/CampaignsPreview.test.tsx +++ b/app/components/UI/Rewards/components/Campaigns/CampaignsPreview.test.tsx @@ -80,7 +80,6 @@ const createTestCampaign = ( endDate: futureDate.toISOString(), termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/components/UI/Rewards/hooks/useRewardCampaigns.test.ts b/app/components/UI/Rewards/hooks/useRewardCampaigns.test.ts index 5cc5d2fe0cf..7f9ea8bc1b5 100644 --- a/app/components/UI/Rewards/hooks/useRewardCampaigns.test.ts +++ b/app/components/UI/Rewards/hooks/useRewardCampaigns.test.ts @@ -81,7 +81,6 @@ const createTestCampaign = ( endDate: '2027-01-01T00:00:00.000Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: true, ...overrides, diff --git a/app/core/Engine/controllers/rewards-controller/RewardsController.test.ts b/app/core/Engine/controllers/rewards-controller/RewardsController.test.ts index 1a38bc0a022..d8614930c54 100644 --- a/app/core/Engine/controllers/rewards-controller/RewardsController.test.ts +++ b/app/core/Engine/controllers/rewards-controller/RewardsController.test.ts @@ -18610,7 +18610,6 @@ describe('RewardsController', () => { endDate: string; termsAndConditions: Json | null; excludedRegions: string[]; - statusLabel: string; details: null; featured: boolean; }> = {}, @@ -18622,7 +18621,6 @@ describe('RewardsController', () => { endDate: '2027-01-01T00:00:00.000Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: false, ...overrides, diff --git a/app/core/Engine/controllers/rewards-controller/services/rewards-data-service.test.ts b/app/core/Engine/controllers/rewards-controller/services/rewards-data-service.test.ts index 81ee5e309d5..7ae9f273a0e 100644 --- a/app/core/Engine/controllers/rewards-controller/services/rewards-data-service.test.ts +++ b/app/core/Engine/controllers/rewards-controller/services/rewards-data-service.test.ts @@ -4321,7 +4321,6 @@ describe('RewardsDataService', () => { endDate: '2027-01-01T00:00:00.000Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: false, }, diff --git a/app/core/Engine/controllers/rewards-controller/types.ts b/app/core/Engine/controllers/rewards-controller/types.ts index 657f2681137..680aadc81d4 100644 --- a/app/core/Engine/controllers/rewards-controller/types.ts +++ b/app/core/Engine/controllers/rewards-controller/types.ts @@ -141,14 +141,13 @@ export interface CampaignDto { excludedRegions: string[]; /** - * Status label for the campaign - * @example 'Active' + * Theme-aware background image for the campaign tile */ - statusLabel: string; + image?: ThemeImage; /** * The details of the campaign - * @example { image: { lightModeUrl: 'https://example.com/image.png', darkModeUrl: 'https://example.com/image-dark.png' }, howItWorks: { title: 'How it works', description: 'How it works', phases: [{ name: 'Phase 1', daysLabel: 'Days', sortOrder: 1, steps: [{ title: 'Step 1', description: 'Step 1', iconName: 'icon-name' }] }] } } + * @example { howItWorks: { title: 'How it works', description: 'How it works', phases: [{ name: 'Phase 1', daysLabel: 'Days', sortOrder: 1, steps: [{ title: 'Step 1', description: 'Step 1', iconName: 'icon-name' }] }] } } */ details: CampaignDetails | null; @@ -169,12 +168,11 @@ export type CampaignsState = { endDate: string; termsAndConditions: Json | null; excludedRegions: string[]; - statusLabel: string; + image?: { + lightModeUrl: string; + darkModeUrl: string; + }; details: { - image: { - lightModeUrl: string; - darkModeUrl: string; - }; howItWorks: { title: string; description: string; @@ -243,7 +241,6 @@ export interface OndoCampaignHowItWorks { } export interface OndoHoldingDetails { - image: ThemeImage; howItWorks: OndoCampaignHowItWorks; } diff --git a/app/reducers/rewards/index.test.ts b/app/reducers/rewards/index.test.ts index 22721d37c1d..17b6301fa77 100644 --- a/app/reducers/rewards/index.test.ts +++ b/app/reducers/rewards/index.test.ts @@ -4471,7 +4471,6 @@ const mockCampaign: CampaignDto = { endDate: '2027-01-01T00:00:00.000Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: false, }; diff --git a/app/reducers/rewards/selectors.test.ts b/app/reducers/rewards/selectors.test.ts index 4d636857ba7..9e870f116a8 100644 --- a/app/reducers/rewards/selectors.test.ts +++ b/app/reducers/rewards/selectors.test.ts @@ -3139,7 +3139,6 @@ describe('Rewards selectors', () => { endDate: '2027-01-01T00:00:00.000Z', termsAndConditions: null, excludedRegions: [], - statusLabel: 'Active', details: null, featured: false, }; From 72c0a94b2dd0ddd42efdb5be25980c904d552645 Mon Sep 17 00:00:00 2001 From: Ganesh Suresh Patra Date: Wed, 25 Mar 2026 13:01:44 +0530 Subject: [PATCH 2/8] =?UTF-8?q?refactor:=20migrate=20reveal-srp-ui=20to=20?= =?UTF-8?q?design=20system=20components=20and=20Tailw=E2=80=A6=20(#27873)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …ind CSS ## **Description** This PR migrates the ChoosePassword view and its FoxRiveLoaderAnimation sub-component away from legacy StyleSheet.create()-based styling toward the MetaMask design system and Tailwind CSS. Jira Link: https://consensyssoftware.atlassian.net/browse/TO-636 ## **Changelog** CHANGELOG entry: migrate reveal-srp-ui to design system components and Tailwind CSS ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** ### **Before** ### **After** https://github.com/user-attachments/assets/719aeac1-bb97-424f-af35-7cba9cf84d50 https://github.com/user-attachments/assets/e4e16264-12ed-40be-9d0c-66947522b91e ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. --- > [!NOTE] > **Medium Risk** > Primarily a UI/styling refactor, but it touches Secret Recovery Phrase reveal/quiz screens where regressions could impact critical security UX and automated flows (Detox selectors/accessibility). > > **Overview** > Migrates `RevealPrivateCredential` (SRP quiz, password entry, SRP text/QR tabs, seed phrase concealer/display) from legacy component-library buttons/text + `StyleSheet.create()` to **design-system React Native components** with **Tailwind (`twClassName`/`tw.style`)** and deletes `RevealPrivateCredential/styles.ts` plus the `styles` prop wiring in related components/types. > > Reuses the shared `SeedPhraseConcealer` in `ManualBackupStep1` (replacing an inline `ImageBackground` blur implementation) and updates the snapshot accordingly. > > Improves testability by adding an `accessibilityLabel` to the password field and updating the Detox page object to locate it via label instead of testID. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit d5fa10ca20eb5bb59fc53a3c21b23cac99992f33. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../__snapshots__/index.test.tsx.snap | 39 ++-- .../Views/ManualBackupStep1/index.tsx | 55 +----- .../RevealPrivateCredential.tsx | 14 +- .../components/PasswordEntry.tsx | 29 ++- .../components/SRPQuizIntroduction.tsx | 121 ++++++------ .../components/SRPSecurityQuiz.tsx | 184 ++++++++++-------- .../components/SRPTabView.tsx | 38 ++-- .../components/SeedPhraseConcealer.tsx | 38 ++-- .../components/SeedPhraseDisplay.tsx | 85 ++++---- .../Views/RevealPrivateCredential/styles.ts | 177 ----------------- .../Views/RevealPrivateCredential/types.ts | 10 +- .../RevealSecretRecoveryPhrase.ts | 2 +- 12 files changed, 312 insertions(+), 480 deletions(-) delete mode 100644 app/components/Views/RevealPrivateCredential/styles.ts diff --git a/app/components/Views/ManualBackupStep1/__snapshots__/index.test.tsx.snap b/app/components/Views/ManualBackupStep1/__snapshots__/index.test.tsx.snap index 8844e0c4c6e..fba420e36c6 100644 --- a/app/components/Views/ManualBackupStep1/__snapshots__/index.test.tsx.snap +++ b/app/components/Views/ManualBackupStep1/__snapshots__/index.test.tsx.snap @@ -331,7 +331,8 @@ exports[`ManualBackupStep1 matches snapshot 1`] = ` accessibilityIgnoresInvertColors={true} style={ [ - "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1 opacity-50", + "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1", + "opacity-50", ] } > @@ -361,11 +362,12 @@ exports[`ManualBackupStep1 matches snapshot 1`] = ` [ [ "flex", - "flex-col", + undefined, + undefined, + false, + undefined, undefined, false, - "items-center", - "justify-center", false, false, false, @@ -378,12 +380,11 @@ exports[`ManualBackupStep1 matches snapshot 1`] = ` false, false, false, - "px-6", false, false, undefined, undefined, - "rounded-lg py-[45px] gap-y-4 h-full flex-1", + "items-center justify-center rounded-lg px-6 py-[45px] gap-y-4 h-full flex-1", ], undefined, ] @@ -885,7 +886,8 @@ exports[`ManualBackupStep1 theme appearance renders with dark theme 1`] = ` accessibilityIgnoresInvertColors={true} style={ [ - "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1 opacity-50", + "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1", + "opacity-50", ] } > @@ -915,11 +917,12 @@ exports[`ManualBackupStep1 theme appearance renders with dark theme 1`] = ` [ [ "flex", - "flex-col", + undefined, + undefined, + false, + undefined, undefined, false, - "items-center", - "justify-center", false, false, false, @@ -932,12 +935,11 @@ exports[`ManualBackupStep1 theme appearance renders with dark theme 1`] = ` false, false, false, - "px-6", false, false, undefined, undefined, - "rounded-lg py-[45px] gap-y-4 h-full flex-1", + "items-center justify-center rounded-lg px-6 py-[45px] gap-y-4 h-full flex-1", ], undefined, ] @@ -1439,7 +1441,8 @@ exports[`ManualBackupStep1 theme appearance renders with light theme on Android accessibilityIgnoresInvertColors={true} style={ [ - "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1 opacity-50", + "absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1", + "opacity-50", ] } > @@ -1469,11 +1472,12 @@ exports[`ManualBackupStep1 theme appearance renders with light theme on Android [ [ "flex", - "flex-col", + undefined, + undefined, + false, + undefined, undefined, false, - "items-center", - "justify-center", false, false, false, @@ -1486,12 +1490,11 @@ exports[`ManualBackupStep1 theme appearance renders with light theme on Android false, false, false, - "px-6", false, false, undefined, undefined, - "rounded-lg py-[45px] gap-y-4 h-full flex-1", + "items-center justify-center rounded-lg px-6 py-[45px] gap-y-4 h-full flex-1", ], undefined, ] diff --git a/app/components/Views/ManualBackupStep1/index.tsx b/app/components/Views/ManualBackupStep1/index.tsx index 5282d7af51d..d96ae6530a9 100644 --- a/app/components/Views/ManualBackupStep1/index.tsx +++ b/app/components/Views/ManualBackupStep1/index.tsx @@ -10,7 +10,6 @@ import { KeyboardAvoidingView, FlatList, TouchableOpacity, - ImageBackground, Platform, } from 'react-native'; import { SafeAreaView } from 'react-native-safe-area-context'; @@ -37,7 +36,6 @@ import { TextButton, TextButtonSize, BoxAlignItems, - BoxFlexDirection, BoxJustifyContent, } from '@metamask/design-system-react-native'; import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; @@ -58,8 +56,8 @@ import { MetaMetricsEvents } from '../../../core/Analytics'; import type { ITrackingEvent } from '../../../core/Analytics/MetaMetrics.types'; import { Authentication } from '../../../core'; import { ManualBackUpStepsSelectorsIDs } from './ManualBackUpSteps.testIds'; +import SeedPhraseConcealer from '../RevealPrivateCredential/components/SeedPhraseConcealer'; import { saveOnboardingEvent as saveEvent } from '../../../actions/onboarding'; -import { AppThemeKey } from '../../../util/theme/models'; import { useAnalytics } from '../../hooks/useAnalytics/useAnalytics'; import { createTrackFunction, @@ -67,9 +65,6 @@ import { showSeedphraseDefinition, } from '../../../util/onboarding/backupUtils'; import type { ManualBackupStep1RouteProp } from './ManualBackupStep1.types'; -import darkBlurImage from '../../../images/dark-blur.png'; -import lightBlurImage from '../../../images/blur.png'; - /** * View that's shown during the second step of * the backup seed phrase flow @@ -298,50 +293,10 @@ const ManualBackupStep1 = () => { }; const renderSeedPhraseConcealer = () => ( - - - - - - - {strings('manual_backup_step_1.reveal')} - - - {strings('manual_backup_step_1.watching')} - - - - + ); const renderConfirmPassword = () => ( diff --git a/app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx b/app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx index d7a4559d0a5..ab03dc24c88 100644 --- a/app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx +++ b/app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx @@ -18,13 +18,11 @@ import ActionView from '../../UI/ActionView'; import { ScreenshotDeterrent } from '../../UI/ScreenshotDeterrent'; import { SRP_GUIDE_URL } from '../../../constants/urls'; import ClipboardManager from '../../../core/ClipboardManager'; -import { useTheme } from '../../../util/theme'; import { MetaMetricsEvents } from '../../../core/Analytics/MetaMetrics.events'; import { passwordRequirementsMet } from '../../../util/password'; import Device from '../../../util/device'; import { strings } from '../../../../locales/i18n'; import AppConstants from '../../../core/AppConstants'; -import { createStyles } from './styles'; import { RevealSeedViewSelectorsIDs } from './RevealSeedView.testIds'; import { selectSelectedInternalAccountFormattedAddress } from '../../../selectors/accountsController'; import { useAnalytics } from '../../../components/hooks/useAnalytics/useAnalytics'; @@ -48,6 +46,7 @@ import { import { useRevealCredential, useSRPQuiz } from './hooks'; import { IRevealPrivateCredentialProps, RevealSrpStage } from './types'; import HeaderCompactStandard from '../../../component-library/components-temp/HeaderCompactStandard'; +import { useTailwind } from '@metamask/design-system-twrnc-preset'; const RevealPrivateCredential = ({ navigation, @@ -68,11 +67,8 @@ const RevealPrivateCredential = ({ const checkSummedAddress = useSelector( selectSelectedInternalAccountFormattedAddress, ); - - const theme = useTheme(); const { trackEvent, createEventBuilder } = useAnalytics(); - const { colors } = theme; - const styles = createStyles(theme, colors); + const tw = useTailwind(); const selectedAddress = route?.params?.selectedAccount?.address || checkSummedAddress; @@ -281,7 +277,7 @@ const RevealPrivateCredential = ({ {strings('reveal_credential.seed_phrase_warning_explanation')} } - style={styles.warningWrapper} + style={tw.style('text-body-sm mt-6')} /> ); @@ -335,7 +331,6 @@ const RevealPrivateCredential = ({ onRevealSeedPhrase={() => setShowSeedPhrase(!showSeedPhrase)} onCopyToClipboard={copyPrivateCredentialToClipboard} onTabChange={onTabBarChange} - styles={styles} /> ) : ( @@ -345,7 +340,6 @@ const RevealPrivateCredential = ({ warningMessage={warningIncorrectPassword} showPassword={showPassword} onToggleShowPassword={() => setShowPassword(!showPassword)} - styles={styles} /> )} @@ -359,7 +353,6 @@ const RevealPrivateCredential = ({ ); } @@ -372,7 +365,6 @@ const RevealPrivateCredential = ({ onAnswerClick={handleQuestionAnswerClick} onContinueClick={handleAnsweredQuestionClick} onLearnMore={handleLearnMoreClick} - styles={styles} /> ); } diff --git a/app/components/Views/RevealPrivateCredential/components/PasswordEntry.tsx b/app/components/Views/RevealPrivateCredential/components/PasswordEntry.tsx index 5852f054a72..8b5b6f1486f 100644 --- a/app/components/Views/RevealPrivateCredential/components/PasswordEntry.tsx +++ b/app/components/Views/RevealPrivateCredential/components/PasswordEntry.tsx @@ -1,13 +1,19 @@ import React from 'react'; -import { ButtonIcon, IconName } from '@metamask/design-system-react-native'; -import Text, { +import { + TextField, + IconName, TextVariant, -} from '../../../../component-library/components/Texts/Text'; -import TextField from '../../../../component-library/components/Form/TextField/TextField'; + Text, + TextFieldSize, + FontWeight, + ButtonIcon, + TextColor, +} from '@metamask/design-system-react-native'; import { strings } from '../../../../../locales/i18n'; import { RevealSeedViewSelectorsIDs } from '../RevealSeedView.testIds'; import { useTheme } from '../../../../util/theme'; import { PasswordEntryProps } from '../types'; +import { useTailwind } from '@metamask/design-system-twrnc-preset'; const PasswordEntry = ({ onPasswordChange, @@ -15,13 +21,18 @@ const PasswordEntry = ({ warningMessage, showPassword, onToggleShowPassword, - styles, }: PasswordEntryProps) => { + const tw = useTailwind(); const { colors, themeAppearance } = useTheme(); return ( <> - + {strings('reveal_credential.enter_password')} } + size={TextFieldSize.Lg} /> {warningMessage} diff --git a/app/components/Views/RevealPrivateCredential/components/SRPQuizIntroduction.tsx b/app/components/Views/RevealPrivateCredential/components/SRPQuizIntroduction.tsx index e73d7cd0298..4a4ae529c8b 100644 --- a/app/components/Views/RevealPrivateCredential/components/SRPQuizIntroduction.tsx +++ b/app/components/Views/RevealPrivateCredential/components/SRPQuizIntroduction.tsx @@ -1,74 +1,85 @@ import React from 'react'; import { Image } from 'react-native'; -import { ButtonSize } from '../../../../component-library/components/Buttons/Button'; -import ButtonPrimary from '../../../../component-library/components/Buttons/Button/variants/ButtonPrimary'; -import ButtonLink from '../../../../component-library/components/Buttons/Button/variants/ButtonLink'; -import Text, { +import { + Box, + Text, TextColor, TextVariant, -} from '../../../../component-library/components/Texts/Text'; -import { Box } from '../../../UI/Box/Box'; -import { - AlignItems, - FlexDirection, - JustifyContent, -} from '../../../UI/Box/box.types'; + Button, + ButtonSize, + ButtonVariant, + BoxFlexDirection, + BoxAlignItems, + BoxJustifyContent, + TextButton, +} from '@metamask/design-system-react-native'; import SecurityQuizLockImage from '../../../../images/reveal_srp_intro.png'; import { strings } from '../../../../../locales/i18n'; import { ExportCredentialsIds } from '../../MultichainAccounts/AccountDetails/ExportCredentials.testIds'; import { SrpQuizGetStartedSelectorsIDs } from '../../Quiz/SRPQuiz/SrpQuizModal.testIds'; import { SRPQuizIntroductionProps } from '../types'; +import { useTailwind } from '@metamask/design-system-twrnc-preset'; const SRPQuizIntroduction = ({ onGetStarted, onLearnMore, - styles, -}: SRPQuizIntroductionProps) => ( - +}: SRPQuizIntroductionProps) => { + const tw = useTailwind(); + + return ( - - - {strings('multichain_accounts.reveal_srp.description')} - - - - - + + + {strings('multichain_accounts.reveal_srp.description')} + + + + + + {strings('multichain_accounts.reveal_srp.learn_more')} + + - -); + ); +}; export default SRPQuizIntroduction; diff --git a/app/components/Views/RevealPrivateCredential/components/SRPSecurityQuiz.tsx b/app/components/Views/RevealPrivateCredential/components/SRPSecurityQuiz.tsx index 0f9aed1d69f..ecb487882dd 100644 --- a/app/components/Views/RevealPrivateCredential/components/SRPSecurityQuiz.tsx +++ b/app/components/Views/RevealPrivateCredential/components/SRPSecurityQuiz.tsx @@ -1,23 +1,20 @@ import React from 'react'; -import { ButtonSize } from '../../../../component-library/components/Buttons/Button'; -import ButtonPrimary from '../../../../component-library/components/Buttons/Button/variants/ButtonPrimary'; -import ButtonSecondary from '../../../../component-library/components/Buttons/Button/variants/ButtonSecondary'; -import ButtonLink from '../../../../component-library/components/Buttons/Button/variants/ButtonLink'; -import Text, { +import { + Box, + Text, TextColor, TextVariant, -} from '../../../../component-library/components/Texts/Text'; -import { Box } from '../../../UI/Box/Box'; -import { - AlignItems, - FlexDirection, - JustifyContent, -} from '../../../UI/Box/box.types'; -import { + Button, + ButtonSize, + ButtonVariant, + IconName, + BoxFlexDirection, + BoxAlignItems, + BoxJustifyContent, Icon, IconColor, - IconName, IconSize, + TextButton, } from '@metamask/design-system-react-native'; import { strings } from '../../../../../locales/i18n'; import { ExportCredentialsIds } from '../../MultichainAccounts/AccountDetails/ExportCredentials.testIds'; @@ -34,20 +31,19 @@ const SRPSecurityQuiz = ({ onAnswerClick, onContinueClick, onLearnMore, - styles, }: SRPSecurityQuizProps) => { const renderQuestionResult = () => ( {strings( correctAnswer @@ -69,19 +67,19 @@ const SRPSecurityQuiz = ({ {currentQuestionIndex === 1 && ( - + {strings( correctAnswer ? 'srp_security_quiz.question_one.right_answer_title' : 'srp_security_quiz.question_one.wrong_answer_title', )} - + {strings( correctAnswer ? 'srp_security_quiz.question_one.right_answer_description' @@ -92,19 +90,19 @@ const SRPSecurityQuiz = ({ )} {currentQuestionIndex === 2 && ( - + {strings( correctAnswer ? 'srp_security_quiz.question_two.right_answer_title' : 'srp_security_quiz.question_two.wrong_answer_title', )} - + {strings( correctAnswer ? 'srp_security_quiz.question_two.right_answer_description' @@ -118,64 +116,68 @@ const SRPSecurityQuiz = ({ const renderAnswerButtons = () => ( - onAnswerClick(1)} size={ButtonSize.Lg} - label={strings( - currentQuestionIndex === 1 - ? 'srp_security_quiz.question_one.wrong_answer' - : 'srp_security_quiz.question_two.right_answer', - )} testID={ currentQuestionIndex === 1 ? SrpSecurityQuestionOneSelectorsIDs.WRONG_ANSWER : SrpSecurityQuestionTwoSelectorsIDs.RIGHT_ANSWER } - style={styles.button} - /> - onAnswerClick(2)} - size={ButtonSize.Lg} - label={strings( + twClassName="w-full text-center" + > + {strings( currentQuestionIndex === 1 - ? 'srp_security_quiz.question_one.right_answer' - : 'srp_security_quiz.question_two.wrong_answer', + ? 'srp_security_quiz.question_one.wrong_answer' + : 'srp_security_quiz.question_two.right_answer', )} + + + + twClassName="w-full text-center flex items-center justify-center" + > + {strings('multichain_accounts.reveal_srp.learn_more')} + ); const renderAnsweredButtons = () => ( - - + {strings( + correctAnswer + ? 'srp_security_quiz.continue' + : 'srp_security_quiz.try_again', + )} + + + twClassName="w-full text-center flex items-center justify-center" + > + {strings('multichain_accounts.reveal_srp.learn_more')} + ); return ( {strings('srp_security_quiz.question_step', { step: currentQuestionIndex, @@ -218,9 +230,9 @@ const SRPSecurityQuiz = ({ {!questionAnswered && ( {strings( currentQuestionIndex === 1 diff --git a/app/components/Views/RevealPrivateCredential/components/SRPTabView.tsx b/app/components/Views/RevealPrivateCredential/components/SRPTabView.tsx index efe15d918c9..66d79a7b2e9 100644 --- a/app/components/Views/RevealPrivateCredential/components/SRPTabView.tsx +++ b/app/components/Views/RevealPrivateCredential/components/SRPTabView.tsx @@ -1,5 +1,11 @@ import React from 'react'; -import { Dimensions, ScrollView, View } from 'react-native'; +import { Dimensions, ScrollView, Platform } from 'react-native'; +import { + Box, + BoxJustifyContent, + BoxAlignItems, + BoxFlexDirection, +} from '@metamask/design-system-react-native'; import ScrollableTabView from '@tommasini/react-native-scrollable-tab-view'; import QRCode from 'react-native-qrcode-svg'; import TabBar from '../../../../component-library/components-temp/TabBar/TabBar'; @@ -10,6 +16,7 @@ import logo from '../../../../images/branding/fox.png'; import SeedPhraseDisplay from './SeedPhraseDisplay'; import SeedPhraseConcealer from './SeedPhraseConcealer'; import { SRPTabViewProps } from '../types'; +import { useTailwind } from '@metamask/design-system-twrnc-preset'; // eslint-disable-next-line @typescript-eslint/no-explicit-any const CustomTabView = ScrollView as any; @@ -21,7 +28,6 @@ const SRPTabView = ({ onRevealSeedPhrase, onCopyToClipboard, onTabChange, - styles, }: SRPTabViewProps) => { const { colors } = useTheme(); const trimmedCredential = clipboardPrivateCredential.trim(); @@ -29,42 +35,44 @@ const SRPTabView = ({ const hasCredential = words.length > 0; const renderTabBar = () => ; + const tw = useTailwind(); return ( - + renderTabBar()} // eslint-disable-next-line @typescript-eslint/no-explicit-any onChangeTab={(event: any) => onTabChange(event)} - style={styles.tabContentContainer} + style={tw.style( + `min-h-[${Platform.OS === 'android' ? 320 : 0}px] flex-grow flex-shrink-0 mb-[${Platform.OS === 'android' ? 20 : 0}px]`, + )} > - + {showSeedPhrase ? ( ) : ( - + )} - + - ) : null} - + - + ); }; diff --git a/app/components/Views/RevealPrivateCredential/components/SeedPhraseConcealer.tsx b/app/components/Views/RevealPrivateCredential/components/SeedPhraseConcealer.tsx index 51a5b832686..e22a707c9f8 100644 --- a/app/components/Views/RevealPrivateCredential/components/SeedPhraseConcealer.tsx +++ b/app/components/Views/RevealPrivateCredential/components/SeedPhraseConcealer.tsx @@ -1,15 +1,16 @@ import React from 'react'; -import { ImageBackground, TouchableOpacity, View } from 'react-native'; +import { ImageBackground, TouchableOpacity } from 'react-native'; import { Icon, IconColor, IconName, IconSize, -} from '@metamask/design-system-react-native'; -import Text, { + Box, + Text, TextColor, TextVariant, -} from '../../../../component-library/components/Texts/Text'; + FontWeight, +} from '@metamask/design-system-react-native'; import { strings } from '../../../../../locales/i18n'; import { RevealSeedViewSelectorsIDs } from '../RevealSeedView.testIds'; import { AppThemeKey } from '../../../../util/theme/models'; @@ -17,42 +18,51 @@ import { useTheme } from '../../../../util/theme'; import blurImage from '../../../../images/blur.png'; import darkBlurImage from '../../../../images/dark-blur.png'; import { SeedPhraseConcealerProps } from '../types'; +import { useTailwind } from '@metamask/design-system-twrnc-preset'; + +const FILL_STYLE = + 'absolute top-0 left-0 bottom-0 right-0 h-full rounded-lg flex-1'; const SeedPhraseConcealer = ({ onReveal, - styles, + testID = RevealSeedViewSelectorsIDs.REVEAL_CREDENTIAL_BUTTON_ID, }: SeedPhraseConcealerProps) => { const { themeAppearance } = useTheme(); + const tw = useTailwind(); return ( - + - + - + {strings('manual_backup_step_1.reveal')} - + {strings('manual_backup_step_1.watching')} - + - + ); }; diff --git a/app/components/Views/RevealPrivateCredential/components/SeedPhraseDisplay.tsx b/app/components/Views/RevealPrivateCredential/components/SeedPhraseDisplay.tsx index d4ec703f5cb..8f17ff50a56 100644 --- a/app/components/Views/RevealPrivateCredential/components/SeedPhraseDisplay.tsx +++ b/app/components/Views/RevealPrivateCredential/components/SeedPhraseDisplay.tsx @@ -1,20 +1,20 @@ import React from 'react'; -import { FlatList, View } from 'react-native'; -import Button, { - ButtonSize, - ButtonVariants, -} from '../../../../component-library/components/Buttons/Button'; -import Text, { +import { FlatList } from 'react-native'; +import { + Box, + Text, TextColor, TextVariant, -} from '../../../../component-library/components/Texts/Text'; -import { Box } from '../../../UI/Box/Box'; -import { - AlignItems, - FlexDirection, - JustifyContent, -} from '../../../UI/Box/box.types'; -import { IconName as IconNameLibrary } from '../../../../component-library/components/Icons/Icon'; + TextButton, + TextButtonSize, + IconName, + BoxFlexDirection, + BoxAlignItems, + BoxJustifyContent, + BoxBorderColor, + BoxBackgroundColor, + IconSize, +} from '@metamask/design-system-react-native'; import { strings } from '../../../../../locales/i18n'; import { ManualBackUpStepsSelectorsIDs } from '../../ManualBackupStep1/ManualBackUpSteps.testIds'; import { RevealSeedViewSelectorsIDs } from '../RevealSeedView.testIds'; @@ -25,58 +25,69 @@ const SeedPhraseDisplay = ({ showSeedPhrase, clipboardEnabled, onCopyToClipboard, - styles, }: SeedPhraseDisplayProps) => ( - + index.toString()} renderItem={({ item, index }) => ( - + {index + 1}. {item} - + )} /> - + {clipboardEnabled ? ( - ) : null} diff --git a/app/components/Views/NftDetails/__snapshots__/NftDetails.test.ts.snap b/app/components/Views/NftDetails/__snapshots__/NftDetails.test.ts.snap index 9471a3bf83f..59830404c0f 100644 --- a/app/components/Views/NftDetails/__snapshots__/NftDetails.test.ts.snap +++ b/app/components/Views/NftDetails/__snapshots__/NftDetails.test.ts.snap @@ -1315,60 +1315,93 @@ exports[`NftDetails renders correctly 1`] = ` } } > - Send - + diff --git a/app/components/Views/confirmations/__mocks__/send.mock.ts b/app/components/Views/confirmations/__mocks__/send.mock.ts index 117086004ab..266512a4c3d 100644 --- a/app/components/Views/confirmations/__mocks__/send.mock.ts +++ b/app/components/Views/confirmations/__mocks__/send.mock.ts @@ -62,6 +62,18 @@ export const MOCK_NFT1155 = { balance: '2', }; +export const MOCK_NFT721 = { + address: '0xBC4CA0EdA7647A8aB7C2061c2E118A18a936f13D', + chainId: 1, + description: 'A collection of 10,000 unique Bored Ape NFTs.', + favorite: false, + image: 'https://example.com/bayc/1.png', + isCurrentlyOwned: true, + name: 'Bored Ape Yacht Club #1', + standard: 'ERC721', + tokenId: '1', +}; + export const evmSendStateMock = { engine: { backgroundState: { diff --git a/app/components/Views/confirmations/components/send/amount/amount.test.tsx b/app/components/Views/confirmations/components/send/amount/amount.test.tsx index 33321ab46cc..79c410e3fcb 100644 --- a/app/components/Views/confirmations/components/send/amount/amount.test.tsx +++ b/app/components/Views/confirmations/components/send/amount/amount.test.tsx @@ -8,6 +8,7 @@ import renderWithProvider, { import { ETHEREUM_ADDRESS, MOCK_NFT1155, + MOCK_NFT721, SOLANA_ASSET, TOKEN_ADDRESS_MOCK_1, evmSendStateMock, @@ -27,7 +28,9 @@ jest.mock('../../../context/send-context', () => ({ jest.mock('../../../hooks/send/useCurrencyConversions'); -jest.mock('../../../hooks/send/useRouteParams'); +jest.mock('../../../hooks/send/useRouteParams', () => ({ + useRouteParams: jest.fn().mockReturnValue({ isLoading: false }), +})); jest.mock('../../../../../../util/navigation/navUtils', () => ({ useParams: jest.fn().mockReturnValue({}), @@ -320,6 +323,23 @@ describe('Amount', () => { expect(getByText('17')).toBeTruthy(); }); + it('displays NFT image and details for ERC721 asset', async () => { + mockUseSendContext.mockReturnValue({ + asset: MOCK_NFT721, + updateValue: jest.fn(), + } as unknown as ReturnType); + + const { getByTestId, getByText, queryByText } = renderComponent(); + + await waitFor(() => { + expect(getByTestId('nft-image')).toBeTruthy(); + }); + + expect(getByText('Bored Ape Yacht Club #1')).toBeTruthy(); + // ERC721 has no ticker/symbol — display symbol must be 'NFT', not empty + expect(queryByText('NFT')).toBeTruthy(); + }); + // it('display total balance correctly for ERC20 token', () => { // mockUseRoute.mockReturnValue({ // params: { diff --git a/app/components/Views/confirmations/components/send/amount/amount.tsx b/app/components/Views/confirmations/components/send/amount/amount.tsx index 2173933ecf1..d15caff4545 100644 --- a/app/components/Views/confirmations/components/send/amount/amount.tsx +++ b/app/components/Views/confirmations/components/send/amount/amount.tsx @@ -19,6 +19,7 @@ import Text, { } from '../../../../../../component-library/components/Texts/Text'; import { selectPrimaryCurrency } from '../../../../../../selectors/settings'; import CollectibleMedia from '../../../../../UI/CollectibleMedia'; +import { Skeleton } from '../../../../../../component-library/components-temp/Skeleton'; import { useStyles } from '../../../../../hooks/useStyles'; import Device from '../../../../../../util/device'; import { AssetType, TokenStandard } from '../../../types/token'; @@ -50,7 +51,9 @@ export const Amount = () => { getFiatValue, getFiatDisplayValue, } = useCurrencyConversions(); - const isNFT = asset?.standard === TokenStandard.ERC1155; + const isNFT = + asset?.standard === TokenStandard.ERC721 || + asset?.standard === TokenStandard.ERC1155; const assetSymbol = isNFT ? undefined : ((asset as AssetType)?.ticker ?? (asset as AssetType)?.symbol); @@ -61,7 +64,7 @@ export const Amount = () => { const isIos = Device.isIos(); const { setAmountInputTypeFiat, setAmountInputTypeToken } = useAmountSelectionMetrics(); - useRouteParams(); + const { isLoading: isNftLoading } = useRouteParams(); useEffect(() => { setFiatMode(primaryCurrency === 'Fiat'); @@ -149,6 +152,13 @@ export const Amount = () => { )} + {isNftLoading && ( + + + + + + )} { )} - - {balanceDisplayValue} - + {isNftLoading ? ( + + ) : ( + + {balanceDisplayValue} + + )} { jest.clearAllMocks(); mockUseSendTokens.mockReturnValue(mockTokens); - mockUseEVMNfts.mockReturnValue(mockNfts); + mockUseEVMNfts.mockReturnValue({ nfts: mockNfts, isLoading: false }); mockUseTokenSearch.mockReturnValue({ searchQuery: '', @@ -699,7 +699,7 @@ describe('Asset', () => { }); it('works correctly with empty nfts from useEVMNfts', () => { - mockUseEVMNfts.mockReturnValue([]); + mockUseEVMNfts.mockReturnValue({ nfts: [], isLoading: false }); mockUseTokenSearch.mockReturnValue({ searchQuery: '', setSearchQuery: mockSetSearchQuery, diff --git a/app/components/Views/confirmations/components/send/asset/asset.tsx b/app/components/Views/confirmations/components/send/asset/asset.tsx index 26e7d1cd0f3..8924198b79a 100644 --- a/app/components/Views/confirmations/components/send/asset/asset.tsx +++ b/app/components/Views/confirmations/components/send/asset/asset.tsx @@ -89,7 +89,7 @@ export const Asset: React.FC = (props = {}) => { }; }, [tokenItems]); - const nfts = useEVMNfts(); + const { nfts } = useEVMNfts(); const [filteredTokensByNetwork, setFilteredTokensByNetwork] = useState(tokens); const [selectedNetworkFilter, setSelectedNetworkFilter] = diff --git a/app/components/Views/confirmations/hooks/send/useNfts.test.tsx b/app/components/Views/confirmations/hooks/send/useNfts.test.tsx index dd4261d6986..716f0029eb7 100644 --- a/app/components/Views/confirmations/hooks/send/useNfts.test.tsx +++ b/app/components/Views/confirmations/hooks/send/useNfts.test.tsx @@ -15,6 +15,9 @@ import { import BigNumber from 'bignumber.js'; import Engine from '../../../../../core/Engine'; +import Logger from '../../../../../util/Logger'; + +jest.mock('../../../../../util/Logger'); import { selectSelectedAccountGroup } from '../../../../../selectors/multichainAccounts/accountTreeController'; import { selectInternalAccountsById } from '../../../../../selectors/accountsController'; import { selectAllNfts } from '../../../../../selectors/nftController'; @@ -258,7 +261,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toEqual([]); + expect(result.current.nfts).toEqual([]); }); }); @@ -278,7 +281,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toEqual([]); + expect(result.current.nfts).toEqual([]); }); }); @@ -296,7 +299,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toEqual([]); + expect(result.current.nfts).toEqual([]); }); }); @@ -320,8 +323,8 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(1); - expect(result.current[0]).toEqual({ + expect(result.current.nfts).toHaveLength(1); + expect(result.current.nfts[0]).toEqual({ address: mockNft.address, standard: 'ERC721', name: mockNft.name, @@ -361,9 +364,9 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(1); - expect(result.current[0].standard).toBe('ERC1155'); - expect(result.current[0].balance).toBe('1'); + expect(result.current.nfts).toHaveLength(1); + expect(result.current.nfts[0].standard).toBe('ERC1155'); + expect(result.current.nfts[0].balance).toBe('1'); }); }); @@ -396,8 +399,8 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(1); - expect(result.current[0].name).toBe('Test NFT'); + expect(result.current.nfts).toHaveLength(1); + expect(result.current.nfts[0].name).toBe('Test NFT'); }); }); @@ -427,7 +430,9 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBe('https://example.com/fallback.png'); + expect(result.current.nfts[0].image).toBe( + 'https://example.com/fallback.png', + ); }); }); @@ -457,7 +462,9 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBe('https://example.com/valid.png'); + expect(result.current.nfts[0].image).toBe( + 'https://example.com/valid.png', + ); }); }); @@ -471,7 +478,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toEqual([]); + expect(result.current.nfts).toEqual([]); }); }); @@ -489,7 +496,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toEqual([]); + expect(result.current.nfts).toEqual([]); }); }); @@ -522,8 +529,8 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(1); - expect(result.current[0].accountId).toBe(mockAccount.id); + expect(result.current.nfts).toHaveLength(1); + expect(result.current.nfts[0].accountId).toBe(mockAccount.id); }); }); @@ -551,9 +558,9 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(2); - expect(result.current[0].chainId).toBe('0x1'); - expect(result.current[1].chainId).toBe('0x89'); + expect(result.current.nfts).toHaveLength(2); + expect(result.current.nfts[0].chainId).toBe('0x1'); + expect(result.current.nfts[1].chainId).toBe('0x89'); }); }); @@ -583,8 +590,8 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current).toHaveLength(1); - expect(result.current[0].name).toBe('Test NFT'); + expect(result.current.nfts).toHaveLength(1); + expect(result.current.nfts[0].name).toBe('Test NFT'); }); }); @@ -622,7 +629,9 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBe('https://example.com/sample1.png'); + expect(result.current.nfts[0].image).toBe( + 'https://example.com/sample1.png', + ); }); }); @@ -657,7 +666,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBeUndefined(); + expect(result.current.nfts[0].image).toBeUndefined(); }); }); @@ -688,7 +697,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBe( + expect(result.current.nfts[0].image).toBe( 'https://example.com/valid-fallback.png', ); }); @@ -725,7 +734,7 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].image).toBe( + expect(result.current.nfts[0].image).toBe( 'https://example.com/collection-fallback.png', ); }); @@ -756,8 +765,8 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].name).toBeUndefined(); - expect(result.current[0].collectionName).toBe('Test Collection'); + expect(result.current.nfts[0].name).toBeUndefined(); + expect(result.current.nfts[0].collectionName).toBe('Test Collection'); }); }); @@ -789,7 +798,90 @@ describe('useEVMNfts', () => { const { result } = renderHookWithStore(() => useEVMNfts()); await waitFor(() => { - expect(result.current[0].collectionName).toBeUndefined(); + expect(result.current.nfts[0].collectionName).toBeUndefined(); + }); + }); + + describe('isLoading', () => { + it('does not update state when the effect is cancelled before processNfts completes', async () => { + // Simulate a slow async transform so the effect cleanup fires mid-flight. + let resolveTransform: () => void; + const transformStarted = new Promise((res) => { + resolveTransform = res; + }); + let resolveSlowTransform: ((value: string) => void) | undefined; + const slowTransformPromise = new Promise((res) => { + resolveSlowTransform = res; + }); + + mockGetFormattedIpfsUrl.mockImplementation(async () => { + resolveTransform(); + return slowTransformPromise; + }); + + const ipfsNft = { + ...mockNft, + image: 'ipfs://QmTest/image.png', + }; + + mockSelectSelectedAccountGroup.mockReturnValue( + createMockAccountGroup(['account-1']), + ); + mockSelectInternalAccountsById.mockReturnValue( + createMockInternalAccountsById({ 'account-1': mockAccount }), + ); + mockSelectAllNfts.mockReturnValue( + createMockAllNfts({ + [mockAccount.address]: { '0x1': [ipfsNft] }, + }), + ); + + const { unmount, result } = renderHookWithStore(() => useEVMNfts()); + + // Wait until processNfts has started the async transform, then unmount + // (simulates a dependency change that re-fires the effect). + await transformStarted; + unmount(); + + // Unblock the slow transform after the cleanup has run. + resolveSlowTransform?.('https://gateway/QmTest/image.png'); + + // State must not have been updated — isLoading stays true (initial value) + // and nfts stays empty because the cancelled run's setters were skipped. + expect(result.current.isLoading).toBe(true); + expect(result.current.nfts).toEqual([]); + }); + + it('settles to false and logs error when processNfts throws', async () => { + mockSelectSelectedAccountGroup.mockReturnValue( + createMockAccountGroup(['account-1']), + ); + mockSelectInternalAccountsById.mockReturnValue( + createMockInternalAccountsById({ + 'account-1': mockAccount, + }), + ); + mockSelectAllNfts.mockReturnValue( + createMockAllNfts({ + [mockAccount.address]: { '0x1': [mockNft] }, + }), + ); + const processingError = new Error('transform failed'); + // getNetworkBadgeSource is called inside transformNft without a try-catch, + // so throwing here propagates out of processNfts to the .catch() handler. + mockGetNetworkBadgeSource.mockImplementation(() => { + throw processingError; + }); + + const { result } = renderHookWithStore(() => useEVMNfts()); + + await waitFor(() => { + expect(result.current.isLoading).toBe(false); + }); + expect(Logger.error).toHaveBeenCalledWith( + processingError, + 'useEVMNfts: processNfts failed', + ); }); }); }); diff --git a/app/components/Views/confirmations/hooks/send/useNfts.ts b/app/components/Views/confirmations/hooks/send/useNfts.ts index 911ccf5cd4d..7bb10c34a9f 100644 --- a/app/components/Views/confirmations/hooks/send/useNfts.ts +++ b/app/components/Views/confirmations/hooks/send/useNfts.ts @@ -14,13 +14,19 @@ import { getFormattedIpfsUrl } from '@metamask/assets-controllers'; import useIpfsGateway from '../../../../hooks/useIpfsGateway'; import Logger from '../../../../../util/Logger'; -export function useEVMNfts(): Nft[] { +export interface UseEVMNftsResult { + nfts: Nft[]; + isLoading: boolean; +} + +export function useEVMNfts(): UseEVMNftsResult { const { NftController, AssetsContractController, NetworkController } = Engine.context; const selectedAccountGroup = useSelector(selectSelectedAccountGroup); const internalAccountsById = useSelector(selectInternalAccountsById); const allNFTS = useSelector(selectAllNfts); const [transformedNfts, setTransformedNfts] = useState([]); + const [isLoading, setIsLoading] = useState(true); const { isSolanaOnly } = useSendScope(); const ipfsGateway = useIpfsGateway(); @@ -29,8 +35,12 @@ export function useEVMNfts(): Nft[] { .filter((account) => isEvmAddress(account.address))?.[0]; useEffect(() => { + let cancelled = false; + + setIsLoading(true); if (!evmAccount || !allNFTS) { setTransformedNfts([]); + setIsLoading(false); return; } @@ -73,10 +83,22 @@ export function useEVMNfts(): Nft[] { } } - setTransformedNfts(transformedResults); + if (!cancelled) { + setTransformedNfts(transformedResults); + setIsLoading(false); + } }; - processNfts(); + processNfts().catch((error) => { + if (!cancelled) { + Logger.error(error, 'useEVMNfts: processNfts failed'); + setIsLoading(false); + } + }); + + return () => { + cancelled = true; + }; }, [ ipfsGateway, evmAccount, @@ -87,10 +109,10 @@ export function useEVMNfts(): Nft[] { ]); if (isSolanaOnly) { - return []; + return { nfts: [], isLoading: false }; } - return transformedNfts; + return { nfts: transformedNfts, isLoading }; } async function getValidImageUrl( diff --git a/app/components/Views/confirmations/hooks/send/useRouteParams.test.ts b/app/components/Views/confirmations/hooks/send/useRouteParams.test.ts index 780ecdd877c..ca6612ef980 100644 --- a/app/components/Views/confirmations/hooks/send/useRouteParams.test.ts +++ b/app/components/Views/confirmations/hooks/send/useRouteParams.test.ts @@ -59,7 +59,7 @@ describe('useRouteParams', () => { return { '0x1': [] }; } }); - mockUseNfts.mockReturnValue([]); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -87,7 +87,7 @@ describe('useRouteParams', () => { return { '0x1': [] }; } }); - mockUseNfts.mockReturnValue([]); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -119,7 +119,7 @@ describe('useRouteParams', () => { return { '0x1': [] }; } }); - mockUseNfts.mockReturnValue([]); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -149,7 +149,7 @@ describe('useRouteParams', () => { return { '0x1': [] }; } }); - mockUseNfts.mockReturnValue([]); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -179,7 +179,7 @@ describe('useRouteParams', () => { return { '0x1': [assetToken] }; } }); - mockUseNfts.mockReturnValue([]); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -193,11 +193,13 @@ describe('useRouteParams', () => { id: '123', address: 'dummy_address', chainId: 'summy_chainId', + tokenId: '1', }; const assetNft = { id: '123', address: 'dummy_address', chainId: 'summy_chainId', + tokenId: '1', }; mockUseParams.mockReturnValue({ asset }); const mockUpdateAsset = jest.fn(); @@ -209,7 +211,10 @@ describe('useRouteParams', () => { return { '0x1': [] }; } }); - mockUseNfts.mockReturnValue([assetNft as unknown as Nft]); + mockUseNfts.mockReturnValue({ + nfts: [assetNft as unknown as Nft], + isLoading: false, + }); renderHookWithProvider(() => useRouteParams(), mockState); @@ -217,4 +222,131 @@ describe('useRouteParams', () => { expect(mockUpdateAsset).toHaveBeenCalledWith(assetNft); }); }); + + it('matches ERC1155 token by tokenId when multiple tokens share the same contract and chain', async () => { + const targetTokenId = '42'; + const paramsAsset = { + address: '0xcontract', + chainId: '0x1', + tokenId: targetTokenId, + }; + const wrongNft = { + address: '0xcontract', + chainId: '0x1', + tokenId: '1', + standard: 'ERC1155', + balance: '5', + }; + const correctNft = { + address: '0xcontract', + chainId: '0x1', + tokenId: targetTokenId, + standard: 'ERC1155', + balance: '2', + }; + mockUseParams.mockReturnValue({ asset: paramsAsset }); + const mockUpdateAsset = jest.fn(); + mockUseSendContext.mockReturnValue({ + updateAsset: mockUpdateAsset, + } as unknown as ReturnType); + mockUseSelector.mockImplementation((selector) => { + if (selector === selectAssetsBySelectedAccountGroup) { + return { '0x1': [] }; + } + }); + mockUseNfts.mockReturnValue({ + nfts: [wrongNft, correctNft] as unknown as Nft[], + isLoading: false, + }); + + renderHookWithProvider(() => useRouteParams(), mockState); + + await waitFor(() => { + expect(mockUpdateAsset).toHaveBeenCalledWith(correctNft); + expect(mockUpdateAsset).not.toHaveBeenCalledWith(wrongNft); + }); + }); + + describe('isLoading', () => { + it('returns true while NFTs are loading and NFT asset is not resolved yet', async () => { + const asset = { + id: '123', + address: 'dummy_address', + chainId: 'dummy_chainId', + tokenId: '42', + }; + mockUseParams.mockReturnValue({ asset }); + mockUseSendContext.mockReturnValue({ + asset: undefined, + updateAsset: jest.fn(), + } as unknown as ReturnType); + mockUseSelector.mockImplementation((selector) => { + if (selector === selectAssetsBySelectedAccountGroup) { + return { '0x1': [] }; + } + }); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: true }); + + const { result } = renderHookWithProvider( + () => useRouteParams(), + mockState, + ); + + expect(result.current.isLoading).toBe(true); + }); + + it('returns false when NFTs finish loading and the asset is resolved', async () => { + const asset = { + id: '123', + address: 'dummy_address', + chainId: 'dummy_chainId', + tokenId: '42', + }; + mockUseParams.mockReturnValue({ asset }); + mockUseSendContext.mockReturnValue({ + asset, + updateAsset: jest.fn(), + } as unknown as ReturnType); + mockUseSelector.mockImplementation((selector) => { + if (selector === selectAssetsBySelectedAccountGroup) { + return { '0x1': [] }; + } + }); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: false }); + + const { result } = renderHookWithProvider( + () => useRouteParams(), + mockState, + ); + + expect(result.current.isLoading).toBe(false); + }); + + it('returns false when the params asset has no tokenId (non-NFT flow)', async () => { + const asset = { + id: '123', + address: 'dummy_address', + chainId: 'dummy_chainId', + symbol: 'ETH', + }; + mockUseParams.mockReturnValue({ asset }); + mockUseSendContext.mockReturnValue({ + asset: undefined, + updateAsset: jest.fn(), + } as unknown as ReturnType); + mockUseSelector.mockImplementation((selector) => { + if (selector === selectAssetsBySelectedAccountGroup) { + return { '0x1': [] }; + } + }); + mockUseNfts.mockReturnValue({ nfts: [], isLoading: true }); + + const { result } = renderHookWithProvider( + () => useRouteParams(), + mockState, + ); + + expect(result.current.isLoading).toBe(false); + }); + }); }); diff --git a/app/components/Views/confirmations/hooks/send/useRouteParams.ts b/app/components/Views/confirmations/hooks/send/useRouteParams.ts index 7509070b198..9e6eda6a3e7 100644 --- a/app/components/Views/confirmations/hooks/send/useRouteParams.ts +++ b/app/components/Views/confirmations/hooks/send/useRouteParams.ts @@ -22,7 +22,7 @@ const createAssetFromParams = (paramsAsset: AssetType): AssetType => ({ export const useRouteParams = () => { const assets = useSelector(selectAssetsBySelectedAccountGroup); const flatAssets = useMemo(() => Object.values(assets).flat(), [assets]); - const nfts = useEVMNfts(); + const { nfts, isLoading: isNftsLoading } = useEVMNfts(); const { asset: paramsAsset } = useParams<{ asset: AssetType; @@ -49,9 +49,10 @@ export const useRouteParams = () => { if (!filteredAsset && nfts.length) { filteredAsset = nfts.find( - ({ address, chainId }) => + ({ address, chainId, tokenId }) => address === paramsAsset.address && - chainId?.toLowerCase() === paramChainId, + chainId?.toLowerCase() === paramChainId && + tokenId === paramsAsset.tokenId, ); } @@ -66,4 +67,10 @@ export const useRouteParams = () => { } } }, [asset, paramsAsset, nfts, flatAssets, updateAsset]); + + // True while NFT processing is in progress and the asset hasn't been + // resolved into the send context yet (i.e. navigated from NftDetails). + const isLoading = Boolean(paramsAsset?.tokenId && isNftsLoading && !asset); + + return { isLoading }; }; From f09974bd9b2184bf99bc78c0b5eb5522331b37a8 Mon Sep 17 00:00:00 2001 From: abretonc7s <107169956+abretonc7s@users.noreply.github.com> Date: Wed, 25 Mar 2026 17:42:19 +0800 Subject: [PATCH 5/8] fix(perps): Pill stop loss don't show up in recent activity (regression) (#27685) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Stop loss and take profit pills were not rendering on activity rows because the HyperLiquid fills API does not return order type information. Fills now get enriched with `detailedOrderType` by cross-referencing historical orders at the provider level. Additionally, the merge logic in both `usePerpsHomeData` and `usePerpsTransactionHistory` now preserves enrichment when WebSocket fills overwrite REST fills. Also fixes the "Liquidated" pill rendering on a separate row by adding the missing `fillTag` style with `flexDirection: 'row'` to PerpsTransactionsView.styles.ts. ## **Changelog** CHANGELOG entry: Fixed stop loss and take profit pills not appearing on recent activity rows in Perps Home and Activity screens ## **Related issues** Fixes: [TAT-2668](https://consensyssoftware.atlassian.net/browse/TAT-2668) ## **Manual testing steps** ```gherkin Feature: Activity pill rendering for TP/SL orders Scenario: Stop loss pill on closed position Given a position was closed via stop loss When the user views recent activity on Perps Home Then the activity row shows a "Stop loss" pill Scenario: Take profit pill on closed position Given a position was closed via take profit When the user views the Activity page Then the activity row shows a "Take profit" pill Scenario: No regression on other pill types Given a position was liquidated When the user views the Activity page Then the "Liquidated" pill renders inline on the Closed Long row ``` ## **Screenshots/Recordings** ### **Before** Pills missing on activity rows for TP/SL closes. "Liquidated" pill on separate row instead of inline with "Closed Long". https://github.com/user-attachments/assets/ef394ef1-97dc-48a9-9780-292bcfd09cd7 ### **After** - Video: `automation/27685/after.mp4` — Perps Activity page with fix applied - CDP eval evidence: 4/4 TP/SL fills enriched (`Stop Market`, `Take Profit Limit`) - `fillTag` style added to ensure pills render inline (row layout) https://github.com/user-attachments/assets/4aa46bef-6bf5-478c-8769-2f0f08509344 ## **Validation Recipe**
Automated validation recipe (validate-recipe.sh) ```json { "pr": "27685", "title": "Stop loss and take profit pills appear in recent activity", "jira": "TAT-2668", "acceptance_criteria": [ "Stop loss and take profit pills appear inline on the activity row when a position is closed via stop loss or take profit", "The Liquidated pill renders on the same row as the Closed Long label", "No other activity pill types are broken or misaligned", "Behavior is consistent across Perps Home, Perp Market screen, and Activity page" ], "validate": { "static": ["yarn lint:tsc"], "runtime": { "pre_conditions": [ "CDP connected", "Wallet unlocked on Wallet route", "Active account has TP/SL fill history (e.g. 0x316bde)" ], "steps": [ { "id": "nav_activity", "description": "Navigate to Perps Activity with Trades tab", "action": "navigate", "target": "PerpsActivity", "params": { "redirectToPerpsTransactions": true } }, { "id": "wait_load", "description": "Wait for transaction data to load", "action": "wait", "ms": 8000 }, { "id": "verify_enrichment", "description": "Verify REST fills are enriched with detailedOrderType from historical orders", "action": "eval_async", "expression": "Engine.context.PerpsController.getActiveProviderOrNull().getOrderFills({aggregateByTime:false}).then(function(fills){var tpsl=fills.filter(function(f){return f.detailedOrderType&&(f.detailedOrderType.indexOf('Stop')>=0||f.detailedOrderType.indexOf('Take Profit')>=0)});return JSON.stringify({total:fills.length,tpslCount:tpsl.length})})", "assert": { "operator": "not_null" } }, { "id": "scroll_flashlist", "description": "Scroll the FlashList to verify testID targeting works", "action": "scroll", "test_id": "perps-transactions-flash-list", "offset": 400 }, { "id": "scroll_back", "description": "Scroll back to top to show first rows with pills", "action": "scroll", "test_id": "perps-transactions-flash-list", "offset": 0 }, { "id": "check_no_errors", "description": "No errors in Metro logs during Activity page load", "action": "log_watch", "window_seconds": 5, "must_not_appear": ["TypeError", "undefined is not an object"] }, { "id": "screenshot_evidence", "description": "Screenshot showing TP/SL pills on activity rows", "action": "screenshot", "filename": "activity-tpsl-pills.png" } ] } } } ```
## **Pre-merge author checklist** - [x] I've followed MetaMask Contributor Docs and Coding Standards - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [ ] I've documented my code using JSDoc format if applicable - [x] I've applied the right labels on the PR ## **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. --- > [!NOTE] > **Medium Risk** > Medium risk because it changes how REST and WebSocket fills are merged and enriched across provider, subscription, and UI activity hooks, which can affect transaction history correctness and ordering. > > **Overview** > Restores *Take Profit/Stop Loss* (and related) pills in Perps activity by enriching HyperLiquid `getOrderFills` responses with `detailedOrderType` via a parallel `historicalOrders` lookup, and enriching WebSocket fills using cached order data. > > Updates merge/dedup logic in `usePerpsHomeData` and `usePerpsTransactionHistory` to **preserve REST-derived enrichment** (including liquidation details / non-`Standard` `fillType`) when live WS data overwrites duplicates, with new unit tests covering these cases. > > Adds E2E selectors (`FlashList` + fill tag testIDs) and a small layout fix (`fillTag` row style) so pills render inline and are targetable in automation. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 71223e105023ab7cfb0d8d3c01ddd5268c660546. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- app/components/UI/Perps/Perps.testIds.ts | 9 ++ .../PerpsTransactionsView.styles.ts | 5 + .../PerpsTransactionsView.tsx | 1 + .../components/PerpsFillTag/PerpsFillTag.tsx | 27 +++-- .../UI/Perps/hooks/usePerpsHomeData.test.ts | 51 ++++++++ .../UI/Perps/hooks/usePerpsHomeData.ts | 13 +- .../hooks/usePerpsTransactionHistory.test.ts | 66 +++++++++++ .../Perps/hooks/usePerpsTransactionHistory.ts | 24 +++- .../providers/HyperLiquidProvider.test.ts | 112 ++++++++++++++++++ .../perps/providers/HyperLiquidProvider.ts | 40 +++++++ .../HyperLiquidSubscriptionService.test.ts | 56 +++++++++ .../HyperLiquidSubscriptionService.ts | 53 +++++---- 12 files changed, 424 insertions(+), 33 deletions(-) diff --git a/app/components/UI/Perps/Perps.testIds.ts b/app/components/UI/Perps/Perps.testIds.ts index a7c63e8d443..9740c90ea71 100644 --- a/app/components/UI/Perps/Perps.testIds.ts +++ b/app/components/UI/Perps/Perps.testIds.ts @@ -417,6 +417,15 @@ export const PerpsTransactionSelectorsIDs = { FUNDING_TRANSACTION_VIEW: 'perps-funding-transaction-view', ORDER_TRANSACTION_VIEW: 'perps-order-transaction-view', + // FlashList + FLASH_LIST: 'perps-transactions-flash-list', + + // Fill tags + FILL_TAG_TAKE_PROFIT: 'perps-fill-tag-take-profit', + FILL_TAG_STOP_LOSS: 'perps-fill-tag-stop-loss', + FILL_TAG_LIQUIDATED: 'perps-fill-tag-liquidated', + FILL_TAG_ADL: 'perps-fill-tag-adl', + // Common buttons BLOCK_EXPLORER_BUTTON: 'block-explorer-button', }; diff --git a/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.styles.ts b/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.styles.ts index 61758f7ccaa..de9f2a783d5 100644 --- a/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.styles.ts +++ b/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.styles.ts @@ -56,6 +56,11 @@ export const styleSheet = (params: { theme: Theme }) => { fontSize: 14, color: colors.text.alternative, }, + fillTag: { + flexDirection: 'row' as const, + alignItems: 'center' as const, + gap: 8, + }, rightContent: { alignItems: 'flex-end' as const, }, diff --git a/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.tsx b/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.tsx index 186ad0df69f..8abce05f38e 100644 --- a/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.tsx +++ b/app/components/UI/Perps/Views/PerpsTransactionsView/PerpsTransactionsView.tsx @@ -481,6 +481,7 @@ const PerpsTransactionsView: React.FC = () => { )} = ({ severity: TagSeverity.Info, textColor: TextColor.Default, includesBorder: false, + testID: PerpsTransactionSelectorsIDs.FILL_TAG_ADL, }, [FillType.Liquidation]: { // Only show if liquidated user is current user @@ -73,18 +75,21 @@ const PerpsFillTag: React.FC = ({ severity: TagSeverity.Danger, textColor: TextColor.Error, includesBorder: false, + testID: PerpsTransactionSelectorsIDs.FILL_TAG_LIQUIDATED, }, [FillType.TakeProfit]: { label: strings('perps.transactions.order.take_profit'), severity: TagSeverity.Default, textColor: TextColor.Alternative, includesBorder: true, + testID: PerpsTransactionSelectorsIDs.FILL_TAG_TAKE_PROFIT, }, [FillType.StopLoss]: { label: strings('perps.transactions.order.stop_loss'), severity: TagSeverity.Default, textColor: TextColor.Alternative, includesBorder: true, + testID: PerpsTransactionSelectorsIDs.FILL_TAG_STOP_LOSS, }, }; @@ -95,15 +100,17 @@ const PerpsFillTag: React.FC = ({ } const tagContent = ( - - - {tagConfig.label} - - + + + + {tagConfig.label} + + + ); // Only wrap in TouchableOpacity for ADL fill type which has an action. diff --git a/app/components/UI/Perps/hooks/usePerpsHomeData.test.ts b/app/components/UI/Perps/hooks/usePerpsHomeData.test.ts index 304bab74e86..86ad423933e 100644 --- a/app/components/UI/Perps/hooks/usePerpsHomeData.test.ts +++ b/app/components/UI/Perps/hooks/usePerpsHomeData.test.ts @@ -976,6 +976,57 @@ describe('usePerpsHomeData', () => { }); }); + it('preserves detailedOrderType from REST fill when WS fill lacks it', async () => { + // Arrange — REST fill has enriched detailedOrderType + const restFill = createMockOrderFill({ + orderId: 'fill-tp-1', + symbol: 'BTC', + timestamp: 1234567800, + detailedOrderType: 'Take Profit Limit', + }); + const mockGetOrderFills = jest.fn().mockResolvedValue([restFill]); + ( + Engine.context.PerpsController.getActiveProviderOrNull as jest.Mock + ).mockReturnValue({ + getOrderFills: mockGetOrderFills, + }); + + // WS fill with same key but no detailedOrderType + const wsFill = createMockOrderFill({ + orderId: 'fill-tp-1', + symbol: 'BTC', + timestamp: 1234567800, + }); + mockUsePerpsLiveFills.mockReturnValue({ + fills: [wsFill], + isInitialLoading: false, + }); + + mockUsePerpsConnection.mockReturnValue({ + isConnected: true, + isInitialized: true, + isConnecting: false, + error: null, + connect: jest.fn(), + disconnect: jest.fn(), + resetError: jest.fn(), + } as never); + + // Act + const { result } = renderHook(() => usePerpsHomeData()); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + // Assert — recentActivity contains the merged fill with preserved detailedOrderType + // The detailedOrderType from REST is preserved during merge, then + // transformFillsToTransactions converts it to FillType.TakeProfit + expect(result.current.recentActivity).toHaveLength(1); + expect(result.current.recentActivity[0].fill?.fillType).toBe( + FillType.TakeProfit, + ); + }); + it('handles special characters in search query', () => { const { result } = renderHook(() => usePerpsHomeData({ searchQuery: '$BTC*' }), diff --git a/app/components/UI/Perps/hooks/usePerpsHomeData.ts b/app/components/UI/Perps/hooks/usePerpsHomeData.ts index 531716bd08d..72df7f435e9 100644 --- a/app/components/UI/Perps/hooks/usePerpsHomeData.ts +++ b/app/components/UI/Perps/hooks/usePerpsHomeData.ts @@ -136,9 +136,20 @@ export const usePerpsHomeData = ({ } // Add live fills (overwrites duplicates from REST - live data is fresher) + // Preserve detailedOrderType from REST fills since WS fills lack it for (const fill of liveFills) { const key = `${fill.orderId}-${fill.timestamp}`; - fillsMap.set(key, fill); + const existing = fillsMap.get(key); + if (existing?.detailedOrderType && !fill.detailedOrderType) { + fillsMap.set(key, { + ...fill, + detailedOrderType: existing.detailedOrderType, + ...(existing.liquidation && + !fill.liquidation && { liquidation: existing.liquidation }), + }); + } else { + fillsMap.set(key, fill); + } } // Convert back to array and sort by timestamp descending (newest first) diff --git a/app/components/UI/Perps/hooks/usePerpsTransactionHistory.test.ts b/app/components/UI/Perps/hooks/usePerpsTransactionHistory.test.ts index 6494d03506a..548acd84f90 100644 --- a/app/components/UI/Perps/hooks/usePerpsTransactionHistory.test.ts +++ b/app/components/UI/Perps/hooks/usePerpsTransactionHistory.test.ts @@ -1077,6 +1077,72 @@ describe('usePerpsTransactionHistory', () => { }); }); + describe('WS fill merge preserves fillType from REST', () => { + it('preserves non-standard fillType when WS fill has standard', async () => { + // Arrange — REST returns a trade with stop_loss fillType + const restTrade = { + ...mockTransformedTransactions[0], + id: 'rest-sl-1', + asset: 'BTC', + timestamp: 1641000000000, + fill: { + ...mockTransformedTransactions[0].fill, + fillType: FillType.StopLoss, + }, + }; + // Live fill with same asset+timestamp(seconds) but standard fillType + const wsFill = { + ...mockTransformedTransactions[0], + id: 'ws-sl-1', + asset: 'BTC', + timestamp: 1641000000000, + fill: { + ...mockTransformedTransactions[0].fill, + fillType: FillType.Standard, + }, + }; + + // Call order: (1) useMemo on initial render with liveFills, + // (2) fetchAllTransactions with REST fills (sets state), + // (3) useMemo re-runs with liveFills after state update + mockTransformFillsToTransactions + .mockReturnValueOnce([wsFill]) // initial render: live fills + .mockReturnValueOnce([restTrade]) // fetchAllTransactions: REST fills + .mockReturnValue([wsFill]); // re-render: live fills again + + mockUsePerpsLiveFills.mockReturnValue({ + fills: [ + { + orderId: 'ws-1', + timestamp: 1641000000000, + symbol: 'BTC', + side: 'buy', + size: '0.1', + price: '50000', + pnl: '0', + direction: 'Open Long', + fee: '5', + feeToken: 'USDC', + }, + ], + isInitialLoading: false, + }); + + // Act + const { result } = renderHook(() => usePerpsTransactionHistory()); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + // Assert — merged trade preserves the stop_loss fillType + const trades = result.current.transactions.filter( + (tx) => tx.type === 'trade', + ); + expect(trades).toHaveLength(1); + expect(trades[0].fill?.fillType).toBe(FillType.StopLoss); + }); + }); + describe('connection state transitions', () => { it('triggers fetch when skipInitialFetch transitions from true to false', async () => { // Reset mocks to track calls clearly diff --git a/app/components/UI/Perps/hooks/usePerpsTransactionHistory.ts b/app/components/UI/Perps/hooks/usePerpsTransactionHistory.ts index 3db153d84f6..1a1b371e232 100644 --- a/app/components/UI/Perps/hooks/usePerpsTransactionHistory.ts +++ b/app/components/UI/Perps/hooks/usePerpsTransactionHistory.ts @@ -9,7 +9,7 @@ import type { CaipAccountId } from '@metamask/utils'; import { areAddressesEqual } from '../../../../util/address'; import { selectNonReplacedTransactions } from '../../../../selectors/transactionController'; import { selectSelectedInternalAccountFormattedAddress } from '../../../../selectors/accountsController'; -import { PerpsTransaction } from '../types/transactionHistory'; +import { FillType, PerpsTransaction } from '../types/transactionHistory'; import { useUserHistory } from './useUserHistory'; import { usePerpsLiveFills } from './stream/usePerpsLiveFills'; import { @@ -288,10 +288,30 @@ export const usePerpsTransactionHistory = ({ } // Add live fills (overwrites REST duplicates - live data is fresher) + // Preserve fillType from REST when WS fill lacks enrichment (TP/SL pills) for (const tx of liveTransactions) { const timestampSeconds = Math.floor(tx.timestamp / 1000); const dedupKey = `${tx.asset}-${timestampSeconds}`; - tradeMap.set(dedupKey, tx); + const existing = tradeMap.get(dedupKey); + if ( + existing?.fill?.fillType && + existing.fill.fillType !== FillType.Standard && + tx.fill?.fillType === FillType.Standard + ) { + tradeMap.set(dedupKey, { + ...tx, + fill: { + ...tx.fill, + fillType: existing.fill.fillType, + ...(existing.fill.liquidation && + !tx.fill.liquidation && { + liquidation: existing.fill.liquidation, + }), + }, + }); + } else { + tradeMap.set(dedupKey, tx); + } } // Combine deduplicated trades with non-trade transactions (including wallet deposits) diff --git a/app/controllers/perps/providers/HyperLiquidProvider.test.ts b/app/controllers/perps/providers/HyperLiquidProvider.test.ts index 6cc73a93b89..136e037c60e 100644 --- a/app/controllers/perps/providers/HyperLiquidProvider.test.ts +++ b/app/controllers/perps/providers/HyperLiquidProvider.test.ts @@ -261,6 +261,9 @@ const createMockInfoClient = (overrides: Record = {}) => ({ ], universe: [], }), + historicalOrders: jest.fn().mockResolvedValue([]), + userFills: jest.fn().mockResolvedValue([]), + userFillsByTime: jest.fn().mockResolvedValue([]), ...overrides, }); @@ -6512,6 +6515,115 @@ describe('HyperLiquidProvider', () => { }); }); + describe('getOrderFills enrichment with detailedOrderType', () => { + it('enriches fills with detailedOrderType from historical orders', async () => { + const mockUserFills = jest.fn().mockResolvedValue([ + { + oid: 100, + coin: 'BTC', + side: 'A', + sz: '0.5', + px: '50000', + fee: '5', + feeToken: 'USDC', + time: Date.now(), + closedPnl: '-200', + dir: 'Close Long', + startPosition: '0.5', + }, + { + oid: 101, + coin: 'ETH', + side: 'B', + sz: '1.0', + px: '3000', + fee: '3', + feeToken: 'USDC', + time: Date.now(), + closedPnl: '100', + dir: 'Close Short', + startPosition: '-1.0', + }, + ]); + + const mockHistoricalOrders = jest.fn().mockResolvedValue([ + { + order: { + oid: 100, + coin: 'BTC', + side: 'A', + sz: '0', + origSz: '0.5', + limitPx: '50000', + orderType: 'Stop Market', + reduceOnly: true, + isTrigger: true, + }, + status: 'filled', + statusTimestamp: Date.now(), + }, + { + order: { + oid: 101, + coin: 'ETH', + side: 'B', + sz: '0', + origSz: '1.0', + limitPx: '3000', + orderType: 'Take Profit Limit', + reduceOnly: true, + isTrigger: true, + }, + status: 'filled', + statusTimestamp: Date.now(), + }, + ]); + + mockClientService.getInfoClient = jest.fn().mockReturnValue( + createMockInfoClient({ + userFills: mockUserFills, + historicalOrders: mockHistoricalOrders, + }), + ); + + const fills = await provider.getOrderFills(); + + expect(fills).toHaveLength(2); + expect(fills[0].detailedOrderType).toBe('Stop Market'); + expect(fills[1].detailedOrderType).toBe('Take Profit Limit'); + }); + + it('gracefully handles historicalOrders failure', async () => { + const mockUserFills = jest.fn().mockResolvedValue([ + { + oid: 200, + coin: 'BTC', + side: 'B', + sz: '0.1', + px: '60000', + fee: '6', + feeToken: 'USDC', + time: Date.now(), + closedPnl: '0', + dir: 'Open Long', + startPosition: '0', + }, + ]); + + mockClientService.getInfoClient = jest.fn().mockReturnValue( + createMockInfoClient({ + userFills: mockUserFills, + historicalOrders: jest.fn().mockRejectedValue(new Error('API error')), + }), + ); + + const fills = await provider.getOrderFills(); + + expect(fills).toHaveLength(1); + expect(fills[0].detailedOrderType).toBeUndefined(); + }); + }); + describe('getOpenOrders additional coverage', () => { it('returns empty array when frontendOpenOrders throws error', async () => { // Arrange diff --git a/app/controllers/perps/providers/HyperLiquidProvider.ts b/app/controllers/perps/providers/HyperLiquidProvider.ts index 480ac72df40..d04409d6c8f 100644 --- a/app/controllers/perps/providers/HyperLiquidProvider.ts +++ b/app/controllers/perps/providers/HyperLiquidProvider.ts @@ -5086,6 +5086,20 @@ export class HyperLiquidProvider implements PerpsProvider { count: rawFills?.length ?? 0, }); + // Start fetching historical orders in parallel with fill transformation. + // The fills API does not return order type, so we cross-reference + // with historical orders to enable TP/SL pill rendering in activity. + const historicalOrdersPromise = ( + infoClient.historicalOrders?.({ user: userAddress }) ?? + Promise.resolve(null) + ).catch((enrichError: unknown) => { + this.#deps.debugLogger.log( + 'Warning: failed to enrich fills with order types:', + enrichError, + ); + return null; + }); + // Transform HyperLiquid fills to abstract OrderFill type const fills = (rawFills || []).reduce((acc: OrderFill[], fill) => { // Perps only, no Spots @@ -5116,6 +5130,32 @@ export class HyperLiquidProvider implements PerpsProvider { return acc; }, []); + // Enrich fills with detailedOrderType from historical orders + // Wrapped in its own try/catch so a malformed order never discards fetched fills + try { + const rawOrders = await historicalOrdersPromise; + if (rawOrders) { + const orderTypeByOid = new Map(); + for (const rawOrder of rawOrders) { + const oid = rawOrder.order?.oid?.toString(); + if (oid && rawOrder.order?.orderType && !orderTypeByOid.has(oid)) { + orderTypeByOid.set(oid, rawOrder.order.orderType); + } + } + for (const fill of fills) { + const orderType = orderTypeByOid.get(fill.orderId); + if (orderType) { + fill.detailedOrderType = orderType; + } + } + } + } catch (enrichError) { + this.#deps.debugLogger.log( + 'Error enriching fills with order types:', + enrichError, + ); + } + return fills; } catch (error) { this.#deps.debugLogger.log('Error getting user fills:', error); diff --git a/app/controllers/perps/services/HyperLiquidSubscriptionService.test.ts b/app/controllers/perps/services/HyperLiquidSubscriptionService.test.ts index 95b4801741d..6a1923f0501 100644 --- a/app/controllers/perps/services/HyperLiquidSubscriptionService.test.ts +++ b/app/controllers/perps/services/HyperLiquidSubscriptionService.test.ts @@ -751,6 +751,62 @@ describe('HyperLiquidSubscriptionService', () => { unsubscribe(); }); + it('enriches WS fills with detailedOrderType from cached orders', async () => { + // Arrange — subscribe to orders first so #cachedOrders gets populated + const orderCallback = jest.fn(); + service.subscribeToOrders({ callback: orderCallback }); + await jest.runAllTimersAsync(); + + // Now subscribe to fills — the callback should enrich with cached order types + const fillCallback = jest.fn(); + mockSubscriptionClient.userFills.mockImplementation( + (_params: any, callback: any) => { + setTimeout(() => { + callback({ + fills: [ + { + oid: BigInt(12345), + coin: 'BTC', + side: 'B', + sz: '0.1', + px: '50000', + fee: '5', + time: Date.now(), + closedPnl: '0', + dir: 'Open Long', + feeToken: 'USDC', + startPosition: '0', + }, + ], + }); + }, 0); + return Promise.resolve({ + unsubscribe: jest.fn().mockResolvedValue(undefined), + }); + }, + ); + + // Act + const unsubscribe = service.subscribeToOrderFills({ + callback: fillCallback, + }); + await jest.runAllTimersAsync(); + + // Assert — fill received with orderId mapped and detailedOrderType enriched + expect(fillCallback).toHaveBeenCalledWith( + [ + expect.objectContaining({ + orderId: '12345', + symbol: 'BTC', + detailedOrderType: 'Limit', + }), + ], + undefined, + ); + + unsubscribe(); + }); + it('should pass isSnapshot flag to callback', async () => { const mockCallback = jest.fn(); diff --git a/app/controllers/perps/services/HyperLiquidSubscriptionService.ts b/app/controllers/perps/services/HyperLiquidSubscriptionService.ts index 5cedb6be657..a56c1a70f08 100644 --- a/app/controllers/perps/services/HyperLiquidSubscriptionService.ts +++ b/app/controllers/perps/services/HyperLiquidSubscriptionService.ts @@ -2115,26 +2115,39 @@ export class HyperLiquidSubscriptionService { const subscription = await subscriptionClient.userFills( { user: userAddress }, (data: UserFillsWsEvent) => { - const orderFills: OrderFill[] = data.fills.map((fill) => ({ - orderId: fill.oid.toString(), - symbol: fill.coin, - side: fill.side, - size: fill.sz, - price: fill.px, - fee: fill.fee, - timestamp: fill.time, - pnl: fill.closedPnl, - direction: fill.dir, - feeToken: fill.feeToken, - startPosition: fill.startPosition, - liquidation: fill.liquidation - ? { - liquidatedUser: fill.liquidation.liquidatedUser, - markPx: fill.liquidation.markPx, - method: fill.liquidation.method, - } - : undefined, - })); + // Build a Map for O(1) lookup instead of O(n) find per fill + const orderMap = new Map(); + if (this.#cachedOrders) { + for (const order of this.#cachedOrders) { + if (order.detailedOrderType) { + orderMap.set(order.orderId, order.detailedOrderType); + } + } + } + const orderFills: OrderFill[] = data.fills.map((fill) => { + const oid = fill.oid.toString(); + return { + orderId: oid, + symbol: fill.coin, + side: fill.side, + size: fill.sz, + price: fill.px, + fee: fill.fee, + timestamp: fill.time, + pnl: fill.closedPnl, + direction: fill.dir, + feeToken: fill.feeToken, + startPosition: fill.startPosition, + liquidation: fill.liquidation + ? { + liquidatedUser: fill.liquidation.liquidatedUser, + markPx: fill.liquidation.markPx, + method: fill.liquidation.method, + } + : undefined, + detailedOrderType: orderMap.get(oid), + }; + }); // Cache fills for cache-first pattern (similar to price caching) // This allows getOrFetchFills() to return cached data without REST API calls From 6fa43afd3c45c4baa5fdb15e99c0d43f8b73ddfb Mon Sep 17 00:00:00 2001 From: Michal Szorad Date: Wed, 25 Mar 2026 10:46:27 +0100 Subject: [PATCH 6/8] fix(perps): reduce max order amount by 0.5% buffer to avoid insufficient margin rejections (#27417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** When users place a long/short Perps order with the amount slider at 100% (or tap "Max"), the app was sending the full theoretical maximum (available balance × leverage). The HyperLiquid API sometimes rejects these with "Order 0: Insufficient margin to place order" due to fees, rounding, and exchange-side margin checks. **Solution:** Introduce a 0.5% margin buffer on the maximum order amount so that "100%" uses 99.5% of the theoretical max. This is applied in a single place (`getMaxAllowedAmount`), and the order form uses this buffered value for the slider max, Max button, and 100% quick button so all paths stay consistent. The buffer is configurable via `MAX_ORDER_MARGIN_BUFFER` in perps config for future tuning or smarter logic (e.g. fee-based). **Changes:** - **perpsConfig**: Added `MAX_ORDER_MARGIN_BUFFER = 0.005` (0.5%). - **getMaxAllowedAmount**: After existing rounding logic, multiply max by `(1 - MAX_ORDER_MARGIN_BUFFER)` and return `floor(bufferedMax)`. - **usePerpsOrderForm**: `handleMaxAmount` and `handlePercentageAmount(1)` now set amount to `maxPossibleAmount` (the buffered max) instead of computing `balance × leverage` directly. - **Tests**: Updated expectations for low-balance scenarios (e.g. $2 @ 3x → max 5 instead of 6); added test that max is below theoretical after buffer. ## **Changelog** CHANGELOG entry: Fixed Perps orders at 100% margin sometimes failing with "Insufficient margin" by applying a small buffer to the maximum order amount. ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/TAT-2502 ## **Manual testing steps** ```gherkin Feature: Perps order placement at maximum margin Scenario: user places order at 100% (slider or Max) without insufficient margin error Given user is on Perps order view with available balance and an asset selected When user sets amount to 100% via the slider or taps the Max / 100% button Then the amount field shows the buffered max (slightly below theoretical max) And placing the order does not result in "Insufficient margin" rejection from the exchange ``` ## **Screenshots/Recordings** ### **Before** N/A (behavioral fix; no UI change) ### **After** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --- > [!NOTE] > **Medium Risk** > Adjusts core order-sizing calculations for max/percentage selections, which can affect how much users trade and may surface edge cases around rounding and balance updates. Changes are localized and covered by updated/additional tests. > > **Overview** > Reduces Perps *maximum order amount* by applying a configurable **0.5% margin buffer** so “Max”/100% selections are less likely to be rejected as *Insufficient margin*. > > This adds `MAX_ORDER_MARGIN_BUFFER` and applies it in `getMaxAllowedAmount`, then updates `usePerpsOrderForm` handlers to clamp percentage-based amounts and set Max to `maxPossibleAmount` (the buffered max). Tests are updated to reflect new buffered expectations and add coverage for near-100% clamping and buffered-max behavior. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 03168c39998d0553215f516be14b28ac28430f90. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../UI/Perps/hooks/usePerpsOrderForm.test.ts | 42 ++++++++++++++----- .../UI/Perps/hooks/usePerpsOrderForm.ts | 18 ++++---- .../UI/Perps/utils/orderCalculations.test.ts | 21 ++++++++++ .../perps/constants/perpsConfig.ts | 8 ++++ .../perps/utils/orderCalculations.ts | 11 ++++- 5 files changed, 80 insertions(+), 20 deletions(-) diff --git a/app/components/UI/Perps/hooks/usePerpsOrderForm.test.ts b/app/components/UI/Perps/hooks/usePerpsOrderForm.test.ts index 08938264bcc..76675819661 100644 --- a/app/components/UI/Perps/hooks/usePerpsOrderForm.test.ts +++ b/app/components/UI/Perps/hooks/usePerpsOrderForm.test.ts @@ -435,9 +435,9 @@ describe('usePerpsOrderForm', () => { }); // Assert - // With $2 balance and 3x leverage = $6 max amount, which is less than $10 default - // Should use the max possible amount ($6) instead of the default ($10) - expect(result.current.orderForm.amount).toBe('6'); + // With $2 balance and 3x leverage, max is floor(6 * (1 - 0.5% buffer)) = 5 (less than $10 default) + // Should use the max possible amount (5) instead of the default ($10) + expect(result.current.orderForm.amount).toBe('5'); }); it('should use default amount when available balance times leverage is greater than default amount', () => { @@ -468,14 +468,14 @@ describe('usePerpsOrderForm', () => { describe('useMemo and useEffect behavior', () => { it('should not overwrite user input when dependencies change', async () => { - // Arrange - Start with balance high enough that max >= 999 (e.g. 334 * 3x = 1002) + // Arrange - Start with balance high enough that max >= 999 after 0.5% buffer (e.g. 335 * 3x → floor(1005*0.995) = 999) const mockAccount = { account: { - availableBalance: '334', + availableBalance: '335', marginUsed: '0', unrealizedPnl: '0', returnOnEquity: '0', - totalBalance: '334', + totalBalance: '335', }, isInitialLoading: false, }; @@ -490,7 +490,7 @@ describe('usePerpsOrderForm', () => { TRADING_DEFAULTS.amount.mainnet.toString(), ); - // Act - User changes the amount (within current max) + // Act - User changes the amount (within current max; 335*3*0.995 >= 999) act(() => { result.current.setAmount('999'); }); @@ -515,7 +515,7 @@ describe('usePerpsOrderForm', () => { // Test 1: Low balance scenario mockUsePerpsLiveAccount.mockReturnValue({ account: { - availableBalance: '2', // $2 balance = $6 max with 3x leverage (less than $10 default) + availableBalance: '2', // $2 balance, 3x leverage: max = floor(6 * 0.995) = 5 (less than $10 default) marginUsed: '0', unrealizedPnl: '0', returnOnEquity: '0', @@ -528,7 +528,7 @@ describe('usePerpsOrderForm', () => { wrapper: createWrapper(), }); - expect(result1.current.orderForm.amount).toBe('6'); // Should use maxPossibleAmount + expect(result1.current.orderForm.amount).toBe('5'); // Should use maxPossibleAmount (with margin buffer) // Test 2: High balance scenario mockUsePerpsLiveAccount.mockReturnValue({ @@ -690,7 +690,8 @@ describe('usePerpsOrderForm', () => { result.current.handleMaxAmount(); }); - expect(result.current.orderForm.amount).toBe('3000'); // 1000 * 3x leverage + // 1000 * 3x leverage with 0.5% margin buffer = floor(3000 * 0.995) = 2985 + expect(result.current.orderForm.amount).toBe('2985'); }); it('should handle min amount for mainnet', () => { @@ -722,6 +723,27 @@ describe('usePerpsOrderForm', () => { ); }); + it('should clamp near-100% amounts to maxPossibleAmount', () => { + const { result } = renderHook(() => usePerpsOrderForm(), { + wrapper: createWrapper(), + }); + + act(() => { + result.current.handlePercentageAmount(0.999); + }); + + const at999 = Number(result.current.orderForm.amount); + + act(() => { + result.current.handlePercentageAmount(1); + }); + + const at100 = Number(result.current.orderForm.amount); + + expect(at999).toBeLessThanOrEqual(at100); + expect(at100).toBe(result.current.maxPossibleAmount); + }); + it('should not update amount when balance is 0', () => { mockUsePerpsLiveAccount.mockReturnValue({ account: { diff --git a/app/components/UI/Perps/hooks/usePerpsOrderForm.ts b/app/components/UI/Perps/hooks/usePerpsOrderForm.ts index 1efff43fb74..05f021c00aa 100644 --- a/app/components/UI/Perps/hooks/usePerpsOrderForm.ts +++ b/app/components/UI/Perps/hooks/usePerpsOrderForm.ts @@ -317,26 +317,28 @@ export function usePerpsOrderForm( setOrderForm((prev) => ({ ...prev, type })); }; - // Handle percentage-based amount selection (respects custom token amount when set) + // Handle percentage-based amount selection (respects custom token amount when set). + // Clamp to maxPossibleAmount so near-100% values never exceed the buffered max. const handlePercentageAmount = useCallback( (percentage: number) => { if (balanceForMax === 0) return; - const newAmount = Math.floor( - balanceForMax * orderForm.leverage * percentage, - ).toString(); + const raw = balanceForMax * orderForm.leverage * percentage; + const clamped = Math.min(raw, maxPossibleAmount); + const newAmount = Math.floor(clamped).toString(); setOrderForm((prev) => ({ ...prev, amount: newAmount })); }, - [balanceForMax, orderForm.leverage], + [balanceForMax, orderForm.leverage, maxPossibleAmount], ); - // Handle max amount selection (respects custom token amount when set) + // Handle max amount selection (respects custom token amount when set). + // Uses maxPossibleAmount (includes margin buffer) to avoid "Insufficient margin" rejections. const handleMaxAmount = useCallback(() => { if (balanceForMax === 0) return; setOrderForm((prev) => ({ ...prev, - amount: Math.floor(balanceForMax * prev.leverage).toString(), + amount: Math.floor(maxPossibleAmount).toString(), })); - }, [balanceForMax]); + }, [balanceForMax, maxPossibleAmount]); // Handle min amount selection const handleMinAmount = useCallback(() => { diff --git a/app/components/UI/Perps/utils/orderCalculations.test.ts b/app/components/UI/Perps/utils/orderCalculations.test.ts index 02d3aa47856..fcec7b02062 100644 --- a/app/components/UI/Perps/utils/orderCalculations.test.ts +++ b/app/components/UI/Perps/utils/orderCalculations.test.ts @@ -355,6 +355,27 @@ describe('orderCalculations', () => { expect(result).toBeGreaterThanOrEqual(0); expect(result).toBeLessThanOrEqual(50); // 10 * 5 leverage }); + + it('should apply margin buffer so result is below theoretical max', () => { + // Arrange - case where theoretical max is 1000 (100 * 10) + const params = { + availableBalance: 100, + assetPrice: 50000, + assetSzDecimals: 6, + leverage: 10, + }; + + // Act + const result = getMaxAllowedAmount(params); + const theoreticalMax = params.availableBalance * params.leverage; + + // Assert - buffer (0.5%) reduces max to avoid "Insufficient margin" rejections + expect(result).toBeGreaterThan(0); + expect(result).toBeLessThanOrEqual(theoreticalMax); + expect(result).toBeLessThanOrEqual( + Math.floor(theoreticalMax * (1 - 0.005)), + ); + }); }); describe('buildOrdersArray', () => { diff --git a/app/controllers/perps/constants/perpsConfig.ts b/app/controllers/perps/constants/perpsConfig.ts index b1b76be7e71..9cb44b94c73 100644 --- a/app/controllers/perps/constants/perpsConfig.ts +++ b/app/controllers/perps/constants/perpsConfig.ts @@ -105,6 +105,14 @@ export const ORDER_SLIPPAGE_CONFIG = { DefaultLimitSlippageBps: 100, } as const; +/** + * Max order amount buffer to reduce "Insufficient margin" rejections from the exchange. + * When the user selects 100% (slider or Max), we cap the order at (1 - this) of the + * theoretical max so that fees, rounding, and exchange-side margin checks are covered. + * Value as decimal (e.g. 0.005 = 0.5%). + */ +export const MAX_ORDER_MARGIN_BUFFER = 0.005; // 0.5% + /** * Performance optimization constants * These values control debouncing and throttling for better performance diff --git a/app/controllers/perps/utils/orderCalculations.ts b/app/controllers/perps/utils/orderCalculations.ts index d6dffbed5d3..facfa45d3f3 100644 --- a/app/controllers/perps/utils/orderCalculations.ts +++ b/app/controllers/perps/utils/orderCalculations.ts @@ -4,7 +4,10 @@ import { formatHyperLiquidPrice, formatHyperLiquidSize, } from './hyperLiquidAdapter'; -import { ORDER_SLIPPAGE_CONFIG } from '../constants/perpsConfig'; +import { + MAX_ORDER_MARGIN_BUFFER, + ORDER_SLIPPAGE_CONFIG, +} from '../constants/perpsConfig'; import { PERPS_ERROR_CODES } from '../perpsErrorCodes'; import type { PerpsDebugLogger } from '../types'; import type { SDKOrderParams } from '../types/hyperliquid-types'; @@ -174,7 +177,11 @@ export function getMaxAllowedAmount(params: MaxAllowedAmountParams): number { maxAmount -= positionSizeIncrementUsd; } - return Math.max(0, maxAmount); + // Apply margin buffer to reduce "Insufficient margin" rejections from the exchange + // (fees, rounding, and exchange-side checks can make 100% theoretical max fail) + const bufferedMax = maxAmount * (1 - MAX_ORDER_MARGIN_BUFFER); + + return Math.max(0, Math.floor(bufferedMax)); } /** From 6df8a92dc26ecb2c979d71fb5f176663a9740375 Mon Sep 17 00:00:00 2001 From: Gaurav Goel Date: Wed, 25 Mar 2026 15:37:31 +0530 Subject: [PATCH 7/8] fix: biometric bug with incorrect password during rehydration (#27900) ## **Description** Seedless/OAuth rehydration was prompting biometrics with a wrong password entry too **Solution** Now the flow is password-first: unlockWallet runs with password only. After a successful unlock, post-unlock steps (including optional biometric upgrade). Failed unlocks no longer trigger biometric prompts. Jira: https://consensyssoftware.atlassian.net/browse/TO-600 ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/TO-600 ## **Manual testing steps** ```gherkin Feature: Seedless OAuth rehydration password before biometrics Scenario: Wrong password does not trigger biometrics before unlock Given the user is on the OAuth rehydration screen with a seedless wallet And device biometrics are available When the user enters an incorrect password and submits Then the app shows an invalid password error And the system biometric prompt is not shown before unlock fails Scenario: Correct password offers biometrics after successful unlock (rehydration) Given the user is on the OAuth rehydration screen And device biometrics are available When the user enters the correct password and submits Then unlock completes successfully And the app may prompt for device biometrics / keychain upgrade only after unlock succeeds ``` ## **Screenshots/Recordings** ### **Before** ### **After** https://github.com/user-attachments/assets/e5ade971-0f7e-4316-b272-9f2aa7c58fb8 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --- > [!NOTE] > **Medium Risk** > Changes the seedless/OAuth rehydration login flow and post-unlock auth preference updates, which can affect authentication UX and keychain storage behavior. Scope is limited to `OAuthRehydration` and adds/adjusts tests to cover ordering and failure cases. > > **Overview** > Updates `OAuthRehydration` so biometric/device-auth prompts no longer occur *before* password verification: `unlockWallet` is called with `currentAuthType: PASSWORD`, and an optional post-unlock step (`upgradeKeychainAuthAfterSuccessfulUnlock`) requests device auth and persists the result via `updateAuthPreference`. > > Extends `OAuthRehydration` tests to assert call ordering (unlock precedes biometrics) and to ensure biometrics/auth-preference updates are not triggered when password unlock fails, including the outdated-password flow. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f0c39635adc100eb6e068a79bd21198a2b36986a. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .../Views/OAuthRehydration/index.test.tsx | 77 +++++++++++++++++++ .../Views/OAuthRehydration/index.tsx | 60 ++++++++++----- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/app/components/Views/OAuthRehydration/index.test.tsx b/app/components/Views/OAuthRehydration/index.test.tsx index 04ed0b58c6a..736439227b1 100644 --- a/app/components/Views/OAuthRehydration/index.test.tsx +++ b/app/components/Views/OAuthRehydration/index.test.tsx @@ -36,6 +36,7 @@ const mockReauthenticate = jest.fn(); const mockRevealSRP = jest.fn(); const mockRevealPrivateKey = jest.fn(); const mockRequestBiometricsAccessControlForIOS = jest.fn(); +const mockUpdateAuthPreference = jest.fn(); jest.mock('../../../core/Authentication/hooks/useAuthentication', () => ({ __esModule: true, @@ -49,6 +50,7 @@ jest.mock('../../../core/Authentication/hooks/useAuthentication', () => ({ revealPrivateKey: mockRevealPrivateKey, requestBiometricsAccessControlForIOS: mockRequestBiometricsAccessControlForIOS, + updateAuthPreference: mockUpdateAuthPreference, }), })); @@ -191,6 +193,7 @@ describe('OAuthRehydration', () => { mockRequestBiometricsAccessControlForIOS.mockResolvedValue( AUTHENTICATION_TYPE.PASSWORD, ); + mockUpdateAuthPreference.mockResolvedValue(undefined); mockUseNetInfo.mockReturnValue({ isConnected: true, isInternetReachable: true, @@ -220,6 +223,9 @@ describe('OAuthRehydration', () => { await waitFor(() => { expect(mockReplace).toHaveBeenCalledWith(Routes.ONBOARDING.HOME_NAV); }); + expect(mockUnlockWallet.mock.invocationCallOrder[0]).toBeLessThan( + mockRequestBiometricsAccessControlForIOS.mock.invocationCallOrder[0], + ); expect(mockRequestBiometricsAccessControlForIOS).toHaveBeenCalledWith( AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, ); @@ -233,6 +239,47 @@ describe('OAuthRehydration', () => { expect(mockTrackOnboarding).toHaveBeenCalled(); }); }); + + it('logs error when post-unlock biometric prompt fails', async () => { + const biometricError = new Error('Biometric prompt failed'); + mockRequestBiometricsAccessControlForIOS.mockRejectedValueOnce( + biometricError, + ); + + const { getByTestId } = renderWithProvider(); + await enterPasswordAndSubmit(getByTestId); + + await waitFor(() => { + expect(mockUnlockWallet).toHaveBeenCalled(); + }); + await waitFor(() => { + expect(Logger.error).toHaveBeenCalledWith( + biometricError, + 'OAuthRehydration: post-unlock biometric preference', + ); + }); + }); + + it('logs error when updateAuthPreference fails after choosing device auth', async () => { + mockRequestBiometricsAccessControlForIOS.mockResolvedValueOnce( + AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, + ); + const preferenceError = new Error('Keychain preference update failed'); + mockUpdateAuthPreference.mockRejectedValueOnce(preferenceError); + + const { getByTestId } = renderWithProvider(); + await enterPasswordAndSubmit(getByTestId); + + await waitFor(() => { + expect(mockUpdateAuthPreference).toHaveBeenCalled(); + }); + await waitFor(() => { + expect(Logger.error).toHaveBeenCalledWith( + preferenceError, + 'OAuthRehydration: post-unlock biometric preference', + ); + }); + }); }); describe('Password validation', () => { @@ -246,6 +293,18 @@ describe('OAuthRehydration', () => { }); }); + it('does not prompt biometrics when password unlock fails', async () => { + mockUnlockWallet.mockRejectedValue(new Error('Error: Decrypt failed')); + const { getByTestId } = renderWithProvider(); + await enterPasswordAndSubmit(getByTestId, 'wrongPassword'); + + await waitFor(() => { + expect(getByTestId(LoginViewSelectors.PASSWORD_ERROR)).toBeTruthy(); + }); + expect(mockRequestBiometricsAccessControlForIOS).not.toHaveBeenCalled(); + expect(mockUpdateAuthPreference).not.toHaveBeenCalled(); + }); + it('clears error when user types new password', async () => { mockUnlockWallet.mockRejectedValue(new Error('Error: Decrypt failed')); const { getByTestId } = renderWithProvider(); @@ -858,10 +917,28 @@ describe('OAuthRehydration', () => { expect.objectContaining({ authPreference: expect.objectContaining({ oauth2Login: false, + currentAuthType: AUTHENTICATION_TYPE.PASSWORD, }), }), ); }); + expect(mockUnlockWallet.mock.invocationCallOrder[0]).toBeLessThan( + mockRequestBiometricsAccessControlForIOS.mock.invocationCallOrder[0], + ); + expect(mockRequestBiometricsAccessControlForIOS).toHaveBeenCalledWith( + AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, + ); + }); + + it('does not prompt biometrics before unlock when password is wrong (outdated password flow)', async () => { + mockUnlockWallet.mockRejectedValue(new Error('Error: Decrypt failed')); + const { getByTestId } = renderWithProvider(); + await enterPasswordAndSubmit(getByTestId, 'wrongPassword'); + + await waitFor(() => { + expect(getByTestId(LoginViewSelectors.PASSWORD_ERROR)).toBeTruthy(); + }); + expect(mockRequestBiometricsAccessControlForIOS).not.toHaveBeenCalled(); }); it('navigates to DELETE_WALLET modal on forgot password press', () => { diff --git a/app/components/Views/OAuthRehydration/index.tsx b/app/components/Views/OAuthRehydration/index.tsx index 28361a07ad7..b5af1054076 100644 --- a/app/components/Views/OAuthRehydration/index.tsx +++ b/app/components/Views/OAuthRehydration/index.tsx @@ -150,8 +150,38 @@ const OAuthRehydration: React.FC = ({ const passwordLoginAttemptTraceCtxRef = useRef(null); - const { unlockWallet, getAuthType, requestBiometricsAccessControlForIOS } = - useAuthentication(); + const { + unlockWallet, + getAuthType, + requestBiometricsAccessControlForIOS, + updateAuthPreference, + } = useAuthentication(); + + /** + * After a successful password unlock, offer device auth / biometrics for keychain storage. + */ + const upgradeKeychainAuthAfterSuccessfulUnlock = useCallback(async () => { + try { + const upgradeAuthType = await requestBiometricsAccessControlForIOS( + AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, + ); + if (upgradeAuthType !== AUTHENTICATION_TYPE.PASSWORD) { + await updateAuthPreference({ + authType: upgradeAuthType, + password, + fallbackToPassword: true, + }); + } + } catch (postUnlockAuthErr) { + Logger.error( + ensureError( + postUnlockAuthErr, + 'Post-unlock auth preference update failed', + ), + 'OAuthRehydration: post-unlock biometric preference', + ); + } + }, [password, requestBiometricsAccessControlForIOS, updateAuthPreference]); const track = useCallback( ( @@ -485,14 +515,9 @@ const OAuthRehydration: React.FC = ({ setLoading(true); - // Ask user to allow biometrics access control - const authType = await requestBiometricsAccessControlForIOS( - AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, - ); - - // Only set oauth2Login for normal rehydration, not when password is outdated + // Password first: do not prompt biometrics until unlock succeeds const authData: AuthData = { - currentAuthType: authType, + currentAuthType: AUTHENTICATION_TYPE.PASSWORD, oauth2Login: true, }; @@ -506,6 +531,8 @@ const OAuthRehydration: React.FC = ({ }, ); + await upgradeKeychainAuthAfterSuccessfulUnlock(); + // Best-effort post-unlock UX: show biometric cancelled alert if needed. // Failure here must not be treated as a login error — unlock already succeeded. try { @@ -542,7 +569,7 @@ const OAuthRehydration: React.FC = ({ track, promptBiometricFailedAlert, unlockWallet, - requestBiometricsAccessControlForIOS, + upgradeKeychainAuthAfterSuccessfulUnlock, ]); const newGlobalPasswordLogin = useCallback(async () => { @@ -551,14 +578,9 @@ const OAuthRehydration: React.FC = ({ setLoading(true); - // Ask user to allow biometrics access control - const authType = await requestBiometricsAccessControlForIOS( - AUTHENTICATION_TYPE.DEVICE_AUTHENTICATION, - ); - - // Only set oauth2Login for normal rehydration, not when password is outdated + // biometrics/passcode preference is applied only after sync succeeds const authData: AuthData = { - currentAuthType: authType, + currentAuthType: AUTHENTICATION_TYPE.PASSWORD, oauth2Login: false, }; @@ -572,6 +594,8 @@ const OAuthRehydration: React.FC = ({ }, ); + await upgradeKeychainAuthAfterSuccessfulUnlock(); + // Best-effort post-unlock UX: show biometric cancelled alert if needed. // Failure here must not be treated as a login error — unlock already succeeded. try { @@ -593,7 +617,7 @@ const OAuthRehydration: React.FC = ({ handleLoginError, promptBiometricFailedAlert, unlockWallet, - requestBiometricsAccessControlForIOS, + upgradeKeychainAuthAfterSuccessfulUnlock, ]); // Cleanup for isMountedRef tracking From 62f61361af00fc43376906fd36552de04d71cdaf Mon Sep 17 00:00:00 2001 From: abretonc7s <107169956+abretonc7s@users.noreply.github.com> Date: Wed, 25 Mar 2026 18:36:04 +0800 Subject: [PATCH 8/8] =?UTF-8?q?feat(perps):=20core=20resolver=20=E2=80=94?= =?UTF-8?q?=20providerCredentials,=20builder=20fee=20injection,=20env=20va?= =?UTF-8?q?r=20centralization=20(#27899)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Resolves several core-parity and architecture concerns for the perps controller: ### 1. Backport core-only changes (existing) - **`stopEligibilityMonitoring()`** — Disables geo-blocking eligibility checks when `useExternalServices` is toggled off. - **Dynamic MYX import** — `await import()` → `.then()/.catch()` to avoid bundling heavy MYX dependencies in extension. ### 2. Nested `providerCredentials` on `PerpsControllerConfig` - Restructures flat MYX config fields (`myxAppIdTestnet`, `myxProviderEnabled`, etc.) into `providerCredentials.myx.*`. - Adds `providerCredentials.hyperliquid.*` for builder fee wallet addresses. - New types: `PerpsProviderCredentials`, `HyperLiquidCredentials`, `MYXCredentials`. ### 3. Builder fee address injection - `HyperLiquidProvider` accepts optional `builderAddressTestnet`/`builderAddressMainnet` via constructor. - Falls back to hardcoded `BUILDER_FEE_CONFIG` defaults when env vars are empty. - New env vars in `.js.env.example`: `MM_PERPS_HL_BUILDER_ADDRESS_TESTNET`, `MM_PERPS_HL_BUILDER_ADDRESS_MAINNET`. ### 4. Env var centralization in mobile adapter - New `createMobileClientConfig()` in `mobileInfrastructure.ts` — all `process.env.*` reads in one place. - Engine init (`perps-controller/index.ts`) reduced from ~73 to ~37 lines — pure controller wiring, no env var reads. ### 5. MYX dynamic import race condition fix - `await import()` inside non-async `#createProviders(): void` → `.then()/.catch()` chain. - Removes `@ts-expect-error` suppression. Fixes provider setup race condition flagged by bugbot. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: N/A — architecture cleanup + core parity ## **Manual testing steps** ```gherkin Feature: Provider credentials and builder fee injection Scenario: HyperLiquid uses env-var builder address when set Given MM_PERPS_HL_BUILDER_ADDRESS_TESTNET is set in .js.env When a trade is placed on testnet Then the builder fee uses the env-var address (not hardcoded default) Scenario: HyperLiquid falls back to default when env var is empty Given MM_PERPS_HL_BUILDER_ADDRESS_TESTNET is empty When a trade is placed on testnet Then the builder fee uses BUILDER_FEE_CONFIG.TestnetBuilder Scenario: MYX provider registers via dynamic import Given MYX provider is enabled When PerpsController initializes Then MYX registers asynchronously via .then()/.catch() And initialization completes without waiting for MYX Scenario: Engine init uses adapter for config Given the app starts When PerpsController is initialized Then clientConfig comes from createMobileClientConfig() And no process.env reads exist in the Engine init file ``` ## **Screenshots/Recordings** N/A — no UI changes ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] 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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --- > [!NOTE] > **Medium Risk** > Touches Perps provider initialization/selection and geo-eligibility monitoring, which can impact trading availability and protocol routing if misconfigured. Changes are largely additive but include async/dynamic-import ordering and new config wiring that should be validated across networks/providers. > > **Overview** > **Refactors Perps controller configuration to use a nested `providerCredentials` structure** and centralizes all Perps `process.env` reads into `createMobileClientConfig()`, simplifying `perpsControllerInit` to pure wiring. > > **Adds HyperLiquid builder-fee address injection** via new env vars and passes these through `PerpsController` into `HyperLiquidProvider`, falling back to hardcoded defaults when env values are empty. > > **Hardens MYX provider registration and eligibility controls** by switching MYX to a dynamic `import()` flow with explicit error handling/awaiting during initialization, adding `stopEligibilityMonitoring()` (and messenger action typing) to defer geolocation checks, and extending tests to cover these behaviors. > > Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 30f6e0a578d6d10e7876d8955d8b5264919353cc. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot). --- .js.env.example | 3 + .../adapters/mobileInfrastructure.test.ts | 64 +++- .../UI/Perps/adapters/mobileInfrastructure.ts | 40 +++ .../PerpsController-method-action-types.ts | 8 +- app/controllers/perps/PerpsController.test.ts | 195 +++++++++++ app/controllers/perps/PerpsController.ts | 218 +++++++++--- .../providers/HyperLiquidProvider.test.ts | 38 ++- .../perps/providers/HyperLiquidProvider.ts | 18 +- app/controllers/perps/types/index.ts | 38 ++- .../perps-controller/index.test.ts | 1 + .../controllers/perps-controller/index.ts | 31 +- docs/perps/perps-refactoring-plan.md | 313 ++++++++++++++++++ scripts/perps/validate-core-sync.sh | 11 - 13 files changed, 872 insertions(+), 106 deletions(-) create mode 100644 docs/perps/perps-refactoring-plan.md diff --git a/.js.env.example b/.js.env.example index a6a75452422..ec8e61873c7 100644 --- a/.js.env.example +++ b/.js.env.example @@ -197,6 +197,9 @@ export MM_PERPS_MYX_BROKER_ADDRESS_TESTNET="" export MM_PERPS_MYX_APP_ID_MAINNET="" export MM_PERPS_MYX_API_SECRET_MAINNET="" export MM_PERPS_MYX_BROKER_ADDRESS_MAINNET="" +# HyperLiquid builder fee wallet addresses (empty = uses hardcoded defaults) +export MM_PERPS_HL_BUILDER_ADDRESS_TESTNET="" +export MM_PERPS_HL_BUILDER_ADDRESS_MAINNET="" # HIP-3 Feature Flags (remote override with local fallback) export MM_PERPS_HIP3_ENABLED="true" export MM_PERPS_HIP3_ALLOWLIST_MARKETS="" # Allowlist: Empty = enable all markets. Examples: "xyz:XYZ100,xyz:TSLA" or "xyz:*,abc:TSLA" diff --git a/app/components/UI/Perps/adapters/mobileInfrastructure.test.ts b/app/components/UI/Perps/adapters/mobileInfrastructure.test.ts index 01922988de3..1c85ba1fc9a 100644 --- a/app/components/UI/Perps/adapters/mobileInfrastructure.test.ts +++ b/app/components/UI/Perps/adapters/mobileInfrastructure.test.ts @@ -3,7 +3,10 @@ import { AnalyticsEventBuilder } from '../../../../util/analytics/AnalyticsEvent import { analytics } from '../../../../util/analytics/analytics'; import Logger from '../../../../util/Logger'; import type { PerpsAnalyticsEvent } from '@metamask/perps-controller'; -import { createMobileInfrastructure } from './mobileInfrastructure'; +import { + createMobileInfrastructure, + createMobileClientConfig, +} from './mobileInfrastructure'; import Engine from '../../../../core/Engine'; jest.mock('../../../../util/analytics/analytics', () => ({ @@ -212,3 +215,62 @@ describe('createMobileInfrastructure', () => { }); }); }); + +describe('createMobileClientConfig', () => { + it('returns default config with empty strings and arrays when no env vars are set', () => { + // Arrange — ensure relevant env vars are absent + const envVars = [ + 'MM_PERPS_BLOCKED_REGIONS', + 'MM_PERPS_HIP3_ENABLED', + 'MM_PERPS_HIP3_ALLOWLIST_MARKETS', + 'MM_PERPS_HIP3_BLOCKLIST_MARKETS', + 'MM_PERPS_HL_BUILDER_ADDRESS_TESTNET', + 'MM_PERPS_HL_BUILDER_ADDRESS_MAINNET', + 'MM_PERPS_MYX_PROVIDER_ENABLED', + 'MM_PERPS_MYX_APP_ID_TESTNET', + 'MM_PERPS_MYX_API_SECRET_TESTNET', + 'MM_PERPS_MYX_BROKER_ADDRESS_TESTNET', + 'MM_PERPS_MYX_APP_ID_MAINNET', + 'MM_PERPS_MYX_API_SECRET_MAINNET', + 'MM_PERPS_MYX_BROKER_ADDRESS_MAINNET', + ]; + const saved: Record = {}; + for (const key of envVars) { + saved[key] = process.env[key]; + delete process.env[key]; + } + + // Act + const config = createMobileClientConfig(); + + // Assert + expect(config).toEqual({ + fallbackBlockedRegions: [], + fallbackHip3Enabled: false, + fallbackHip3AllowlistMarkets: [], + fallbackHip3BlocklistMarkets: [], + providerCredentials: { + hyperliquid: { + builderAddressTestnet: '', + builderAddressMainnet: '', + }, + myx: { + enabled: false, + appIdTestnet: '', + apiSecretTestnet: '', + brokerAddressTestnet: '', + appIdMainnet: '', + apiSecretMainnet: '', + brokerAddressMainnet: '', + }, + }, + }); + + // Restore + for (const key of envVars) { + if (saved[key] !== undefined) { + process.env[key] = saved[key]; + } + } + }); +}); diff --git a/app/components/UI/Perps/adapters/mobileInfrastructure.ts b/app/components/UI/Perps/adapters/mobileInfrastructure.ts index 5dbf7f24858..9bd0eee5198 100644 --- a/app/components/UI/Perps/adapters/mobileInfrastructure.ts +++ b/app/components/UI/Perps/adapters/mobileInfrastructure.ts @@ -21,7 +21,9 @@ import { getStreamManagerInstance } from '../providers/PerpsStreamManager'; import Engine from '../../../../core/Engine'; import { PERPS_CONSTANTS, + parseCommaSeparatedString, type PerpsPlatformDependencies, + type PerpsControllerConfig, type PerpsMetrics, type PerpsTraceName, type PerpsTraceValue, @@ -155,6 +157,44 @@ function createCacheInvalidatorAdapter() { }; } +/** + * Creates mobile-specific client config from environment variables. + * Centralizes all process.env reads so the Engine init file stays pure wiring. + */ +export function createMobileClientConfig(): PerpsControllerConfig { + return { + fallbackBlockedRegions: parseCommaSeparatedString( + process.env.MM_PERPS_BLOCKED_REGIONS ?? '', + ), + fallbackHip3Enabled: process.env.MM_PERPS_HIP3_ENABLED === 'true', + fallbackHip3AllowlistMarkets: parseCommaSeparatedString( + process.env.MM_PERPS_HIP3_ALLOWLIST_MARKETS ?? '', + ), + fallbackHip3BlocklistMarkets: parseCommaSeparatedString( + process.env.MM_PERPS_HIP3_BLOCKLIST_MARKETS ?? '', + ), + providerCredentials: { + hyperliquid: { + builderAddressTestnet: + process.env.MM_PERPS_HL_BUILDER_ADDRESS_TESTNET ?? '', + builderAddressMainnet: + process.env.MM_PERPS_HL_BUILDER_ADDRESS_MAINNET ?? '', + }, + myx: { + enabled: process.env.MM_PERPS_MYX_PROVIDER_ENABLED === 'true', + appIdTestnet: process.env.MM_PERPS_MYX_APP_ID_TESTNET ?? '', + apiSecretTestnet: process.env.MM_PERPS_MYX_API_SECRET_TESTNET ?? '', + brokerAddressTestnet: + process.env.MM_PERPS_MYX_BROKER_ADDRESS_TESTNET ?? '', + appIdMainnet: process.env.MM_PERPS_MYX_APP_ID_MAINNET ?? '', + apiSecretMainnet: process.env.MM_PERPS_MYX_API_SECRET_MAINNET ?? '', + brokerAddressMainnet: + process.env.MM_PERPS_MYX_BROKER_ADDRESS_MAINNET ?? '', + }, + }, + }; +} + /** * Creates mobile-specific platform dependencies for PerpsController. * Controller access uses messenger pattern (messenger.call()). diff --git a/app/controllers/perps/PerpsController-method-action-types.ts b/app/controllers/perps/PerpsController-method-action-types.ts index 6231b294c9f..4237da1c649 100644 --- a/app/controllers/perps/PerpsController-method-action-types.ts +++ b/app/controllers/perps/PerpsController-method-action-types.ts @@ -175,6 +175,11 @@ export type PerpsControllerStartEligibilityMonitoringAction = { handler: PerpsController['startEligibilityMonitoring']; }; +export type PerpsControllerStopEligibilityMonitoringAction = { + type: 'PerpsController:stopEligibilityMonitoring'; + handler: PerpsController['stopEligibilityMonitoring']; +}; + export type PerpsControllerMethodActions = | PerpsControllerPlaceOrderAction | PerpsControllerEditOrderAction @@ -210,4 +215,5 @@ export type PerpsControllerMethodActions = | PerpsControllerSaveOrderBookGroupingAction | PerpsControllerSetSelectedPaymentTokenAction | PerpsControllerResetSelectedPaymentTokenAction - | PerpsControllerStartEligibilityMonitoringAction; + | PerpsControllerStartEligibilityMonitoringAction + | PerpsControllerStopEligibilityMonitoringAction; diff --git a/app/controllers/perps/PerpsController.test.ts b/app/controllers/perps/PerpsController.test.ts index 9d190be6321..7856676af95 100644 --- a/app/controllers/perps/PerpsController.test.ts +++ b/app/controllers/perps/PerpsController.test.ts @@ -24,6 +24,8 @@ import { PerpsController, getDefaultPerpsControllerState, InitializationState, + firstNonEmpty, + resolveMyxAuthConfig, } from './PerpsController'; import type { PerpsControllerState } from './PerpsController'; import { PERPS_ERROR_CODES } from './perpsErrorCodes'; @@ -387,6 +389,16 @@ class TestablePerpsController extends PerpsController { public testHasStandaloneProvider(): boolean { return this.hasStandaloneProvider(); } + + public testRegisterMYXProvider( + MYXProvider: new (opts: Record) => PerpsProvider, + ) { + this.registerMYXProvider(MYXProvider as never); + } + + public testHandleMYXImportError(error: unknown) { + this.handleMYXImportError(error); + } } describe('PerpsController', () => { @@ -800,6 +812,35 @@ describe('PerpsController', () => { }), ); }); + + it('stopEligibilityMonitoring defers subsequent refreshEligibility calls', async () => { + // Arrange — controller without deferral + const testMockCall = jest.fn().mockImplementation((action: string) => { + if (action === 'RemoteFeatureFlagController:getState') { + return { remoteFeatureFlags: {} }; + } + if (action === 'GeolocationController:getGeolocation') { + return 'US'; + } + return undefined; + }); + + const testController = new TestablePerpsController({ + messenger: createMockMessenger({ call: testMockCall }), + state: getDefaultPerpsControllerState(), + infrastructure: createMockInfrastructure(), + }); + testMockCall.mockClear(); + + // Act + testController.stopEligibilityMonitoring(); + await testController.refreshEligibility(); + + // Assert — geolocation was never called + expect(testMockCall).not.toHaveBeenCalledWith( + 'GeolocationController:getGeolocation', + ); + }); }); describe('HIP-3 Configuration Integration', () => { @@ -4551,6 +4592,17 @@ describe('PerpsController', () => { providers.set('myx', mockMYXProvider as any); myxController.testSetProviders(providers); + // Mock init on the reinit call inside switchProvider. + // Dynamic import() rejects in Jest (no --experimental-vm-modules), + // so MYX can't register via #createProviders. Mock init to + // simulate successful reinitialization while preserving our + // manually-injected MYX provider in the map. + jest.spyOn(myxController, 'init').mockImplementationOnce(async () => { + myxController.testUpdate((state) => { + state.initializationState = InitializationState.Initialized; + }); + }); + const result = await myxController.switchProvider('myx'); expect(result.success).toBe(true); @@ -4643,6 +4695,59 @@ describe('PerpsController', () => { // The init path should detect MYX is not available and fall back expect(controller.state.activeProvider).toBe('hyperliquid'); }); + + it('registerMYXProvider creates and registers the MYX provider', () => { + // Arrange + const mockMYXInstance = createMockHyperLiquidProvider(); + const MockMYXConstructor = jest.fn(() => mockMYXInstance); + + // Act + controller.testRegisterMYXProvider( + MockMYXConstructor as unknown as new ( + opts: Record, + ) => PerpsProvider, + ); + + // Assert + const providers = controller.testGetProviders(); + expect(providers.get('myx')).toBe(mockMYXInstance); + expect(MockMYXConstructor).toHaveBeenCalledWith( + expect.objectContaining({ isTestnet: false }), + ); + }); + + it('handleMYXImportError logs debug for MODULE_NOT_FOUND errors', () => { + // Arrange — Node sets code: 'MODULE_NOT_FOUND' on missing modules + const moduleError = Object.assign( + new Error('Cannot find module ./providers/MYXProvider'), + { code: 'MODULE_NOT_FOUND' }, + ); + + // Act + controller.testHandleMYXImportError(moduleError); + + // Assert + expect(mockInfrastructure.debugLogger.log).toHaveBeenCalledWith( + 'PerpsController: MYX provider module not available, skipping registration', + ); + }); + + it('handleMYXImportError routes runtime errors to logError', () => { + // Act — error without MODULE_NOT_FOUND code goes to Sentry + controller.testHandleMYXImportError(new Error('Invalid auth config')); + + // Assert + expect(mockInfrastructure.logger.error).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Invalid auth config' }), + expect.objectContaining({ + context: expect.objectContaining({ + data: expect.objectContaining({ + method: 'createProviders.myx', + }), + }), + }), + ); + }); }); describe('getOpenOrders with standalone mode', () => { @@ -5675,3 +5780,93 @@ describe('PerpsController', () => { }); }); }); + +describe('firstNonEmpty', () => { + it('returns the first non-empty string', () => { + expect(firstNonEmpty('', undefined, 'hello', 'world')).toBe('hello'); + }); + + it('returns empty string when all values are empty or undefined', () => { + expect(firstNonEmpty('', undefined, '')).toBe(''); + }); + + it('returns the first value if it is non-empty', () => { + expect(firstNonEmpty('first', 'second')).toBe('first'); + }); + + it('skips empty strings and returns the fallback', () => { + expect(firstNonEmpty('', 'fallback')).toBe('fallback'); + }); +}); + +describe('resolveMyxAuthConfig', () => { + it('uses testnet credentials on testnet', () => { + // Arrange + const myx = { + appIdTestnet: 'test-app', + apiSecretTestnet: 'test-secret', + brokerAddressTestnet: '0xTestBroker', + appIdMainnet: 'main-app', + apiSecretMainnet: 'main-secret', + brokerAddressMainnet: '0xMainBroker', + }; + + // Act + const result = resolveMyxAuthConfig(myx, true); + + // Assert + expect(result.appId).toBe('test-app'); + expect(result.apiSecret).toBe('test-secret'); + expect(result.brokerAddress).toBe('0xTestBroker'); + }); + + it('uses mainnet credentials on mainnet', () => { + // Arrange + const myx = { + appIdTestnet: 'test-app', + apiSecretTestnet: 'test-secret', + brokerAddressTestnet: '0xTestBroker', + appIdMainnet: 'main-app', + apiSecretMainnet: 'main-secret', + brokerAddressMainnet: '0xMainBroker', + }; + + // Act + const result = resolveMyxAuthConfig(myx, false); + + // Assert + expect(result.appId).toBe('main-app'); + expect(result.apiSecret).toBe('main-secret'); + expect(result.brokerAddress).toBe('0xMainBroker'); + }); + + it('falls back to testnet credentials when mainnet are empty', () => { + // Arrange + const myx = { + appIdTestnet: 'test-app', + apiSecretTestnet: 'test-secret', + brokerAddressTestnet: '0xTestBroker', + appIdMainnet: '', + apiSecretMainnet: '', + brokerAddressMainnet: '', + }; + + // Act + const result = resolveMyxAuthConfig(myx, false); + + // Assert + expect(result.appId).toBe('test-app'); + expect(result.apiSecret).toBe('test-secret'); + expect(result.brokerAddress).toBe('0xTestBroker'); + }); + + it('returns empty strings when no credentials are set', () => { + // Act + const result = resolveMyxAuthConfig({}, true); + + // Assert + expect(result.appId).toBe(''); + expect(result.apiSecret).toBe(''); + expect(result.brokerAddress).toBe(''); + }); +}); diff --git a/app/controllers/perps/PerpsController.ts b/app/controllers/perps/PerpsController.ts index 904c8130860..d22438bb3a4 100644 --- a/app/controllers/perps/PerpsController.ts +++ b/app/controllers/perps/PerpsController.ts @@ -27,7 +27,6 @@ import type { PerpsControllerMethodActions } from './PerpsController-method-acti import { PERPS_ERROR_CODES } from './perpsErrorCodes'; import { AggregatedPerpsProvider } from './providers/AggregatedPerpsProvider'; import { HyperLiquidProvider } from './providers/HyperLiquidProvider'; -import { MYXProvider } from './providers/MYXProvider'; import { AccountService } from './services/AccountService'; import { DataLakeService } from './services/DataLakeService'; import { DepositService } from './services/DepositService'; @@ -107,6 +106,7 @@ import type { PerpsRemoteFeatureFlagState, PerpsTransactionParams, PerpsAddTransactionOptions, + MYXCredentials, } from './types'; import type { PerpsControllerAllowedActions, @@ -124,6 +124,45 @@ import { wait } from './utils/wait'; /** Derived type for logger options from PerpsLogger interface */ type PerpsLoggerOptions = Parameters[1]; + +/** + * Returns the first non-empty string from the given values. + * Env vars default to '' (not null/undefined), so ?? wouldn't fall through. + * + * @param vals - String values to check in order. + * @returns The first non-empty string, or '' if all are empty/undefined. + */ +export function firstNonEmpty(...vals: (string | undefined)[]): string { + return ( + vals.find((val) => val !== null && val !== undefined && val !== '') ?? '' + ); +} + +/** + * Resolves MYX auth config from provider credentials, handling + * testnet/mainnet fallback logic. + * + * @param myx - MYX provider credentials. + * @param isTestnet - Whether the controller is in testnet mode. + * @returns Resolved appId, apiSecret, and brokerAddress. + */ +export function resolveMyxAuthConfig( + myx: MYXCredentials, + isTestnet: boolean, +): { appId: string; apiSecret: string; brokerAddress: string } { + return { + appId: isTestnet + ? (myx.appIdTestnet ?? '') + : firstNonEmpty(myx.appIdMainnet, myx.appIdTestnet), + apiSecret: isTestnet + ? (myx.apiSecretTestnet ?? '') + : firstNonEmpty(myx.apiSecretMainnet, myx.apiSecretTestnet), + brokerAddress: isTestnet + ? (myx.brokerAddressTestnet ?? '') + : firstNonEmpty(myx.brokerAddressMainnet, myx.brokerAddressTestnet), + }; +} + // PaymentToken: minimal interface for deposit flow (replaces mobile-only AssetType) /** @@ -639,6 +678,7 @@ const MESSENGER_EXPOSED_METHODS = [ 'setSelectedPaymentToken', 'resetSelectedPaymentToken', 'startEligibilityMonitoring', + 'stopEligibilityMonitoring', ] as const; /** @@ -662,6 +702,9 @@ export class PerpsController extends BaseController< #isReinitializing = false; + /** Tracks the async MYX dynamic import so performInitialization can await it. */ + #myxRegistrationPromise: Promise | null = null; + protected blockedRegionList: BlockedRegionList = { list: [], source: 'fallback', @@ -691,12 +734,12 @@ export class PerpsController extends BaseController< * @returns True if the condition is met. */ #isMYXProviderEnabled(): boolean { - const config = this.#options.clientConfig ?? {}; + const myx = this.#options.clientConfig?.providerCredentials?.myx; // Local env-var override (MM_PERPS_MYX_PROVIDER_ENABLED) always wins — // matches the UI selector (resolvePerpsMyxProviderEnabled) so controller // and UI agree on whether MYX is available. - if (config.myxProviderEnabled) { + if (myx?.enabled) { return true; } @@ -704,7 +747,7 @@ export class PerpsController extends BaseController< // Use || so empty-string env vars (default '') fall through. const hasCredentials = Boolean( // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - config.myxAppIdTestnet || config.myxAppIdMainnet, + myx?.appIdTestnet || myx?.appIdMainnet, ); if (hasCredentials) { @@ -1107,6 +1150,12 @@ export class PerpsController extends BaseController< blocklistMarkets: this.#hip3BlocklistMarkets, platformDependencies: this.#options.infrastructure, messenger: this.messenger, + builderAddressTestnet: + this.#options.clientConfig?.providerCredentials?.hyperliquid + ?.builderAddressTestnet, + builderAddressMainnet: + this.#options.clientConfig?.providerCredentials?.hyperliquid + ?.builderAddressMainnet, }); this.#standaloneProviderIsTestnet = currentIsTestnet; this.#standaloneProviderHip3Version = currentHip3Version; @@ -1444,8 +1493,16 @@ export class PerpsController extends BaseController< this.#createProviders(); - // Wait for WebSocket transport to be ready before marking as initialized - await wait(PERPS_CONSTANTS.ReconnectionCleanupDelayMs); + // Await MYX dynamic import (if started) so MYX is in the providers + // map before we assign the active provider. Runs concurrently with + // the WebSocket readiness delay for zero additional latency. + await Promise.all([ + wait(PERPS_CONSTANTS.ReconnectionCleanupDelayMs), + this.#myxRegistrationPromise, + ]); + this.#myxRegistrationPromise = null; + + this.#assignActiveProvider(); this.isInitialized = true; this.update((state) => { @@ -1533,53 +1590,106 @@ export class PerpsController extends BaseController< blocklistMarkets: this.#hip3BlocklistMarkets, platformDependencies: this.#options.infrastructure, messenger: this.messenger, + builderAddressTestnet: + this.#options.clientConfig?.providerCredentials?.hyperliquid + ?.builderAddressTestnet, + builderAddressMainnet: + this.#options.clientConfig?.providerCredentials?.hyperliquid + ?.builderAddressMainnet, }); this.providers.set('hyperliquid', hyperLiquidProvider); - // Register MYX provider if enabled via feature flag + // Register MYX provider if enabled via feature flag. + // Dynamic import because the MYX package pulls in heavy dependencies we + // don't want bundled in extension. Until MYX fixes their package, extension + // doesn't ship it — the catch branch silently skips registration. + // Uses .then()/.catch() instead of await because #createProviders is not async; + // MYX registration completing asynchronously is fine since it's only used when + // explicitly enabled and selected. const isMYXEnabled = this.#isMYXProviderEnabled(); if (isMYXEnabled) { - const myxIsTestnet = - PROVIDER_CONFIG.MYX_TESTNET_ONLY || this.state.isTestnet; - const config = this.#options.clientConfig ?? {}; - // When on mainnet, fall back to testnet credentials if mainnet ones are empty. - // Uses firstNonEmpty because env vars default to '' (not null/undefined), - // so ?? would not fall through on empty strings. - const firstNonEmpty = (...vals: (string | undefined)[]): string => - vals.find((val) => val !== null && val !== undefined && val !== '') ?? - ''; - const myxAppId = myxIsTestnet - ? (config.myxAppIdTestnet ?? '') - : firstNonEmpty(config.myxAppIdMainnet, config.myxAppIdTestnet); - const myxApiSecret = myxIsTestnet - ? (config.myxApiSecretTestnet ?? '') - : firstNonEmpty(config.myxApiSecretMainnet, config.myxApiSecretTestnet); - const myxBrokerAddress = myxIsTestnet - ? (config.myxBrokerAddressTestnet ?? '') - : firstNonEmpty( - config.myxBrokerAddressMainnet, - config.myxBrokerAddressTestnet, - ); - const myxProvider = new MYXProvider({ - isTestnet: myxIsTestnet, - platformDependencies: this.#options.infrastructure, - messenger: this.messenger, - myxAuthConfig: { - appId: myxAppId, - apiSecret: myxApiSecret, - brokerAddress: myxBrokerAddress, - }, - }); - this.providers.set('myx', myxProvider); - this.#debugLog('PerpsController: MYX provider registered', { - isTestnet: myxIsTestnet, - }); + // IMPORTANT: Must use import() — NOT require() — for core/extension tree-shaking. + // require() is synchronous and bundlers include it in the main bundle. + // import() enables true code splitting so MYX is excluded when not enabled. + this.#myxRegistrationPromise = import('./providers/MYXProvider') + .then(({ MYXProvider }) => { + this.registerMYXProvider(MYXProvider); + return undefined; + }) + .catch((error: unknown) => this.handleMYXImportError(error)); + } + } + + /** + * Registers the MYX provider after dynamic import resolves. + * + * Extracted from the import().then() callback so it can be tested directly + * (Jest cannot resolve dynamic imports without --experimental-vm-modules). + * + * @param MYXProvider - Constructor class for the MYX provider. + */ + protected registerMYXProvider( + MYXProvider: new (opts: { + isTestnet: boolean; + platformDependencies: PerpsPlatformDependencies; + messenger: PerpsControllerMessenger; + myxAuthConfig: ReturnType; + }) => PerpsProvider, + ): void { + const myxIsTestnet = + PROVIDER_CONFIG.MYX_TESTNET_ONLY || this.state.isTestnet; + const myx = this.#options.clientConfig?.providerCredentials?.myx ?? {}; + const myxAuthConfig = resolveMyxAuthConfig(myx, myxIsTestnet); + const myxProvider = new MYXProvider({ + isTestnet: myxIsTestnet, + platformDependencies: this.#options.infrastructure, + messenger: this.messenger, + myxAuthConfig, + }); + this.providers.set('myx', myxProvider); + this.#debugLog('PerpsController: MYX provider registered', { + isTestnet: myxIsTestnet, + }); + } + + /** + * Handles errors from the MYX dynamic import. + * + * Module-not-found errors are expected (extension doesn't ship MYX) → debug log. + * Other errors indicate constructor/config problems → Sentry via logError. + * + * @param error - The caught error from the dynamic import or constructor. + */ + protected handleMYXImportError(error: unknown): void { + const isModuleError = + (error as Record)?.code === 'MODULE_NOT_FOUND'; + if (isModuleError) { + this.#debugLog( + 'PerpsController: MYX provider module not available, skipping registration', + ); + } else { + this.#logError( + error instanceof Error ? error : new Error(String(error)), + this.#getErrorContext('createProviders.myx'), + ); + } + } + + /** + * Assigns the active provider instance based on the current activeProvider state. + * Separated from #createProviders so it runs after async MYX registration settles. + */ + #assignActiveProvider(): void { + const { activeProvider } = this.state; + const hyperLiquidProvider = this.providers.get('hyperliquid'); + + if (!hyperLiquidProvider) { + throw new Error( + 'HyperLiquid provider not registered — cannot assign active provider', + ); } - // Set up active provider based on activeProvider value in state - // 'aggregated' is treated as just another provider that wraps others if (activeProvider === 'aggregated') { - // Aggregated mode: wrap in AggregatedPerpsProvider for multi-provider support this.activeProviderInstance = new AggregatedPerpsProvider({ providers: this.providers, defaultProvider: 'hyperliquid', @@ -1590,20 +1700,17 @@ export class PerpsController extends BaseController< { registeredProviders: Array.from(this.providers.keys()) }, ); } else if (activeProvider === 'hyperliquid') { - // Direct provider mode: use HyperLiquid provider directly this.activeProviderInstance = hyperLiquidProvider; this.#debugLog( `PerpsController: Using direct provider (${activeProvider})`, ); } else if (activeProvider === 'myx') { - // MYX provider mode const myxProvider = this.providers.get('myx'); if (myxProvider) { this.activeProviderInstance = myxProvider; } else { - // MYX feature flag is disabled — fall back to HyperLiquid this.#debugLog( - 'PerpsController: MYX provider not available (feature flag disabled), falling back to hyperliquid', + 'PerpsController: MYX provider not available, falling back to hyperliquid', ); this.activeProviderInstance = hyperLiquidProvider; this.update((state) => { @@ -1614,7 +1721,6 @@ export class PerpsController extends BaseController< `PerpsController: Using direct provider (${this.activeProviderInstance === hyperLiquidProvider ? 'hyperliquid' : activeProvider})`, ); } else { - // Unsupported provider - throw error to prevent silent misconfiguration throw new Error( `Unsupported provider: ${String(activeProvider)}. Currently only 'hyperliquid', 'myx', and 'aggregated' are supported.`, ); @@ -1687,7 +1793,7 @@ export class PerpsController extends BaseController< }, stateManager: { update: (updater: (state: PerpsControllerState) => void) => - // @ts-expect-error TS2589 - excessively deep instantiation when inferring stateManager from BaseController + // @ts-expect-error TS2589 - excessively deep instantiation from BaseController generic this.update(updater), getState: (): PerpsControllerState => this.#getControllerState(), }, @@ -3952,6 +4058,16 @@ export class PerpsController extends BaseController< } } + /** + * Stops geo-blocking eligibility monitoring. + * Call this when the user disables basic functionality (e.g. useExternalServices becomes false). + * Prevents geolocation calls until startEligibilityMonitoring() is called again. + * Safe to call multiple times. + */ + stopEligibilityMonitoring(): void { + this.#eligibilityCheckDeferred = true; + } + async refreshEligibility(): Promise { if (this.#eligibilityCheckDeferred) { return; diff --git a/app/controllers/perps/providers/HyperLiquidProvider.test.ts b/app/controllers/perps/providers/HyperLiquidProvider.test.ts index 136e037c60e..444c5b48df4 100644 --- a/app/controllers/perps/providers/HyperLiquidProvider.test.ts +++ b/app/controllers/perps/providers/HyperLiquidProvider.test.ts @@ -5,7 +5,10 @@ import { createMockMessenger, } from '../../../components/UI/Perps/__mocks__/serviceMocks'; import { CandlePeriod } from '../constants/chartConfig'; -import { REFERRAL_CONFIG } from '../constants/hyperLiquidConfig'; +import { + BUILDER_FEE_CONFIG, + REFERRAL_CONFIG, +} from '../constants/hyperLiquidConfig'; import { PERPS_ERROR_CODES } from '../perpsErrorCodes'; import { HyperLiquidClientService } from '../services/HyperLiquidClientService'; import { HyperLiquidSubscriptionService } from '../services/HyperLiquidSubscriptionService'; @@ -5660,6 +5663,39 @@ describe('HyperLiquidProvider', () => { mockClientService.getExchangeClient().setReferrer, ).not.toHaveBeenCalled(); }); + + it('uses testnet builder address when in testnet mode', async () => { + // Arrange — flip to testnet mode + mockClientService.isTestnetMode.mockReturnValue(true); + + mockClientService.getInfoClient = jest.fn().mockReturnValue( + createMockInfoClient({ + maxBuilderFee: jest.fn().mockResolvedValue(1), // Already approved + }), + ); + + const orderParams: OrderParams = { + symbol: 'BTC', + isBuy: true, + size: '0.1', + orderType: 'market', + currentPrice: 50000, + }; + + // Act + const result = await provider.placeOrder(orderParams); + + // Assert — order placed with the testnet builder address + expect(result.success).toBe(true); + expect(mockClientService.getExchangeClient().order).toHaveBeenCalledWith( + expect.objectContaining({ + builder: { + b: BUILDER_FEE_CONFIG.TestnetBuilder, + f: expect.any(Number), + }, + }), + ); + }); }); // TODO: Refactor to test through public API — ES # private fields prevent direct access diff --git a/app/controllers/perps/providers/HyperLiquidProvider.ts b/app/controllers/perps/providers/HyperLiquidProvider.ts index d04409d6c8f..cb0d1bbc3a8 100644 --- a/app/controllers/perps/providers/HyperLiquidProvider.ts +++ b/app/controllers/perps/providers/HyperLiquidProvider.ts @@ -375,6 +375,10 @@ export class HyperLiquidProvider implements PerpsProvider { readonly #messenger: PerpsControllerMessengerBase; + readonly #builderAddressTestnet?: string; + + readonly #builderAddressMainnet?: string; + constructor(options: { isTestnet?: boolean; hip3Enabled?: boolean; @@ -384,9 +388,13 @@ export class HyperLiquidProvider implements PerpsProvider { platformDependencies: PerpsPlatformDependencies; messenger: PerpsControllerMessengerBase; initialAssetMapping?: [string, number][]; + builderAddressTestnet?: string; + builderAddressMainnet?: string; }) { this.#deps = options.platformDependencies; this.#messenger = options.messenger; + this.#builderAddressTestnet = options.builderAddressTestnet; + this.#builderAddressMainnet = options.builderAddressMainnet; const isTestnet = options.isTestnet ?? false; // Dev-friendly defaults: Enable all markets by default for easier testing (discovery mode) @@ -8075,9 +8083,13 @@ export class HyperLiquidProvider implements PerpsProvider { } #getBuilderAddress(isTestnet: boolean): string { - return isTestnet - ? BUILDER_FEE_CONFIG.TestnetBuilder - : BUILDER_FEE_CONFIG.MainnetBuilder; + // || intentional: env vars default to '' which must fall through to the hardcoded default + if (isTestnet) { + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + return this.#builderAddressTestnet || BUILDER_FEE_CONFIG.TestnetBuilder; + } + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + return this.#builderAddressMainnet || BUILDER_FEE_CONFIG.MainnetBuilder; } #getReferralCode(isTestnet: boolean): string { diff --git a/app/controllers/perps/types/index.ts b/app/controllers/perps/types/index.ts index 7b096606d1c..9dcfb312a78 100644 --- a/app/controllers/perps/types/index.ts +++ b/app/controllers/perps/types/index.ts @@ -596,21 +596,35 @@ export type PerpsControllerConfig = { fallbackHip3BlocklistMarkets?: string[]; /** - * MYX provider credentials. + * Per-provider credentials and configuration. + * Nested by provider name so each provider's settings are self-contained + * and new protocols can be added without polluting the top-level config. * Passed from the init file where `process.env.X` is babel-transformed at build time. */ - myxAppIdTestnet?: string; - myxApiSecretTestnet?: string; - myxBrokerAddressTestnet?: string; - myxAppIdMainnet?: string; - myxApiSecretMainnet?: string; - myxBrokerAddressMainnet?: string; + providerCredentials?: PerpsProviderCredentials; +}; - /** - * Whether MYX provider is enabled via local env var (MM_PERPS_MYX_PROVIDER_ENABLED). - * Must match the UI selector logic so the controller and UI agree on MYX availability. - */ - myxProviderEnabled?: boolean; +export type HyperLiquidCredentials = { + /** Builder fee wallet address for testnet. Empty/omitted = uses BUILDER_FEE_CONFIG default. */ + builderAddressTestnet?: string; + /** Builder fee wallet address for mainnet. Empty/omitted = uses BUILDER_FEE_CONFIG default. */ + builderAddressMainnet?: string; +}; + +export type MYXCredentials = { + /** Whether MYX provider is enabled via local env var. */ + enabled?: boolean; + appIdTestnet?: string; + apiSecretTestnet?: string; + brokerAddressTestnet?: string; + appIdMainnet?: string; + apiSecretMainnet?: string; + brokerAddressMainnet?: string; +}; + +export type PerpsProviderCredentials = { + hyperliquid?: HyperLiquidCredentials; + myx?: MYXCredentials; }; export type PriceUpdate = { diff --git a/app/core/Engine/controllers/perps-controller/index.test.ts b/app/core/Engine/controllers/perps-controller/index.test.ts index 829ff3d2d88..9cbae1d3e0f 100644 --- a/app/core/Engine/controllers/perps-controller/index.test.ts +++ b/app/core/Engine/controllers/perps-controller/index.test.ts @@ -17,6 +17,7 @@ jest.mock( '../../../../components/UI/Perps/adapters/mobileInfrastructure', () => ({ createMobileInfrastructure: jest.fn(() => ({})), + createMobileClientConfig: jest.fn(() => ({})), }), ); jest.mock('../../../../components/UI/Perps/utils/e2eBridgePerps', () => ({ diff --git a/app/core/Engine/controllers/perps-controller/index.ts b/app/core/Engine/controllers/perps-controller/index.ts index 94065a09d55..72bad653197 100644 --- a/app/core/Engine/controllers/perps-controller/index.ts +++ b/app/core/Engine/controllers/perps-controller/index.ts @@ -3,10 +3,12 @@ import { PerpsController, PerpsControllerMessenger, getDefaultPerpsControllerState, - parseCommaSeparatedString, } from '@metamask/perps-controller'; import { applyE2EControllerMocks } from '../../../../components/UI/Perps/utils/e2eBridgePerps'; -import { createMobileInfrastructure } from '../../../../components/UI/Perps/adapters/mobileInfrastructure'; +import { + createMobileInfrastructure, + createMobileClientConfig, +} from '../../../../components/UI/Perps/adapters/mobileInfrastructure'; /** * Initialize the PerpsController. @@ -23,34 +25,11 @@ export const perpsControllerInit: ControllerInitFunction< const perpsControllerState = persistedState.PerpsController ?? getDefaultPerpsControllerState(); - // Pass fallback HIP-3 values from local env vars - // PerpsController will try to read remote feature flags on construction - // and subscribe to updates via RemoteFeatureFlagController:stateChange const controller = new PerpsController({ messenger: controllerMessenger, state: perpsControllerState, infrastructure: createMobileInfrastructure(), - clientConfig: { - fallbackBlockedRegions: parseCommaSeparatedString( - process.env.MM_PERPS_BLOCKED_REGIONS ?? '', - ), - fallbackHip3Enabled: process.env.MM_PERPS_HIP3_ENABLED === 'true', - fallbackHip3AllowlistMarkets: parseCommaSeparatedString( - process.env.MM_PERPS_HIP3_ALLOWLIST_MARKETS ?? '', - ), - fallbackHip3BlocklistMarkets: parseCommaSeparatedString( - process.env.MM_PERPS_HIP3_BLOCKLIST_MARKETS ?? '', - ), - myxProviderEnabled: process.env.MM_PERPS_MYX_PROVIDER_ENABLED === 'true', - myxAppIdTestnet: process.env.MM_PERPS_MYX_APP_ID_TESTNET ?? '', - myxApiSecretTestnet: process.env.MM_PERPS_MYX_API_SECRET_TESTNET ?? '', - myxBrokerAddressTestnet: - process.env.MM_PERPS_MYX_BROKER_ADDRESS_TESTNET ?? '', - myxAppIdMainnet: process.env.MM_PERPS_MYX_APP_ID_MAINNET ?? '', - myxApiSecretMainnet: process.env.MM_PERPS_MYX_API_SECRET_MAINNET ?? '', - myxBrokerAddressMainnet: - process.env.MM_PERPS_MYX_BROKER_ADDRESS_MAINNET ?? '', - }, + clientConfig: createMobileClientConfig(), }); // Apply E2E mocks if configured via bridge diff --git a/docs/perps/perps-refactoring-plan.md b/docs/perps/perps-refactoring-plan.md new file mode 100644 index 00000000000..ba076720fce --- /dev/null +++ b/docs/perps/perps-refactoring-plan.md @@ -0,0 +1,313 @@ +# Perps Refactoring Plan + +## Overview + +The perps controller layer has grown significantly since its initial extraction (Feb 11, 2026). Four files now account for 52% of all perps production code (18,733 of 35,920 lines across 62 files). This document investigates the root causes and provides a phased plan to bring file sizes under control. + +**Locations**: `app/controllers/perps/` + +--- + +## 1. Investigation: Why Did Files Explode? + +### Growth Timeline — PerpsController.ts + +| Date | Commit | Lines | Delta | What landed | +| ------ | --------------------------------------------- | ----: | ----: | ------------------------------------------------------ | +| Feb 11 | `4cd86d34` perps-controller folder setup | 3,343 | — | Initial extraction from monolith | +| Feb 16 | `3ab7d90b` preload market and user data | 3,884 | +541 | Preloading added directly to controller | +| Mar 05 | `7140d264` MYX provider infrastructure | 4,648 | +764 | MYX lifecycle, provider switching, aggregated provider | +| Mar 10 | `03597ba3` Geolocation Controller integration | 4,526 | -122 | Geolocation + minor cleanup | +| Mar 25 | `a679c0e3` (HEAD) | 4,689 | +163 | MYX error handling, core resolver changes | + +**+1,346 lines (40% growth) in 6 weeks**, despite several refactoring PRs in the same period. + +### Root Causes + +#### 1a. Preloading added directly to controller (~505 lines) + +**Status: Already resolved by PR #27898** (removes preloading from PerpsController entirely). + +Lines 2715–3211 contain the full preload lifecycle: `startMarketDataPreload`, `stopMarketDataPreload`, `#performMarketDataPreload`, `#performUserDataPreload`, plus ~50 lines of guard/debounce logic near line 990 and state field declarations near line 344. This was the single largest feature addition (+541 lines on Feb 16) and was added to the controller because it needed access to state, messenger, and provider — but it has no reason to live inside the controller class. + +#### 1b. Deposit lifecycle tracking (~285 lines) + +Lines 2073–2357: `depositWithConfirmation()` alone is 285 lines. It orchestrates transaction preparation, `depositRequests` state tracking, success/failure/cancellation toast flows, and analytics. A `DepositService` already exists (imported at line 32), but the controller still owns the full deposit orchestration logic — the extraction was incomplete. + +#### 1c. MYX provider registration (~150 lines) + +Lines 706–766 (feature flag checks, registration promise) + lines 1602–1676 (`registerMYXProvider`, `handleMYXImportError`) + lines 1682–1730 (`#assignActiveProvider`). Provider lifecycle management (creation, dynamic import, assignment, error handling) belongs in a dedicated registry, not the main controller. + +#### 1d. Subscription boilerplate (~240 lines) + +Lines 3638–3941: Nine `subscribeTo*` methods follow an identical pattern: + +1. Null-check `activeProviderInstance` +2. Call `provider.subscribeTo*(params)` +3. Catch + log error → return no-op unsubscribe + +Only `subscribeToAccount` differs (intercepts callback to update Redux state). The other 8 are pure delegation boilerplate that could be generated by a generic helper. + +#### 1e. State preference getters/setters (~460 lines) + +Lines 4124–4689: 14+ methods for trade configuration, pending config (with 5-min expiry), market filter preferences, payment token selection, order book grouping, and watchlist management. Each method is simple CRUD (read/write state, debug log, optional analytics), but they accumulate. These have no business logic coupling to the controller — pure state management. + +### Anti-Patterns Table + +| Pattern | Example | Lesson | +| -------------------------------- | ---------------------------------------------------------------------- | ----------------------------------------------------------- | +| "Just add it to the controller" | Preloading (+541 lines in one PR) | New features should start as separate services from day one | +| Incomplete extraction | DepositService exists but controller still owns 285-line orchestration | Extraction means moving ALL logic, not just helpers | +| Provider lifecycle in controller | MYX registration, feature flags, error handling all in controller | Provider creation/management is a separate concern | +| Copy-paste delegation | 8 identical `subscribeTo*` methods | Use a generic subscription proxy or code generation | +| Preference creep | 14 get/set methods accumulated over 6 PRs | Group preferences into a dedicated PreferencesService early | + +--- + +## 2. Large File Inventory + +All files over 1,000 lines in `app/controllers/perps/` (production only, excludes tests): + +| # | File | Lines | Primary bloat source | +| --- | -------------------------------------------- | ----: | ------------------------------------------------------------ | +| 1 | `providers/HyperLiquidProvider.ts` | 8,342 | Orders, positions, markets, and adapter logic in one class | +| 2 | `PerpsController.ts` | 4,689 | Preloading, deposits, subscriptions, preferences (see above) | +| 3 | `services/HyperLiquidSubscriptionService.ts` | 3,643 | All WebSocket channels in one file | +| 4 | `services/TradingService.ts` | 2,059 | Order + position operations mixed | +| 5 | `types/index.ts` | 1,677 | All domain types in single barrel file | +| 6 | `providers/MYXProvider.ts` | 1,182 | Acceptable — single provider, self-contained | +| 7 | `services/HyperLiquidClientService.ts` | 1,115 | HTTP client methods — low complexity per line | +| 8 | `services/MYXClientService.ts` | 1,070 | HTTP client methods — low complexity per line | +| 9 | `services/MarketDataService.ts` | 1,052 | Acceptable — single domain | + +**After PR #27898 merges**: PerpsController drops ~500 lines → ~4,200 lines. Top-4 total drops from 18,733 to ~18,200. + +--- + +## 3. Phased Refactoring Plan + +### Phase 1: PerpsController.ts (post-preload-removal ~4,200 → ~2,500 target) + +Preloading removal (PR #27898) is already in flight. The remaining extractions target the four next-largest sections. + +#### 1a. DepositFlowService (~280 lines) + +**Extract**: `depositWithConfirmation()` orchestration (lines 2073–2357), `clearDepositResult()`, `clearWithdrawResult()`, and deposit request state management. + +**Target**: New `services/DepositFlowService.ts` that takes `update()`, `getState()`, provider, and metrics as constructor args. The existing `DepositService` handles transaction preparation — `DepositFlowService` owns the higher-level lifecycle (confirmation screen flow, toast, analytics, `depositRequests` bookkeeping). + +**Risk**: Low — deposit flow is self-contained with clear inputs/outputs. + +#### 1b. Collapse subscription delegation (~200 lines saved) + +The 7 identical `subscribeTo*` methods (Prices, Positions, OrderFills, Orders, OrderBook, Candles, OICaps) have no pause/resume, no state tracking — they are pure null-check + try/catch + `provider.subscribeTo*(params)`. There is no reason for 7 separate methods. + +**Replace with a single generic method**: + +```typescript +// Before: 7 × ~22 lines = ~154 lines of identical boilerplate +subscribeToPrices(params: SubscribePricesParams): () => void { + const provider = this.getActiveProviderOrNull(); + if (!provider) return () => {}; + try { return provider.subscribeToPrices(params); } + catch (error) { this.#logError(...); return () => {}; } +} +// ... repeated 6 more times + +// After: single generic method (~20 lines) +subscribe( + channel: C, + params: SubscriptionParamsMap[C], +): () => void { + const provider = this.getActiveProviderOrNull(); + if (!provider) return () => {}; + try { return provider[`subscribeTo${channel}`](params); } + catch (error) { + this.#logError(ensureError(error, `PerpsController.subscribeTo${channel}`), + this.#getErrorContext(`subscribeTo${channel}`, params)); + return () => {}; + } +} +``` + +Keep `subscribeToAccount` (intercepts callback to update Redux) and `subscribeToConnectionState` (different fallback logic) as manual methods — they have real custom behavior. + +**Risk**: Low — purely mechanical. Callers switch from `controller.subscribeToPrices(p)` to `controller.subscribe('Prices', p)`, or keep thin typed aliases that call the generic method. + +#### 1c. PreferencesService (~400 lines saved) + +**Extract**: Lines 4124–4689 (trade configuration, pending config, market filter preferences, payment token, order book grouping, watchlist) into `services/PreferencesService.ts`. + +The service receives `update()` and `getState()` callbacks. Each method is a simple read/write with optional analytics. No coupling to provider, messenger, or other controller internals. + +**Risk**: Very low — pure state CRUD with no async operations. + +#### 1d. ProviderRegistryService (~150 lines saved) + +**Extract**: Provider creation (`#createProviders`), MYX registration (`registerMYXProvider`, `handleMYXImportError`), provider assignment (`#assignActiveProvider`), and feature flag checks (`#isMYXProviderEnabled`) into `services/ProviderRegistryService.ts`. + +Controller calls `registry.createProviders(config)` and `registry.getActiveProvider()`. Registry owns the provider `Map`, dynamic import logic, and fallback behavior. + +**Risk**: Medium — touches initialization sequence. Requires careful constructor ordering. Should be validated with existing integration tests. + +#### Phase 1 Summary + +| Extraction | Lines removed | New file | Risk | +| ----------------------- | ------------: | ------------------------------------- | -------- | +| Preloading (PR #27898) | ~500 | _(already in flight)_ | Done | +| DepositFlowService | ~280 | `services/DepositFlowService.ts` | Low | +| Subscription collapse | ~200 | _(inline generic method)_ | Low | +| PreferencesService | ~400 | `services/PreferencesService.ts` | Very low | +| ProviderRegistryService | ~150 | `services/ProviderRegistryService.ts` | Medium | +| **Total** | **~1,510** | | | + +**Post-Phase 1 PerpsController**: ~4,200 - 1,010 = **~3,190 lines** (without preload), on track toward 2,500 target with further cleanup. + +--- + +### Phase 2: HyperLiquidProvider.ts (8,342 → ~3,500 target) + +HyperLiquidProvider is the largest file in the perps codebase. It contains four distinct concerns that map cleanly to separate services. + +#### 2a. HyperLiquidOrderService (~1,800 lines) + +**Extract**: Order placement, modification, cancellation, TP/SL management. Methods include `placeOrder`, `cancelOrder`, `modifyOrder`, `createTPSL`, `updateTPSL`, and their supporting validation/signing logic. + +#### 2b. HyperLiquidPositionService (~1,500 lines) + +**Extract**: Position queries, close operations, margin management, liquidation calculations. Methods include `getPositions`, `closePosition`, `closePositions`, `updateMargin`, `calculateLiquidationPrice`. + +#### 2c. HyperLiquidMarketService (~1,200 lines) + +**Extract**: Market data fetching, funding rates, candle data, order book queries, market metadata. Methods include `getMarketData`, `getMarketDataWithPrices`, `getFundingRates`, `getHistoricalCandles`. + +#### 2d. Consolidate adapter layer (~300 lines saved) + +The provider contains inline data transformation (HyperLiquid API shapes → normalized types). These should consolidate into `utils/hyperLiquidAdapter.ts` (466 lines already exists) to eliminate duplication between the provider and the existing adapter utility. + +#### Phase 2 Summary + +| Extraction | Lines moved | New file | +| -------------------------- | ----------: | ---------------------------------------- | +| HyperLiquidOrderService | ~1,800 | `services/HyperLiquidOrderService.ts` | +| HyperLiquidPositionService | ~1,500 | `services/HyperLiquidPositionService.ts` | +| HyperLiquidMarketService | ~1,200 | `services/HyperLiquidMarketService.ts` | +| Adapter consolidation | ~300 | _(merged into existing adapter)_ | + +**Post-Phase 2 HyperLiquidProvider**: ~8,342 - 4,800 = **~3,500 lines** (coordinator + account + connection management). + +--- + +### Phase 3: Other Large Files + +#### 3a. HyperLiquidSubscriptionService (3,643 → ~1,800 target) + +Split by WebSocket channel family: + +- `HLPriceSubscriptions.ts` — price/allMids/activeAssetCtx channels +- `HLUserSubscriptions.ts` — positions, orders, order fills, account +- `HLMarketSubscriptions.ts` — order book, candles, OI caps + +The base class retains connection management, reconnection logic, and the generic subscription lifecycle. + +#### 3b. TradingService (2,059 → ~1,200 target) + +Split into: + +- `OrderExecutionService.ts` — `placeOrder`, `cancelOrder`, `cancelOrders` + order analytics +- `PositionExecutionService.ts` — `closePosition`, `closePositions`, `updatePositionTPSL`, `updateMargin` + position analytics + +Both share the error context helper and metrics delegation, which stays in a shared base or utility. + +#### 3c. types/index.ts (1,677 → split by domain) + +Split into domain-specific type files: + +- `types/market.ts` — market data, candles, funding rates +- `types/order.ts` — order types, order parameters, order results +- `types/position.ts` — position types, margin, liquidation +- `types/account.ts` — account state, balances, deposit/withdrawal +- `types/subscription.ts` — subscription params, callbacks +- `types/index.ts` — re-exports (barrel file, ~50 lines) + +This is a low-risk, high-clarity improvement. Re-exports from `types/index.ts` maintain backward compatibility. + +--- + +## 4. Guardrails to Prevent Re-Growth + +### File Size Budgets + +| Threshold | Action | +| ------------ | ---------------------------------------------------------------------- | +| 2,000 lines | Acceptable target for complex service files | +| 2,500 lines | Warning — must justify in PR description why extraction isn't feasible | +| 3,000+ lines | Hard fail — PR cannot merge without extraction plan | + +### Process Rules + +1. **New feature = new service file**. If a feature adds >100 lines of logic to an existing file, it gets its own service. The controller delegates; it does not implement. + +2. **Complete extractions only**. Moving helpers but leaving orchestration in the controller is not an extraction. The controller should have a single delegation call, not a 200-line method that calls 3 helpers. + +3. **No boilerplate accumulation**. If you're writing the same pattern a third time, extract a generic helper (subscription proxy, preference CRUD generator, etc.). + +4. **Provider-specific logic stays in provider directory**. HyperLiquid-specific code goes in `providers/` or `services/HyperLiquid*`. Controller code must be provider-agnostic. + +### Proposed CI Check + +Add a file-size lint step to CI that scans `app/controllers/perps/**/*.ts` (excluding tests): + +```yaml +# .github/workflows/perps-file-size.yml (conceptual) +- name: Check perps file sizes + run: | + find app/controllers/perps -name '*.ts' ! -name '*.test.ts' ! -name '*.spec.ts' \ + -exec wc -l {} + | sort -rn | while read lines file; do + [ "$file" = "total" ] && continue + if [ "$lines" -gt 3000 ]; then + echo "::error file=$file::$lines lines exceeds 3,000 line hard limit" + exit 1 + elif [ "$lines" -gt 2500 ]; then + echo "::warning file=$file::$lines lines approaching limit (2,500+)" + fi + done +``` + +### Tracking + +Each phase extraction should be a separate PR with: + +- Before/after line counts in PR description +- No behavior changes — pure mechanical moves + delegation wiring +- Existing tests pass without modification (or with import path updates only) + +--- + +## Appendix: Current File Size Distribution + +``` +8,342 providers/HyperLiquidProvider.ts +4,689 PerpsController.ts +3,643 services/HyperLiquidSubscriptionService.ts +2,059 services/TradingService.ts +1,677 types/index.ts +1,182 providers/MYXProvider.ts +1,115 services/HyperLiquidClientService.ts +1,070 services/MYXClientService.ts +1,052 services/MarketDataService.ts + 793 providers/AggregatedPerpsProvider.ts + 690 utils/myxAdapter.ts + 611 aggregation/SubscriptionMultiplexer.ts + 539 utils/hyperLiquidValidation.ts + 480 constants/eventNames.ts + 477 index.ts + 466 utils/hyperLiquidAdapter.ts + 462 constants/hyperLiquidConfig.ts + 451 utils/orderCalculations.ts + 408 services/AccountService.ts +───── +35,920 total (62 production files) +``` + +_Line counts as of Mar 25, 2026 on branch `feat/perps/core-resolver`._ diff --git a/scripts/perps/validate-core-sync.sh b/scripts/perps/validate-core-sync.sh index 5001b97a02a..84bc37b6dc9 100755 --- a/scripts/perps/validate-core-sync.sh +++ b/scripts/perps/validate-core-sync.sh @@ -327,13 +327,7 @@ step_verify_fixes() { step_eslint_fix() { cd "$CORE_PATH" - # Back up suppressions file so we don't leave dirty changes in Core local supp_file="$CORE_PATH/eslint-suppressions.json" - local supp_backup="" - if [[ -f "$supp_file" ]]; then - supp_backup=$(mktemp) - cp "$supp_file" "$supp_backup" - fi progress " ├─ Running --fix" yarn eslint 'packages/perps-controller/src/**/*.ts' --fix || true @@ -359,11 +353,6 @@ step_eslint_fix() { SUPPRESSION_COUNT=0 fi - # Restore original suppressions file - if [[ -n "$supp_backup" ]]; then - mv "$supp_backup" "$supp_file" - fi - cd "$MOBILE_ROOT" return 0 }