feat: defi positions v2 controller - #44392
Merged
Merged
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Contributor
✨ Files requiring CODEOWNER review ✨💎 @MetaMask/metamask-assets (5 files, +103 -21)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Builds ready [0535b6a]
⚡ Performance Benchmarks (Total: 🟢 12 pass · 🟡 12 warn · 🔴 0 fail)
Bundle sizes
|
Contributor
Builds ready [cb1ec44]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 11 warn · 🔴 0 fail)
Bundle sizes
|
bergarces
marked this pull request as draft
July 13, 2026 15:18
bergarces
marked this pull request as ready for review
July 13, 2026 15:20
bergarces
marked this pull request as draft
July 13, 2026 15:20
Contributor
Builds ready [2d05fde] [reused from 4757a65]
⚡ Performance Benchmarks (Total: 🟢 0 pass · 🟡 0 warn · 🔴 0 fail)
Bundle size diffs: data not available. |
Contributor
Builds ready [3c1fc12]
⚡ Performance Benchmarks (Total: 🟢 16 pass · 🟡 8 warn · 🔴 0 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
Builds ready [455b2cb]
⚡ Performance Benchmarks (Total: 🟢 15 pass · 🟡 8 warn · 🔴 1 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eca0b90. Configure here.
Prithpal-Sooriya
previously approved these changes
Jul 28, 2026
Contributor
Builds ready [93b6499]
⚡ Performance Benchmarks (Total: 🟢 12 pass · 🟡 11 warn · 🔴 1 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| showSortControl = true, | ||
| onNetworkSelect, | ||
| onRefresh, | ||
| }: AssetListControlBarProps) => { |
Contributor
There was a problem hiding this comment.
This is fair, we have ticketed an overhaul to build the TDP from ground up (removing debt, cleaning surfaces, and improving DX)
Prithpal-Sooriya
previously approved these changes
Jul 29, 2026
juanmigdr
requested changes
Jul 29, 2026
Comment on lines
+52
to
+58
| const hasPositions = accountIds.some( | ||
| (id) => positionsByAccount[id] !== undefined, | ||
| ); | ||
| const positions = useMemo( | ||
| () => mergePositionsForAccounts(positionsByAccount, accountIds), | ||
| [positionsByAccount, accountIds], | ||
| ); |
Member
There was a problem hiding this comment.
One from cursor (marked as HIGH):
false-positive error state on cold start
failedAccountGroup starts null; isError = failedAccountGroup === selectedAccountGroup. If selectedAccountGroup itself hasn't loaded yet (also null), null === null evaluates true on mount — before any fetch has failed. Since isLoading = !hasPositions && !isError, this can suppress the loading spinner and briefly show the error/redirect UI instead. Fix: track "not ready" separately from "errored" (e.g. a ref/flag) instead of comparing against the group id.
Contributor
Author
There was a problem hiding this comment.
Added a null check to avoid that potential issue during cold starts.
| return assetReference as TokenWithFiatAmount['address']; | ||
| } | ||
|
|
||
| return toChecksumHexAddress(assetReference) as TokenWithFiatAmount['address']; |
Member
There was a problem hiding this comment.
Unguarded cast can crash the page... toChecksumHexAddress(assetReference) throws on a malformed CAIP asset id, and this mapper has no try/catch. It's called from useMemo in defi-details-position-cell-v2.tsx:23, so one malformed position from the controller would crash the whole details page with no visible error boundary
Contributor
Author
There was a problem hiding this comment.
Good catch, fixing now.
Contributor
Author
There was a problem hiding this comment.
The function is now a bit more conscious about the different address types. I think we ought to create utils to move between assetId -> address and viceversa in assets-controller though.
|
Contributor
Builds ready [9f13bce]
⚡ Performance Benchmarks (Total: 🟢 17 pass · 🟡 6 warn · 🔴 1 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Prithpal-Sooriya
approved these changes
Jul 29, 2026
juanmigdr
approved these changes
Jul 30, 2026
Contributor
🧪 Validation RunVerdict: Note Trial run of the MetaMask evidence skills — A counting consumer under six unrelated store writes, run in CI twice: as committed, and with the empty case returning a stable reference. The arm-B edit is applied by the runner and reverted after. Consumer render countVerdict: delta measured: 6 → 1 renders with a stable empty-object constant
$ yarn jest ui/selectors/defi-controller-v2/__render_probe__.test.tsx
RENDER_COUNT consumer=6 parentRenders=6
$ sed -i '18s|.*| return state?.metamask?.allDeFiPositionsV2 ?? (globalThis.__EMPTY_DEFI ??= {});|' ui/selectors/defi-controller-v2/positions.ts && yarn jest ui/selectors/defi-controller-v2/__render_probe__.test.tsx
RENDER_COUNT consumer=1 parentRenders=6This counts renders of one named consumer across a defined interaction. It is not a count Produced by Follows from the arms above
Open for review: how much of a session sits in the unset state — cold start, account-group switch, an account with no positions. Arm B is an attribution control, not a proposed patch; it counts renders, not whether the output is equivalent. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A new set of UI pages, components and hooks have been created, all under the same folder, to fetch and display the contents of the new DeFi response in accounts-api.
All these changes are behind a feature flag, currently only enabled in dev.
Changelog
CHANGELOG entry: Changed DeFi tab to use new controller when the flag is enabled
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3601
Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3602
Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3603
Manual testing steps
Screenshots/Recordings
Before
After
feat_.defi.positions.v2.controller.by.bergarces.Pull.Request.44392.MetaMask_metamask-extension.-.28.July.2026.mp4
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Touches home DeFi UX and background fetch via messenger with nuanced loading/error/account-group behavior, but changes are gated by a remote flag and V1 remains the default path.
Overview
Adds a DeFi Controller V2 experience (behind
defiControllerV2) that loads protocol groups fromDeFiPositionsControllerV2via route messengerfetchDeFiPositions, with new tab list, protocol details, and mapping from V2 underlying positions into existing token cells.When the flag is on, the DeFi tab and details route use
useDeFiPositionsV2(store-driven loading, group-scoped errors,forceRefreshon user refresh) andRouteWithMessengerwithDEFI_ROUTE_ALLOWED_CAPABILITIES. When off, legacy V1 tab/details behavior is unchanged. DeFi tab implementation moves underui/pages/defi/; the assetsdefi-tabentry re-exports the page module.Asset list control bar gains optional
onRefresh: withshowImportTokenButton={false}, users get a more-options menu with Refresh list only (wired on the V2 DeFi tab).getImageForChainIdnow resolves EVM CAIP chain IDs for badges/icons.Tests cover V2 hooks, pages, mappers, control bar refresh UX, and integration tests pin V2 off so they stay on V1 UI.
Reviewed by Cursor Bugbot for commit 9f13bce. Bugbot is set up for automated code reviews on this repo. Configure here.