Skip to content

Denario onboarding: clean up the five review follow-ups from #4717 - #4876

Open
joshuakrueger-dfx wants to merge 2 commits into
developfrom
fix/denario-onboarding-cleanup
Open

Denario onboarding: clean up the five review follow-ups from #4717#4876
joshuakrueger-dfx wants to merge 2 commits into
developfrom
fix/denario-onboarding-cleanup

Conversation

@joshuakrueger-dfx

@joshuakrueger-dfx joshuakrueger-dfx commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Cleans up the review follow-ups Mara Steiner filed in #4850 while reviewing #4717 — none behaviour-changing.

Not symptom-driven: Follow-up to code-review findings from #4850, not a reported production issue.
Scale: Three independent items in seven files, all inside the surface #4717 introduced; no repo-wide pattern to sweep.
Smaller fix considered: Doing nothing was considered and rejected for items 1–3 — CONTRIBUTING explicitly requires stale-comment removal and eslint-disable justification, and the leaked env var is a real cross-test hazard. Items 4 and 5 are deliberately left as the smaller (no) fix, reasoned in the PR body below.

What

  1. Stale comment. migration/1786384000000-EnableDenarioAssetsOutsidePrd.js:18-19 still described "both directions priced off the Denario ask", superseded later in the same PR by SetDenarioSellPriceSource and PricingDenarioService. Deleted, no replacement.
  2. Leaking test env vars. pricing-denario.service.spec.ts set DENARIO_PRICE_URL/DENARIO_PRICE_API_KEY in beforeAll and never restored them. Added afterAll restoring the pre-suite values (or deleting, if they were unset before).
  3. Unjustified eslint-disable. The five new Denario migration specs from Onboard the Denario tokens on dev: list, price two-sided and enable trading outside prd #4717 had a bare // eslint-disable-next-line @typescript-eslint/no-require-imports. Added the same justification already used elsewhere in the repo for this exact pattern (plain CommonJS migration module).
  4. Import order — deliberately NOT changed. Sorting PricingDenarioService in pricing.service.ts / pricing.module.ts would touch two unpinned production files. CONTRIBUTING requires 100% coverage of every touched file, including import-only edits. Bringing those files to 100% and pinning them is a coverage rewrite of the pricing service, outside this follow-up's scope.
  5. Missing column length — deliberately NOT changed. PriceRule.sellPriceSource has no length, like all eight sibling string columns on the same entity (priceSource, priceAsset, priceReference, check1*/check2*), and the migration mirrors them with a plain character varying. Fixing only the new column would make the entity inconsistent; the right fix is either the CONTRIBUTING rule or the whole entity, both outside this follow-up's scope.

Closes #4850.

Migration file touched — comment-only

