Skip to content

Address the release review findings for the Monerium EUR onramp - #1381

Merged
ebma merged 11 commits into
stagingfrom
fix/release-review-findings
Sep 19, 2026
Merged

ebma merged 11 commits into
stagingfrom
fix/release-review-findings

Conversation

@ebma

@ebma ebma commented Sep 19, 2026

Copy link
Copy Markdown
Member

Follow-up to the deep review of release PR #1379. One commit per finding.

Fixed

  • Uniswap swap resume (P1 before EUR enablement). UniswapSwapExecutor checked balance, allowance and quote before the idempotent broadcast(). A mined swap consumes the allowance, so a crash between settlement and phase completion paused every retry for reconciliation although the swap had succeeded. A successful receipt for the deterministic hash now skips the pre-flight checks. Regression test: a resumed swap with allowance 0 and a settled receipt completes without resending or simulating.
  • Widget reconnect cancel loop (P2). The Monerium completion guard in kyc.states.ts only checked status === "APPROVED", so cancelling the reconnect prompt of an approved profile with a lost session moved on to wallet linking, which failed on the same error. The guard now also requires no rampError, and the quote screen shows the reauthentication message. Test added in ramp.machine.test.ts.
  • Silent 5xx token refresh (P3). getValidCredentials keeps the session on an upstream failure on purpose, but nothing logged it. It now warns with the entity and upstream status, never the token. Test asserts the session survives, the refresh is retried, and the log carries no refresh token.
  • Wallet link without the profile lock (P3). linkMoneriumWallet now runs its list reads, link and IBAN request under the same profile advisory lock that IBAN moves and issue registration take. Test asserts the ordering lock → link → IBAN → unlock.
  • Unbounded dashboard poll (P3). MoneriumWalletLinkFlow stops polling GET /v1/monerium/status after 36 polls of 5 s, the widget's bound, and offers a "Check again" link. The interval logic is a pure helper with tests.
  • Dead MykoboFeeUnavailableError import removed from the quote service; the Monerium issue executor test reads the EURe address from MONERIUM_ISSUE_NETWORKS instead of a literal.
  • docs/security-spec/05-integrations/monerium.md invariants 18 and 21 updated for the resume rule and the wallet-link lock.

Deliberately not changed

  • Onboarding status Monerium refresh throttle. Throttling would require caching the readiness and reauthentication outcome for the TTL, which would delay the dashboard's reaction to a reconnect or a wallet link by up to a minute. The extra load is one status read plus two list reads per poll, only while another non-terminal onboarding keeps the poll alive.
  • Process-local NodeCache for OAuth state and credentials: single-instance deployment assumption, unchanged.
  • White-label 403 treated as "profile invisible" in identity.ts: documented design decision, unchanged.

Verification

  • apps/api: tsc --noEmit clean; 89 unit tests across the touched suites pass; hermetic eur-onramp-monerium.scenario.test.ts plus both Polygon flow tests pass (10/10)
  • apps/frontend: tsc --noEmit clean; vitest run src/machines/ramp.machine.test.ts 43/43
  • apps/dashboard: tsc --noEmit clean; wallet-step tests 7/7
  • bun verify clean

The swap executor checked balance, allowance and quote before reaching the idempotent broadcast. A mined swap consumes the allowance, so a crash between settlement and phase completion made every retry pause the ramp for reconciliation although the swap had succeeded. A successful receipt for the deterministic hash now skips the pre-flight checks.
…celled

The completion guard only looked at the APPROVED status, so cancelling the reconnect prompt of an approved profile with a lost session moved on to wallet linking, which failed on the same reauthentication error.
A 5xx from the token endpoint keeps the session for the next attempt on purpose, but nothing logged it, so a sustained outage on an expired token was indistinguishable from a blip.
IBAN moves and issue registration already serialise on the profile advisory lock; the wallet link mutated the same address and IBAN lists without it.
The widget bounds the same IBAN wait; the dashboard polled forever.
@netlify

netlify Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for vortexfi canceled.

Name Link
🔨 Latest commit 5d4549a
🔍 Latest deploy log https://app.netlify.com/projects/vortexfi/deploys/6aae8fd862f91000081e1803

@netlify

netlify Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for vortex-sandbox ready!

