[LWDM] chore(aggregated-assets): move domain layer behind shims (LIVE-35226) - #20345
[LWDM] chore(aggregated-assets): move domain layer behind shims (LIVE-35226)#20345LucasWerey wants to merge 2 commits into
Conversation
Relocate the dada-client entity types and API layer into the aggregated-assets DDD packages (LIVE-35226, epic LIVE-35223). Behaviour-neutral: zero consumer files change, all 82 consumers keep working through one-line shims at the old paths. Pure relocation - no symbol renames, no signature changes, no zod, and no endpoint restructuring. The per-use-case injectEndpoints split is LIVE-35301. entity-aggregated-asset CryptoAssetMeta entity-interest-rate InterestRate, ApyType api-aggregated-assets wire schema, transforms, api, internals, errors, mocks transforms.ts is split out of api.ts so the wire->entity boundary is separately testable, matching the market-sentiment precedent. NetworkInfo and CurrenciesOrder land in the api package rather than an entity: a network IS a chain, already modelled by entity-currency-crypto, and CurrenciesOrder is server sort metadata. Both were entities in the original scoping; the naming review corrected that. Breaks the last boundary violation. dadaIdToMarketId and the market item type are copied from libs/ledger-live-common/src/market into internals/market.ts, since a domain/* package cannot import legacy libs/*. Drift risk is documented there and in the README: every field is optional, so divergence will never produce a type error. reducerPath stays the literal "assetsDataApi" and is now pinned by a test. createCurrencyDataSelector hand-scans that key as a string and Storybook stories preload it, so a rename would silently return undefined for every market and interest-rate lookup with no type error. The characterization tests from LIVE-35224 pass unmodified: 21 suites, 245 tests.
LucasWerey
changed the title
live-github-bot
Bot
changed the title
Web Tools Build Status
|
There was a problem hiding this comment.
Pull request overview
Relocates the legacy libs/ledger-live-common/src/dada-client “domain layer” into the DDD domain/entity/* and domain/api/* packages, while keeping the previous import paths working via one-line re-export shims in live-common. This supports the ongoing DDD migration without forcing consumer rewrites in the same step.
Changes:
- Introduces
@domain/entity-aggregated-asset,@domain/entity-interest-rate, and@domain/api-aggregated-assetsimplementations (types, RTK Query API, transforms, errors, and internals). - Replaces the legacy
dada-clientimplementation files with re-export shims pointing to the new domain packages. - Updates workspace dependencies (live-common + lockfile) and adds a small test to pin the frozen
assetsDataApi.reducerPath.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Adds new workspace importers; also contains additional lockfile churn. |
libs/ledger-live-common/src/dada-client/utils/mergeAssetsDataPages.ts |
Shim re-export to domain API implementation. |
libs/ledger-live-common/src/dada-client/utils/errorUtils.ts |
Shim re-export of error utilities/types from domain API. |
libs/ledger-live-common/src/dada-client/utils/deepMergeCryptoAssets.ts |
Shim re-export to domain API implementation. |
libs/ledger-live-common/src/dada-client/utils/chunkCurrencyIds.ts |
Shim re-export of chunking util/type from domain API. |
libs/ledger-live-common/src/dada-client/types/trend.ts |
Moves ApyType export to the interest-rate entity package. |
libs/ledger-live-common/src/dada-client/state-manager/types.ts |
Shim re-export of API types/enums/constants from domain API. |
libs/ledger-live-common/src/dada-client/state-manager/api.ts |
Shim re-export of the full aggregated-assets API surface. |
libs/ledger-live-common/src/dada-client/mocks/stocks.mock.ts |
Shim re-export of stocks mock from domain API subpath export. |
libs/ledger-live-common/src/dada-client/mocks/stablecoins.mock.ts |
Shim re-export of stablecoins mock from domain API subpath export. |
libs/ledger-live-common/src/dada-client/entities/index.ts |
Shims entity/wire types to the new DDD packages. |
libs/ledger-live-common/src/dada-client/__mocks__/assets.mock.ts |
Shim re-export of assets mocks from domain API mock export. |
libs/ledger-live-common/package.json |
Adds the new @domain/* packages as dependencies (and tidies ordering). |
domain/entity/interest-rate/src/types.ts |
Introduces ApyType union in entity package. |
domain/entity/interest-rate/src/schema.ts |
Adds interest-rate type definition (currently as TS interface). |
domain/entity/interest-rate/src/index.ts |
Exposes interest-rate exports from the entity package. |
domain/entity/aggregated-asset/src/schema.ts |
Adds aggregated-asset meta type definition (currently as TS interface). |
domain/entity/aggregated-asset/src/index.ts |
Exposes aggregated-asset exports from the entity package. |
domain/api/aggregated-assets/tsconfig.json |
Aligns TS libs/types with other domain/api packages (DOM + node types). |
domain/api/aggregated-assets/src/types.ts |
Defines API-facing types/enums/params used by RTK Query and consumers. |
domain/api/aggregated-assets/src/transforms.ts |
Extracts response transforms + wire→entity currency conversion. |
domain/api/aggregated-assets/src/stocks.mock.ts |
Moves stocks mock into the domain API package. |
domain/api/aggregated-assets/src/stablecoins.mock.ts |
Moves stablecoins mock into the domain API package. |
domain/api/aggregated-assets/src/schema.ts |
Defines the raw wire contract shapes (interfaces). |
domain/api/aggregated-assets/src/internals/mergeAssetsDataPages.ts |
Internal page-merging helper relocated into the domain API package. |
domain/api/aggregated-assets/src/internals/market.ts |
Internal market typing + dadaIdToMarketId copied to avoid legacy imports. |
domain/api/aggregated-assets/src/internals/deepMergeCryptoAssets.ts |
Internal deep-merge helper relocated into the domain API package. |
domain/api/aggregated-assets/src/internals/chunkCurrencyIds.ts |
Internal chunking helper relocated into the domain API package. |
domain/api/aggregated-assets/src/index.ts |
Barrel exports for the new domain API package (incl. selected internals + mocks). |
domain/api/aggregated-assets/src/errors.ts |
Error utilities moved into the domain API package. |
domain/api/aggregated-assets/src/assetsData.mock.ts |
Moves aggregated-assets mock fixtures into the domain API package. |
domain/api/aggregated-assets/src/api.ts |
RTK Query API relocated into the domain API package (incl. frozen reducerPath comment). |
domain/api/aggregated-assets/src/api.test.ts |
Adds test pinning assetsDataApi.reducerPath. |
domain/api/aggregated-assets/package.json |
Defines exports (incl. mock subpaths), deps, and peers for the domain API package. |
.changeset/quiet-comets-relocate.md |
Changeset documenting the relocation + live-common patch bump. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
| /** An interest rate attached to one currency. */ | ||
| export interface InterestRate { | ||
| /** Currency identifier */ | ||
| currencyId: string; | ||
| /** Interest rate value */ |
There was a problem hiding this comment.
You're right — fixed in 22b1c04.
domain/entity/README.md is explicit: "Use zod for schema definition", with schema.ts and schema.test.ts listed as required, and "The schema is always required — it is the canonical data model and the primary reason the package exists." Shipping plain interfaces diverged from that.
Now follows the domain/entity/market-sentiment layout: schema.ts holds the Zod schema, types.ts infers the type via z.infer, schema.test.ts covers validation, and the barrel re-exports both.
Worth noting why this was safe to do inside a behaviour-neutral relocation, since my initial reasoning for deferring it was wrong. I had assumed adopting Zod meant adopting branded types, which would force a .parse() at the boundary and break consumers building these shapes from raw JSON. But a plain z.object() without .brand() infers a structurally identical type — so the schema can exist and be exported without anything being obliged to parse yet. Confirmed: libs/ledger-live-common still typechecks with 0 errors and the characterization tests pass unmodified (21 suites, 245 tests).
Applying .parse() at the api boundary remains LIVE-35232, which is where the drop-invalid-item semantics get decided.
Two loosenesses are deliberate and now documented and tested rather than tightened:
InterestRate.typestaysz.string()rather than theApyTypeenum. DADA genuinely sends kinds outside"NRR" | "APY" | "APR"anduseInterestRatesByCurrenciesdrops them — behaviour pinned in test(dada-client): characterize the untested surface (LIVE-35224) #20276. Narrowing here would claim a guarantee the wire doesn't give.fetchAtstaysz.string()rather thanDateTimeIsoSchema. I checked: it has zero production reads — every occurrence is a mock, fixture or Storybook story. Validating a field nobody consumes could only discard otherwise-good rates.
| /** | ||
| * An aggregated asset: one logical asset grouping several per-network currencies. | ||
| * | ||
| * `assetsIds` maps a network id to the currency id that represents this asset on that network, | ||
| * which is what makes it "aggregated" rather than a single currency. |
There was a problem hiding this comment.
You're right — fixed in 22b1c04.
domain/entity/README.md is explicit: "Use zod for schema definition", with schema.ts and schema.test.ts listed as required, and "The schema is always required — it is the canonical data model and the primary reason the package exists." Shipping plain interfaces diverged from that.
Now follows the domain/entity/market-sentiment layout: schema.ts holds the Zod schema, types.ts infers the type via z.infer, schema.test.ts covers validation, and the barrel re-exports both.
Worth noting why this was safe to do inside a behaviour-neutral relocation, since my initial reasoning for deferring it was wrong. I had assumed adopting Zod meant adopting branded types, which would force a .parse() at the boundary and break consumers building these shapes from raw JSON. But a plain z.object() without .brand() infers a structurally identical type — so the schema can exist and be exported without anything being obliged to parse yet. Confirmed: libs/ledger-live-common still typechecks with 0 errors and the characterization tests pass unmodified (21 suites, 245 tests).
Applying .parse() at the api boundary remains LIVE-35232, which is where the drop-invalid-item semantics get decided.
Two loosenesses are deliberate and now documented and tested rather than tightened:
InterestRate.typestaysz.string()rather than theApyTypeenum. DADA genuinely sends kinds outside"NRR" | "APY" | "APR"anduseInterestRatesByCurrenciesdrops them — behaviour pinned in test(dada-client): characterize the untested surface (LIVE-35224) #20276. Narrowing here would claim a guarantee the wire doesn't give.fetchAtstaysz.string()rather thanDateTimeIsoSchema. I checked: it has zero production reads — every occurrence is a mock, fixture or Storybook story. Validating a field nobody consumes could only discard otherwise-good rates.
| metro-react-native-babel-preset: '*' | ||
| react: 19.1.4 | ||
| react-dom: 19.1.4 | ||
| webpack: ^5.89.0 | ||
| webpack: '*' | ||
| peerDependenciesMeta: |
There was a problem hiding this comment.
Fixed in 22b1c04 — the lockfile diff is now 48 insertions, 0 deletions, containing only the three new workspace importers.
You were right to keep pushing on this. I'd previously concluded the churn was unavoidable pnpm re-resolution triggered by the workspace set changing, and said so on an earlier PR. That was wrong. I tested it properly this time: reverting the two peer-range rewrites by hand and then running pnpm install --frozen-lockfile passes, which proves they were cosmetic rather than required.
The two reverted:
any-observable@0.3.0:rxjs: ^5.5.10(pnpm had rewritten it to'*')@storybook/addon-react-native-web@0.0.29:webpack: ^5.89.0(rewritten to'*')
Neither package is touched by this PR, and --frozen-lockfile is what CI enforces, so keeping develop's values is both correct and reviewable.
For the record on the other half of your note: packageManager pins pnpm@10.24.0 and that is the version used, so the rewrites weren't a version mismatch — pnpm just normalises those ranges opportunistically whenever it rewrites the file.
| @@ -25425,7 +25465,7 @@ packages: | |||
| resolution: {integrity: sha512-/FQM1EDkTsf63Ub2C6O7GuYFDsSXUwsaZDurV0np41ocwq0jthUAYCmhBX9f+KwlaCgIuWyr/4WlUQUBfKfZog==} | |||
| engines: {node: '>=6'} | |||
| peerDependencies: | |||
| rxjs: ^5.5.10 | |||
| rxjs: '*' | |||
There was a problem hiding this comment.
Fixed in 22b1c04 — the lockfile diff is now 48 insertions, 0 deletions, containing only the three new workspace importers.
You were right to keep pushing on this. I'd previously concluded the churn was unavoidable pnpm re-resolution triggered by the workspace set changing, and said so on an earlier PR. That was wrong. I tested it properly this time: reverting the two peer-range rewrites by hand and then running pnpm install --frozen-lockfile passes, which proves they were cosmetic rather than required.
The two reverted:
any-observable@0.3.0:rxjs: ^5.5.10(pnpm had rewritten it to'*')@storybook/addon-react-native-web@0.0.29:webpack: ^5.89.0(rewritten to'*')
Neither package is touched by this PR, and --frozen-lockfile is what CI enforces, so keeping develop's values is both correct and reviewable.
For the record on the other half of your note: packageManager pins pnpm@10.24.0 and that is the version used, so the rewrites weren't a version mismatch — pnpm just normalises those ranges opportunistically whenever it rewrites the file.
Rsdoctor Bundle Diff AnalysisFound 7 projects in monorepo, 2 projects with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 desktop-rendererPath:
📦 Download Diff Report: desktop-renderer Bundle Diff 📁 mobilePath:
Generated by Rsdoctor GitHub Action |
Addresses review on #20345. domain/entity/README.md requires each entity package to define its canonical model as a Zod schema: 'Use zod for schema definition', with schema.ts and schema.test.ts listed as required. Both new entity packages shipped plain TypeScript interfaces instead, which diverged from that convention. Converted to the market-sentiment layout: schema.ts holds the Zod schema, types.ts infers the type, schema.test.ts covers validation, barrel re-exports both. Behaviour-neutral. The schemas use plain z.object without .brand(), so z.infer produces types structurally identical to the previous interfaces - live-common still typechecks with zero errors and the characterization tests pass unchanged. Nothing calls .parse() at the api boundary yet; that is LIVE-35232. Two deliberate loosenesses are documented and tested rather than tightened: InterestRate.type stays a plain string because DADA sends kinds outside ApyType and consumers drop them, and fetchAt stays a plain string rather than DateTimeIsoSchema because nothing reads it, so validating the format could only discard otherwise-good rates. Also fixes the Domain Test CI failure: the job installs only ./domain/** and ./shared/**, so @shared/env's transitive @ledgerhq/live-env was unresolvable. api.test.ts now mocks @shared/env with a factory so the real module is never required. And trims pnpm-lock.yaml to the three new importers only. The peer-range rewrites pnpm emitted for any-observable and @storybook/addon-react-native-web were cosmetic - reverting them keeps 'pnpm install --frozen-lockfile' passing, so the diff is now purely additive.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
domain/api/aggregated-assets/src/api.ts:7
FetchBaseQueryError,FetchBaseQueryMeta, andQueryReturnValueare types, but they’re imported as values. With SWC/Jest (and in general ESM builds), importing type-only names as runtime exports can lead to runtime import errors or unnecessary runtime dependencies. Align with other domain/api packages by marking these as type imports.
import {
createApi,
fetchBaseQuery,
FetchBaseQueryError,
FetchBaseQueryMeta,
QueryReturnValue,
} from "@reduxjs/toolkit/query/react";
domain/api/aggregated-assets/src/errors.ts:1
FetchBaseQueryErroris only used as a type (in a type guard), so it should be imported as a type-only import. This avoids emitting a runtime named import for a type-only symbol (which may not exist at runtime) and matches the pattern used in other domain/api packages.
import { FetchBaseQueryError } from "@reduxjs/toolkit/query";
domain/api/aggregated-assets/src/api.ts:182
- This comment documents a frozen workaround (hard-coded reducerPath). Per repo comment guidance, workaround comments should include a link to the tracking ticket so the rationale doesn’t go stale when the migration continues.
/*
* FROZEN. createCurrencyDataSelector hand-scans state.assetsDataApi.queries by string, and
* Storybook stories preload this exact key. Renaming it silently returns undefined for every
* market and interest-rate lookup, with no type error.
*/
|
LucasWerey
changed the title
Won't be fixed here as we are only moving things |
✅ Checklist
npx changesetwas attached.reducerPath.- Behaviour-neutral by design. Zero consumer files change; all 82 consumers keep working through one-line re-export shims at the old paths.
- No runtime behaviour is altered — no symbol renames, no signature changes, no zod, no endpoint restructuring.
- QA focus if anything is spot-checked: Market (price / 24h change badges), Portfolio (distribution), and the asset/network selectors — these are the surfaces fed by the relocated cache selectors.
📝 Description
Problem.
libs/ledger-live-common/src/dada-clientis being migrated into the DDD layers under LIVE-35223. The packages were scaffolded empty in #20285; this PR moves the domain layer into them.Solution. Relocate the entity types and the API layer, leaving shims behind so consumer retargeting is a separate, independently revertable step (LIVE-35228 / 35229 / 35230).
@domain/entity-aggregated-assetCryptoAssetMeta@domain/entity-interest-rateInterestRate,ApyType@domain/api-aggregated-assetsschema.ts(wire format),types.ts,transforms.ts,api.ts,errors.ts,internals/, 3 mocks via./mock*exports11 files under
dada-clientbecame one-line re-exports.libs/ledger-live-commongains the three packages as workspace deps — it already declared 8@domain/*deps, so this is the established pattern.Decisions worth reviewing
transforms.tsis split out ofapi.ts.convertApiAssets+transformAssetsResponseare now separately testable, matching thedomain/api/market-sentimentprecedent wheretransforms.tsis where the wire schema meets the entity.NetworkInfoandCurrenciesOrderland in the api package, not an entity. This corrects the original scoping. A network is a chain, already modelled by@domain/entity-currency-crypto, soNetworkInfostays a wire type resolving to that rather than duplicating the concept.CurrenciesOrderis{ key, order, metaCurrencyIds }— server sort metadata, not a business object. Agreed with @gre and Yoann; see DADA DDD compliant.dadaIdToMarketId()and the market item type are copied intointernals/market.tsrather than imported, because adomain/*package must not import legacylibs/*. The drift risk is documented in the file and the README:PartialMarketItemResponseisPartial<MarketItemResponse>, so every field is optional and future divergence will never produce a type error. Carries a TODO pointing at a future market entity.reducerPathstays the literal"assetsDataApi", now pinned by a test.createCurrencyDataSelectorhand-scans that key as a string and three Storybook files preload it, so a rename would silently returnundefinedfor every market and interest-rate lookup with no type error. Any rename belongs to LIVE-35301.internals/is exported, despite the name.chunkCurrencyIds,mergeAssetsDataPagesanddadaIdToMarketIdare needed by the platform-layer hooks andassetDiscoveryin LIVE-35227, so they are public with a comment explaining why. Flagging since the directory name now slightly overstates.What was deliberately not touched
convertApiAssets— unconvertible tokens are still silently dropped, and cryptos missing from the local CAL are still synthesised rather than dropped. That leniency is load-bearing and now carries a comment saying so.getChunkedAssetsDatastill succeeds if any chunk resolves. Portfolio distribution depends on it.id: ""still fails the entire query. Pinned as current behaviour; fixed in LIVE-35233.injectEndpointsper-use-case split is LIVE-35301.Verification
domain/api/aggregated-assets/src/transforms.ts, which confirms they exercise the moved code through the shims rather than passing vacuously.libs/ledger-live-commontypechecks with 0 errors.nx show projectslists all four; oxfmt and oxlint clean;commitlint --from origin/developpasses.dada-client, the three target packages, and the two manifests is touched.Pre-existing issues encountered, not caused by this PR
libs/ui/packages/react/libis unbuilt, so all 62 suites fail intests/jestSetup.json@ledgerhq/react-ui/assets/fonts. I verified consumers still resolve via desktop typecheck instead.dada-client/entities/selectorUtils.ts:34(untyped cache scan). That file is byte-identical to develop and imports only@reduxjs/toolkit, so the error is pre-existing — and a fitting demonstration of why this code is guarded by tests rather than types.libs/asset-detailreports 118 implicit-any errors, all in unrelated live-common files (account/formatters.ts,hw/deviceAccess.ts, exchange, bridge). None mention this code.❓ Context
🧐 Checklist for the PR Reviewers