migration/1786384000000-EnableDenarioAssetsOutsidePrd.js is already merged (via #4717), so the
"Migration immutability" CI job only allows whole-line comment edits to it. Verified locally with
the exact diffing the job uses:

diff -q <(git show origin/develop:<file> | grep -vE '^[[:space:]]*//') \
        <(git show HEAD:<file>            | grep -vE '^[[:space:]]*//')

Identical — no executable line changed, only the three stale comment lines were removed.

Verification

  • Studio, head bc21163fc: prettier --check on the seven touched files (clean), eslint on the six specs (empty), npm run type-check (clean).
  • Studio focused Jest: 5 suites / 76 tests passed (pricing-denario.service.spec.ts plus the four Denario / sell-price-source migration specs).
  • Production files pricing.service.ts and pricing.module.ts are no longer in the diff (TaprootFreak review, 2026-08-13).
  • Not verified: add-price-rule-sell-price.migration.spec.ts was prettier/eslint-checked but not in the focused Jest run (comment-only eslint-disable justification, same pattern as the four that ran). No full-suite run outside this repo's own CI.

Final pass (bc21163):
Coherent: Every remaining file serves the same purpose — closing the hygiene items #4850 filed against #4717 that can be done without touching unpinned production files: one stale comment, one test env-var leak, five missing eslint-disable justifications.
Nothing extra: No import-order change (item 4) and no column-length change (item 5) — both left as the smaller (no) fix, because each would force a whole-file coverage rewrite outside this follow-up.
Sources closed: #4850 items 1–3 implemented. Items 4–5 deliberately not implemented, reasoning stated above. TaprootFreak CHANGES_REQUESTED (coverage of the two import-reordered production files) addressed by dropping those files from the PR. Mara Steiner's prior approve was on 3530767ec and is requested again on this head. CI: 15/15 checks green on bc21163fc, including Build and checks, Migration immutability, Coverage ratchet (94 unpinned-complete files, none of them in this diff) and Full-stack E2E.

Follow-up to #4717 (review by Mara Steiner).

- Delete the stale comment in EnableDenarioAssetsOutsidePrd superseded by the
  ask-on-buy/bid-on-sell split added later in the same PR.
- Restore DENARIO_PRICE_URL / DENARIO_PRICE_API_KEY after the Denario provider spec
  instead of leaking them into later suites.
- Justify the eslint-disable for the CommonJS require() in the five Denario
  migration specs, matching the repo's existing precedent.
- Sort the PricingDenarioService import alphabetically in pricing.service.ts and
  pricing.module.ts.

Not changed: the missing column length on PriceRule.sellPriceSource. All eight
sibling string columns on the same entity share the same declaration; changing only
the new column would make the entity inconsistent, and fixing that consistently is
outside this follow-up's scope.

Closes #4850.
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as ready for review August 12, 2026 09:43
@marassteiner

Copy link
Copy Markdown
Collaborator

Sorry — I've reached my hourly job limit and can't take this on right now. I'll pick it up automatically once the limit resets.

@joshuakrueger-dfx
joshuakrueger-dfx requested review from marassteiner and removed request for marassteiner August 12, 2026 15:23
@marassteiner

Copy link
Copy Markdown
Collaborator

EN: Working on this now — job dfxswiss__api__4876__pr-review.
DE: Ich arbeite jetzt daran — Auftrag dfxswiss__api__4876__pr-review.

@marassteiner marassteiner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review verdict: approve

No merge-blockers. Hygiene-only follow-up to #4717 / closes #4850; CI green on the reviewed head (3530767ecc).

Diff review (conformance + correctness)

Both review dimensions completed on the full origin/develop...HEAD diff (9 files, +17/−5).

Findings: 0

Checked explicitly:

Item Result
Stale migration comment removal (EnableDenarioAssetsOutsidePrd) Comment-only; executable migration lines unchanged (immutability-safe)
afterAll restore of DENARIO_PRICE_URL / DENARIO_PRICE_API_KEY Capture before mutation; restore vs delete for previously unset keys is correct
eslint-disable justifications on the five CommonJS migration specs Matches existing repo precedent; no-op on runtime path
Alphabetical import of PricingDenarioService Import reorder only; DI / providers array unchanged
Item 5 (sellPriceSource column length) left unchanged Consistent with sibling string columns; no correctness risk in this PR

Local run (Gate B)

Evidence from this branch’s build and process:

  • Focused Jest on all touched specs: 6 suites / 77 tests passed
  • npm run builddist/src/main.js produced
  • API boot (node dist/src/main, ENVIRONMENT=loc, port 3020): log line Nest application successfully started
  • GET /health → HTTP 200, database.status=ok (overall degraded expected locally: disabled processes / no node-payment-liquidity observers)
  • GET /swagger-json → OpenAPI 3.0.0, title DFX API, 192 paths

Merge-blockers

None.

Follow-up issues

None opened — no follow-up findings from either review dimension.

Not merging.

@TaprootFreak TaprootFreak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EN: Changes required: this PR touches pricing.service.ts and pricing.module.ts without bringing them to 100% or pinning them in the coverage ratchet.
DE: Änderung zwingend erforderlich: Dieser PR fasst pricing.service.ts und pricing.module.ts an, ohne sie auf 100% zu bringen oder im Coverage-Ratchet zu pinnen.

Details

CONTRIBUTING.md § Test Coverage applies to the whole file, not only the changed lines. Touching a file makes you responsible for it. Import-only edits still count.

Touched production files not pinned on this head or on develop:

  • src/subdomains/supporting/pricing/services/pricing.service.ts (import reorder)
  • src/subdomains/supporting/pricing/pricing.module.ts (import reorder)

jest.coverage-gate.config.js is not in the PR.

Required: either

  1. bring both files to 100% on all four metrics and pin them here (pricing.module.ts in the declarative list if it has no branches, pricing.service.ts in PINNED_LOGIC), or
  2. drop those files from this PR (keep the import order as on develop) and land the comment-only migration/spec cleanups without touching unpinned production files.

E2E/handbook: not applicable. Migration specs already exist; this PR only adds comments there.

CONTRIBUTING requires 100% coverage of every touched production file.
The alphabetical import of PricingDenarioService was the only change in
pricing.service.ts and pricing.module.ts. Reverting those two files keeps
the comment-only migration and spec cleanups without a coverage rewrite
of the pricing service.

Item 4 of #4850 is left as the smaller (no) fix, same reasoning as item 5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Denario onboarding: stale comment, leaked test env vars, unjustified eslint-disable, import order, column length

3 participants