Name Link
🔨 Latest commit 5d4549a
🔍 Latest deploy log https://app.netlify.com/projects/vortex-sandbox/deploys/6aae919752c77d4c1cf1f3c2
😎 Deploy Preview https://deploy-preview-1381--vortex-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for vrtx-dashboard canceled.

Name Link
🔨 Latest commit 5d4549a
🔍 Latest deploy log https://app.netlify.com/projects/vrtx-dashboard/deploys/6aae8fd87071af00085ac9cd

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The polling budget can expire before provisioning begins, and swap recovery can still attempt to resend an already-mined transaction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Addresses release-review findings for the Monerium EUR onramp across API recovery, wallet coordination, and client flows.

Changes:

  • Adds settled Uniswap swap recovery and related specification coverage.
  • Improves Monerium reconnect handling and bounds dashboard polling.
  • Serializes wallet linking and adds OAuth refresh-failure logging.
File Description
docs/​security-spec/​05-integrations/​monerium.md Documents swap recovery and wallet locking.
apps/​frontend/​src/​machines/​ramp.machine.test.ts Tests reconnect cancellation routing.
apps/​frontend/​src/​machines/​kyc.states.ts Prevents wallet linking after reauthentication errors.
apps/​dashboard/​src/​components/​onboarding/​monerium/​walletStep.ts Defines bounded status polling.
apps/​dashboard/​src/​components/​onboarding/​monerium/​MoneriumWalletLinkFlow.tsx Applies polling limit and retry UI.
apps/​dashboard/​src/​components/​onboarding/​monerium/​MoneriumWalletLinkFlow.test.ts Tests polling helper boundaries.
apps/​api/​src/​api/​services/​quote/​index.ts Removes an unused import.
apps/​api/​src/​api/​services/​phases/​blocks/​phases/​uniswap-v3-fixed-swap/​execution.ts Detects previously settled swaps.
apps/​api/​src/​api/​services/​phases/​blocks/​__tests__/​uniswap-v3-fixed-swap.test.ts Covers resumed settled swaps.
apps/​api/​src/​api/​services/​phases/​blocks/​__tests__/​monerium-issue.executor.test.ts Uses configured EURe address.
apps/​api/​src/​api/​services/​monerium/​wallet.ts Locks wallet-link provider operations.
apps/​api/​src/​api/​services/​monerium/​wallet.test.ts Verifies lock ordering.
apps/​api/​src/​api/​services/​monerium/​monerium.service.ts Logs refresh failures safely.
apps/​api/​src/​api/​services/​monerium/​monerium.service.test.ts Tests refresh retry and redacted logging.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/api/src/api/services/phases/blocks/phases/uniswap-v3-fixed-swap/execution.ts Outdated
Comment thread apps/dashboard/src/components/onboarding/monerium/MoneriumWalletLinkFlow.tsx Outdated
broadcast() re-reads the receipt and treats a transient RPC failure as not mined, so entering it after the resume lookup could resend the mined transaction. One lookup now decides the resume.
dataUpdateCount is cumulative for the cached query, so time spent before linking or a quick reopen consumed the budget. The baseline now restarts on open, after a successful link or move, and on Check again.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Settled-swap recovery can complete while leaving its durable financial-operation journal entry in a submitted state.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread apps/api/src/api/services/phases/blocks/phases/uniswap-v3-fixed-swap/execution.ts Outdated
Skipping broadcast() also skipped runFinancialOperation, so a row left submitted by a crash between send and result persistence never became confirmed. The receipt read for the resume now feeds the journal's reconcile and perform callbacks, which confirms the row without a second RPC read or a resend; the receipt short-circuit that bypassed the journal is gone.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The recovery and locking changes are well-covered; only a non-blocking polling off-by-one remains.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Exclude initial status fetch from polling timeout count

apps/​dashboard/​src/​components/​onboarding/​monerium/​MoneriumWalletLinkFlow.tsx:52

dataUpdateCount includes the immediate status fetch made when the attempt starts, so this reaches the limit after only 35 five-second refetches (~175 s), one poll earlier than the widget's 36-poll/180-second bound. Exclude that initial fetch from both the interval and exhausted calculations so the implemented timeout matches the stated bound.

@ebma
ebma merged commit 57e35b7 into staging Sep 19, 2026
7 checks passed
@ebma
ebma deleted the fix/release-review-findings branch September 19, 2026 13:49
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.

2 participants