Skip to content

feat: DFX App 2.0 preview at /app2#1162

Open
joshuakrueger-dfx wants to merge 24 commits into
DFXswiss:developfrom
joshuakrueger-dfx:feat/app2-preview
Open

feat: DFX App 2.0 preview at /app2#1162
joshuakrueger-dfx wants to merge 24 commits into
DFXswiss:developfrom
joshuakrueger-dfx:feat/app2-preview

Conversation

@joshuakrueger-dfx

@joshuakrueger-dfx joshuakrueger-dfx commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

DFX App 2.0 — React preview at /app2/

This PR ships the standalone App2 React target alongside the existing production app for stakeholder review. App2 is built from src/app2/, uses the shared @dfx.swiss/react API layer, and is deployed as its own isolated artifact under /app2/.

What ships

  • React App2 target: hash-routed buy, sell, swap, account, transaction, KYC, support, and wallet-connect flows.
  • Isolated build artifact: scripts/build-app2.sh produces app2-dist/; scripts/postprocess-app2.js removes shared main-app public files, stages only App2 PWA assets, and injects App2's CSP.
  • CI/CD integration: PR CI builds App2; DEV and PRD build app2-dist/, stage it at build/app2/, upload it to /app2/*, and re-upload app2/index.html with no-cache, must-revalidate.
  • Single implementation: the legacy public/app2/ static app, vendored browser bundles, and vendored WebAssembly are deleted.

Review hardening

  • Corrected fiat direction flags, Bank-only payment methods, mandatory sell IBAN gating, full beneficiary details, full JWT chain reachability, and unreachable-chain filtering.
  • Made quote/gate errors visible and actionable, keyed errors to current inputs, stopped background quotes for inactive modes, and kept payment snapshots frozen except for explicit retries.
  • Cleared persisted WalletConnect sessions before pairing and on logout; cancellation now covers pairing, signing, and auth; injected addresses are checksummed and account/chain changes invalidate the session.
  • Added safe KYC portal handoffs for token/data steps, support delivery tracking with failed-message retry, working PDF/document downloads, bounded transaction-history windows, correct referral links, and mode-aware drawer navigation.
  • Added a branded error boundary, inert/focus-trapped sheets, consolidated HTTPS URL validation, structured API error mapping, and translated fallbacks without raw server-message leakage.

Security and artifact isolation

  • App2's generated HTML has an explicit CSP, external scripts only, strict referrer policy, and its own manifest/icons.
  • The artifact contains no nested app2/, main-app manifest/logo/version files, legacy vendor bundles, or vendored WASM.
  • The PWA uses document-relative URLs and a hash route, so refreshes remain subpath-safe.

Verification

  • npm run lint
  • npm test -- --runInBand29 suites, 318 tests, including 8 App2 review regressions
  • npm run build:dev
  • npm run widget:dev
  • npm run app2:dev
  • npm run app2 (production)
  • DEV/PRD/PR workflow YAML parsed successfully
  • Generated artifact checked for CSP, external-only scripts, complete asset references, App2 license/PWA files, and absence of shared/nested artifacts

Deploy behavior

On merge to develop, DEV publishes dev.app.dfx.swiss/app2/index.html. The production workflow publishes the same isolated React target to app.dfx.swiss/app2/index.html. The existing root app remains in place.

Ships the new standalone single-file DFX app under public/app2/, served
from the same $web static site at /app2/index.html on dev and prod.

- Coexists with the current React app; no changes to src/, build config,
  or the SPA-fallback routing. Entry is the explicit /app2/index.html.
- All asset paths are document-relative and the service worker is scoped
  to root only, so the app runs unchanged under the subpath.
- Adds an app2 upload step to dev.yml and prd.yml (mirrors the widget
  upload); the entry HTML is uploaded no-cache so redeploys are fresh.
- Fix DOM XSS (js/xss-through-dom): the image-fallback handler no longer
  assigns a DOM attribute value to img.src; the mono placeholder is rebuilt
  via mono(), whose encodeURIComponent neutralises any injected markup.
- Fix prototype pollution (js/prototype-polluting-assignment x2): the wallet
  switcher lookup uses a null-prototype object, so URL-seeded addresses such
  as __proto__ can no longer reach Object.prototype.
  Verified with CodeQL 2.25.6 (javascript-code-scanning): 0 alerts.
- Add SIL OFL 1.1 license for the self-hosted Inter fonts and a
  THIRD-PARTY-NOTICES.md for the vendored libraries.
- Deploy the preview on dev only (remove the app2 upload from prd.yml).
Model which network the CONNECTED wallet can actually settle on (Lightning-
aware) instead of treating every session chain as an interchangeable receive
target:
- Lightning is offered only for a Lightning identity (LNURL/LNNID) or a pure-
  Lightning session — never for an on-chain SegWit (bc1) address, which is
  paired with Lightning in the JWT but cannot receive on it. Fixes the BTC
  buy defaulting to / offering the Lightning network.
- Swap now validates the receive chain (chain2) too, prompting a wallet
  connect instead of letting the output land on an unreachable chain.
- Mode switches and defaults pick a wallet-reachable chain, not the first one.
- Asset-picker filter chips are derived from the reachable pool (no dead-end
  'no assets found' on single-chain wallets) with an 'All' chip and a helpful
  'not available with your wallet' hint; sell fee never mislabels a crypto
  amount with a fiat symbol; corrected the 'we flag the cheapest' copy and the
  scary pre-filled sell/swap amount.

CodeQL javascript-code-scanning: 0 alerts. Offline suite: 144 passed.
New DFX users auto-register on connect, but the backend blocks trading until a
referral is submitted (RecommendationRequired). Instead of linking out to
app.dfx.swiss, the confirm-sheet gate now:
- shows an 'Invitation needed' state explaining a referral is required,
- takes the invitation/referral code OR email inline and submits it to the KYC
  Recommendation step (PUT the step session URL with { key }),
- falls back to the full KYC flow when prerequisite steps come first,
- maps 404/400 to clear 'no matching customer' / 'invalid code' messages,
- tells the user the referral must still be confirmed by whoever invited them.
The KYC Recommendation step form gains the same lead + confirmation note.

CodeQL javascript-code-scanning: 0 alerts. Offline suite: 146 passed.
… account

- The drawer's OpenCryptoPay entry used a generic cube glyph; it now renders
  the actual OCP brand mark (buildMenu gains full-SVG icon support).
- Account 'Display currency' was hard-wired to EUR; it now reflects the
  selected fiat (updated in renderAccount and on currency change).

CodeQL: 0 alerts. Offline suite: 145 passed.
Replace the muddy squiggle glyph on the account Display-currency row with the
universal currency sign — a clean circle with four rays — that reads clearly
at icon size and matches the line-icon set.
…more blurry flag)

Replace the low-res flag png in the buy/sell/swap fiat pill and the currency
picker with a sharp SVG coin showing the currency symbol (EUR €, CHF Fr, USD $,
GBP £), matching the round crypto coins. Language picker keeps its flags.
Bring back the flags (EUR/CHF/USD/GBP) but render them as inline SVG instead of
<img src>: an img rasterises the SVG at CSS-px size and looks blurry on retina,
while inline SVG renders at device resolution and stays crisp. Each flag uses a
unique mask id so multiple inline flags don't collide on a shared '#a'. Currency
symbol coin remains the fallback for currencies without a bundled flag.
…n fallback

Harden shownChainsFor/supportedChainOf so a Taproot/on-chain BTC address never
sees Lightning as a network even when the session's blockchain list is empty or
unexpected (the previous fallback returned all of the asset's chains).
…nal-IBAN overclaim

The pay box hid the reference and claimed 'This is your personal IBAN' whenever
the API flagged isPersonalIban — misleading (most users have no personal IBAN)
and money-critical: if a shared/collection IBAN ever carried isPersonalIban the
required reference would be hidden and the transfer stranded. Now the reference
is shown whenever the API returns a remittanceInfo (the authoritative signal),
the label stays a neutral 'IBAN', and a no-reference IBAN just says 'no payment
reference needed' without claiming it's personal. Added a money-safety test.
…es=)

When the embedding wallet reports balances via the ?balances=amount@asset
contract (DFX's own primary balance source), the sell / swap-source picker now
lists ONLY the assets actually held, sorted by balance, with the amount shown
per row. Without balance data (e.g. a bare hardware-wallet connect, no contract
addresses available client-side) it falls back to all sellable assets.
The API's Swiss QR-bill SVG was inlined via innerHTML into a fixed 158x158
box that forced a non-square aspect (distorted modules) and was too small for
a dense Swiss QR to scan. Now it renders as a data-URI <img> at 220px with
object-fit:contain (never distorted), matching the production app; GiroCode
text still gets encoded locally. The QR content itself is API-provided.
…i18n keys

- The account Display-currency icon was a static € even when the selected fiat
  was CHF/USD; it now follows the currency (€/Fr/$/£), matching the value.
- Remove the now-unused yourIban / personalIbanNote strings left over from the
  remittanceInfo-driven IBAN rework.
…ache CSP breakage

On localhost the app no longer registers the SW and actively unregisters any
leftover worker + clears caches. A stale cached index.html served by an old SW
had a CSP hash that no longer matched the inline script, so the browser blocked
ALL app JS — connect-wallet and every button went dead. Root cause removed for
local dev; prod/root still gets the SW.
- The asset-picker 'not available with your wallet' message wrongly appeared
  when a category chip simply filtered out a reachable asset during search
  (e.g. Bitcoin chip + 'ETH'); it now shows plain 'no results' and only claims
  wallet-incompatibility when the asset is genuinely unreachable.
- Escape the currency symbol in fiatGlyph, matching curSymSvg.
…verflow the rounded corners

.panels had border-radius but no overflow:hidden, so the darker You-receive
panel's square background poked past the rounded bottom corners. Added
overflow:hidden (fab is centered, box-shadow is unaffected).
@Danswar

Danswar commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Review notes

Scope check: the diff outside public/app2/ is limited to dev.yml (prd.yml nets to zero — the app2 upload was added then removed in a later commit, confirmed no leftover references). Everything else is new assets plus one file, public/app2/index.html, which is the entire app (~3,600 lines) inside a single inline <script> tag.

Findings

  1. No test files in the diff. Several commits claim an "offline suite" (144/145/146 tests) and a dedicated "money-safety test" for the IBAN/remittanceInfo fix, but the PR contains zero test files. As it stands these claims aren't reproducible or CI-enforced.
  2. CSP script hash is hand-maintained. script-src pins the inline script via a literal sha256-... hash (verified it currently matches the script body byte-for-byte). Every future edit to that script requires manually regenerating this hash, or the browser silently blocks all app JS with no console error explaining why — this is the same failure class the "no service worker on localhost" commit had to patch around from the caching side. Worth a CI check that fails the build if the hash goes stale.
  3. Cache-header ordering clobbers the wasm rule. dev.yml uploads all *.wasm first with max-age=31536000, immutable, then the new app2/* upload-batch step (--overwrite) matches recursively and re-uploads assets/vendor/bitbox_api_bg.wasm with only max-age=3600. Minor, but the immutable caching intended for wasm assets doesn't apply under /app2/.
  4. A couple of hrefs built from API-returned URLs are HTML-escaped but not scheme-validated (e.g. the KYC identification link). esc() neutralizes markup but wouldn't stop a javascript: URL if the API ever returned one. Low risk given the source is first-party, but cheap to add an ^https?:\/\// check.

What looks solid: isolation from the existing app is real (verified no src/ changes, no leftover prd.yml references), the esc() helper is applied consistently across the innerHTML call sites, the prototype-pollution guard (null-prototype map for address lookups) is correctly used, and the Lightning chain-reachability / IBAN reference logic matches their commit descriptions on inspection.

One architectural question worth settling before this grows further: the app doesn't reuse any of the existing codebase's wallet-connect, chain-config, or KYC-flow logic — it's a from-scratch reimplementation, which is how it ends up re-solving (and re-introducing bugs in) problems the existing app already fixed. That's a reasonable tradeoff for a short-lived stakeholder-preview spike, but the PR description's own "ahead of any root cutover" framing suggests this might be heading toward production. Worth confirming the intended lifecycle now — disposable preview vs. seed of the real rewrite — since that changes how much investment (tests, shared logic, maintainability) makes sense here.

@Danswar

Danswar commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Architecture suggestion — keep the isolation, gain reusability

Following up on my review notes: the standalone approach makes sense for a fast stakeholder preview, but if there's any chance this graduates ("ahead of any root cutover"), the current shape — the full app hand-written in one inline <script>, with its own reimplementation of auth/session, buy/sell/swap, KYC, and payment routes — means everything gets thrown away or rewritten at exactly the moment it succeeds. There's a way to keep the same deploy isolation while making the work promotable, and this repo already contains all the pieces.

Two observations from the repo:

  1. @dfx.swiss/react already provides, headless (no UI), everything this app reimplements: auth, api-session, buy, sell, swap, asset, fiat, kyc, bank-account, payment-routes, support (tickets/chat), transaction, user, language. The API semantics this PR had to rediscover commit-by-commit (Lightning reachability, remittanceInfo/IBAN handling, KYC step flow) are already encoded and battle-tested behind those hooks.
  2. The repo already builds multiple artifacts from one codebase — the main app, the widget (index-widget.tsx + build-widget.shbuild/widget/, deployed by the same upload step this PR mirrors), and the npm lib (Main.lib.tsx@dfx.swiss/services-react). A fourth entry point is the established pattern here, not a new concept.

Proposal: build the new app as a fourth build target

  • src/app2/ (name TBD, see below) — the new UI written fresh in React + TypeScript. No imports from the existing app's screens//components/, so the visual/UX experiment stays clean-room.
  • All state/API/wallet logic from @dfx.swiss/react and the repo's existing wallet-connect hooks — no reimplementation of money-path semantics.
  • Entry point + build script cloned from the widget pattern (index-app2.tsx, scripts/build-app2.sh with BUILD_PATH=./app2), output landing in build/app2/the deploy step in this PR then works completely unchanged (same pattern allowlist, same explicit entry path, SPA fallback untouched).
  • Zero new dependencies. Every runtime dep the app needs is already in package.json (React, the DFX hooks, Ledger/Trezor/BitBox, WalletConnect, wagmi/viem, Solana/Tron, QR). Notably, the current PR is what adds duplicate dependencies: assets/vendor/wc-provider.js, ledger.js, bitbox_api_bg.wasm, and qrcode.js are frozen copies of libraries npm already manages — outside npm audit, lockfile, and version updates.

What this buys:

  • Same isolation: separate artifact, separate path, root app byte-for-byte untouched — identical to this PR's deploy story.
  • CSP hash problem disappears: bundled, content-hashed JS under script-src 'self' instead of a hand-maintained inline-script hash that silently bricks the app when it goes stale.
  • CI-enforced quality: the code falls under the repo's existing TypeScript, ESLint, Jest, and Playwright setup (extend the lint globs, point e2e specs at the subpath) — replacing the uncommitted "offline suite" with tests that run on every PR.
  • Mechanical promotion: if the experiment wins, cutover means pointing the root build at the new entry — components are already typed and wired to the production logic layer. If it loses, delete one folder.

An optional step further would be giving the new entry its own Vite config (two devDependencies) instead of a third instance of the index-swap build hack — also a low-stakes pilot for eventually leaving CRA/react-app-rewired, which is deprecated upstream. But the zero-new-tooling variant above already delivers the main value.

Naming: reconsider app2

Cheap to change now, expensive after preview links circulate — the string is about to be baked into the CDN path, the workflow pattern allowlist, and every bookmark stakeholders share for sign-off.

  • It names the app by sequence number rather than role, unlike the repo's existing artifacts (widget, lib). If the experiment dies, /app2 is a dangling number; if it wins, the folder is called app2 while being the app — either way the name is wrong at end-of-life.
  • "App 2.0" is also a branding/versioning claim that probably belongs to a product decision, not a folder name.

Suggestions: /next (truthful through the whole lifecycle — it's "the next app" while experimental, and the path simply retires at cutover; cf. react.dev/Reddit/YouTube shipping under next/new paths) or /preview (matches exactly what the PR description says it is). Happy to discuss.

@joshuakrueger-dfx

Copy link
Copy Markdown
Contributor Author

@Danswar Thanks for the review — both the findings and the architecture proposal were on point. Rather than argue, I built it: your fourth-build-target proposal is now implemented on feat/app2-react (diff vs develop).

What it is — exactly the shape you sketched:

  • src/app2/ — the App 2.0 UI rebuilt in React + TS, visually 1:1 with the preview (CSS ported verbatim, 650 i18n keys × 4 languages, landing/connect/trade/account/tx/KYC/support).
  • All state/API/money logic from @dfx.swiss/reactuseBuy/useSell/useSwap, useAuth/useApiSession, useKyc, useTransaction, useSupportChat. Zero hand-rolled fetches, zero reimplemented API semantics.
  • scripts/build-app2.sh + npm run app2[:dev|:loc], cloned from the widget pattern (BUILD_PATH, index swap). Hash routing, document-relative assets — the artifact runs unchanged under /app2/, deploy step stays as-is.
  • Zero new dependencies, zero vendored bundlesassets/vendor/* is gone; WalletConnect/Ledger/BitBox/QR all come from the existing lockfile.

Your findings, addressed by construction or directly:

  1. Tests — the code now falls under the repo's TS/ESLint/Jest/Playwright setup; porting the offline suite into CI is the immediate next step on this branch.
  2. CSP hash — no inline script anymore, so the hand-maintained hash ceases to exist.
  3. wasm cache ordering — moot once the static tree is replaced by this build (no vendored wasm under app2/).
  4. Scheme validation — every href built from API data goes through an https-check helper (KYC ident link included).

Login slice so far: MetaMask + WalletConnect v2 + email magic-link (signInWithMail), invite-code gate; the other wallet methods are rendered as "coming soon" until their connectors are ported. On-chain sell/swap signing and the full KYC data-entry wizards are the remaining gaps. An adversarial review round (money-path, auth, e2e smoke on both viewports) ran against the branch — findings like a WalletConnect cancel deadlock and a quote-refresh race on the payment sheet are already fixed in the second commit.

Open question where I'd like your take: keep shipping under /app2 for link-compat with this preview, or take the rename window now (/next or /preview) before stakeholder links spread?

@Danswar

Danswar commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Keep /app2 — the preview links already circulating outweigh any naming win, and a later rename is cheap via redirect if it ever matters. Your feat/app2-react commits are now on this PR (c2c8305..18b2c29, fast-forward).

@Danswar Danswar 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 round 2 — React build target (3e398fc4, 18b2c291)

The architecture is what was proposed, and a lot of it verifies clean (credit list below). But the PR currently contradicts its own description on the most important point, and the money path has two flow-bricking bugs plus a cluster of inverted API semantics — several of them regressions of bugs the static preview's own commit history already fixed once. Requesting changes.

The structural problem: both apps ship, and only the old one deploys

  1. build-app2.sh outputs to app2-dist/ (gitignored) — no workflow builds or uploads it. dev.yml uploads ./build with --pattern "app2/*", which is CRA's verbatim copy of public/app2/ — the static preview. Through CI, /app2/ still serves the vanilla app; none of the React work reaches users.
  2. public/app2/ is fully intact at head (94 files, 4.9 MB, including all of assets/vendor/ — 3.3 MB). "assets/vendor/* is gone" doesn't hold: the PR contains zero deletions.
  3. Consequently round-1 findings 2 and 3 are not moot — the hand-maintained CSP hash and the wasm cache-order clobber (dev.yml:117 uploads *.wasm immutable, :135 re-uploads app2/assets/vendor/*.wasm at max-age=3600) are both still live in what deploys.
  4. The React artifact itself (built locally — CI never has): 8.2 MB, with the full 5.2 MB static tree embedded at app2-dist/app2/ (CRA copies public/ wholesale into BUILD_PATH), plus the main app's favicon.ico/logo.png/manifest.json. And it ships no CSP at all — the strict policy exists only in the static index.html. On the CSP axis the React artifact is currently a regression, not a fix.
  5. Round-1 finding 1 (tests) is still fully open: zero test files, npm run test is --passWithNoTests, and pr.yml doesn't run app2:dev — green CI proves typecheck and nothing more for ~10.7k new lines of money-path UI.
  6. The PR description still describes the static single-file drop-in — worth rewriting so approvals are based on what the PR now is.

Code findings

Details are inline. The two critical ones:

  • New users are bricked in all three trade modes: every gate that explains/unblocks (email verification, KYC/limits, min/max) renders only inside the PaymentSheet, which only opens on a valid quote — and a new user's every quote fails (EmailRequired/RecommendationRequired). Disabled CTA, "quote error", no way forward. (home.tsx:312)
  • Persisted WalletConnect sessions survive logout and pre-empt new pairings: silent auto-login as the previous wallet on shared machines, and a self-bricking hang after a phone-side disconnect, with no reachable code path that clears the stale session. (providers.ts:127)

High-severity, inline: fiat buyable/sellable filters inverted against the API convention (home.tsx:615); card/instant offered although the API rejects card unconditionally (PaymentMethodPicker.tsx:45); chain reachability wrong in both directions — blockchains[0] hides valid EVM chains (session.tsx:190) while the asset-pool fallback re-offers Lightning to non-LN wallets (asset-pool.ts:66); SEPA details missing the beneficiary (PaymentSheet.tsx:413); sell quotes fired without the mandatory IBAN and the 400 latched into the payment snapshot (useTradeQuote.ts:60); the cancel fix only covers the QR phase (session.tsx:238); injected addresses sent to /auth lowercase (providers.ts:102); Sumsub ident sessions dead-end silently (kyc.tsx:58); KYC data-entry steps loop with no portal link (kyc.tsx:366); failed support messages silently lost (support.tsx:255); PDF attachments can't be opened (support.tsx:507).

Not inline but worth fixing in the same pass: no accountsChanged/chainChanged handling anywhere in src/app2 (switching MetaMask accounts leaves the session — and sell instructions — on the old address; production re-verifies, src/hooks/wallets/metamask.hook.ts:111); no error boundary above the router (App.tsx — a provider throw is an unbranded error page in a money app); the Sheet background is neither inert nor focus-trapped, which is also what lets the "frozen" snapshot re-latch under an open sheet (home.tsx:287); ticket/thread load failures render as empty states ("No tickets yet") with unhandled rejections (support.tsx:167); transactions fetch and render the full history unbounded (transactions.tsx:132); the referral row copies the bare code while toasting "Referral link copied" (account.tsx:263); Drawer's Sell/Swap items route to / without setting the mode (Drawer.tsx:53) — decorative today.

Verified as claimed — credit where due

  • Zero hand-rolled fetches; zero new dependencies — both checked mechanically. All hook usage exists in the pinned lib version. (One version edge: err.code === 'TFA_REQUIRED' in kyc.tsx:145 needs ≥1.4.1 — fine for the locked build, silently dead when developing against a stale local install; worth a note.)
  • i18n claim verified: 659 keys × 4 languages, zero missing/mismatched, placeholder tokens consistent, all dynamic enum keys (kn_*, ks_*, is_*) match the lib exactly.
  • Round-1 finding 4 properly fixed: real URL parsing, rejects javascript:/data:/protocol-relative, applied at every API-derived href/window.open sink I could find — stricter than production, which iframes the ident URL unchecked. (Two identical copies of the helper exist — format.ts:89 and amount.ts:85 — merge them before they drift.)
  • No HTML-injection surface: no dangerouslySetInnerHTML/innerHTML in src/app2; the API-supplied QR-bill SVG is deliberately rendered via img data-URI; every target="_blank" carries noopener.
  • The quote engine's ordering discipline (monotonic seq + key-tied freshness) is sound; there is zero client-side money arithmetic on the payment path; copy buttons copy raw machine values; the magic-link flow returns to /app2/ (not the root app), scrubs credentials from the URL, and authMsgOk refusing to blind-sign an unrelated challenge is a protection most dapps skip.

What I'd gate merge on

  1. Resolve the two-apps state: wire app2:dev into pr.yml, deploy app2-dist in dev.yml, decouple build-app2.sh from public/app2, then delete the static tree.
  2. The two criticals + the fiat-flag inversion.
  3. A CSP for the React artifact (meta tag in the template or headers at the edge).
  4. A committed smoke-level test suite — the money-path fixes in this PR's own history are exactly the regressions tests should be pinning.
  5. Updated PR description.

Comment thread .github/workflows/dev.yml
Comment thread package.json
Comment thread scripts/build-app2.sh Outdated
Comment thread src/app2/screens/home.tsx
Comment thread src/app2/wallets/providers.ts
Comment thread src/app2/screens/kyc.tsx Outdated
Comment thread src/app2/screens/support.tsx Outdated
Comment thread src/app2/screens/support.tsx Outdated
Comment thread src/app2/wallets/providers.ts
Comment thread src/app2/screens/parts/Landing.tsx Outdated

Copy link
Copy Markdown
Contributor Author

@Danswar I addressed the complete round-2 review in commit 22dd0cc and updated the PR description to match the React build.

Highlights:

  • CI now builds and deploys the isolated app2-dist artifact in PR/DEV/PRD; the 94-file legacy public/app2 tree, vendored bundles, and WASM are removed. The generated React artifact has its own CSP/PWA identity and no nested/shared app payload.
  • Trade fixes cover actionable gate errors, corrected fiat semantics, Bank-only payments, mandatory-IBAN sell quotes, beneficiary details, current-key error handling, active-mode quote gating, full JWT chain reachability, and no unreachable-chain fallback.
  • Wallet fixes cover WC cleanup before pairing/on logout, cancellation through signing/auth, EIP-55 normalization, and injected account/chain invalidation.
  • KYC token/data steps now have safe portal handoffs; support tracks real delivery and retries failures, PDFs open with error handling; the remaining navigation, history, referral, error-boundary, Sheet focus/inert, and URL-helper findings are also addressed.
  • Added App2 regression coverage for the money-path semantics and invite normalization.

Local verification:

  • 29 test suites / 318 tests passed
  • lint passed
  • main DEV build, widget DEV build, App2 DEV build, and App2 production build passed
  • all three workflow YAML files and the generated CSP/artifact checks passed

I left the review threads open for your verification.

@Danswar Danswar 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.

@joshuakrueger-dfx

Review round 3 (22dd0cc)

Round-2 verification first: all 20 threads check out against the code — resolved. The structural fix is genuinely complete (CI builds and deploys app2-dist in all three workflows, static tree gone, CSP injected with fail-closed assertions), and the regression suite is real: every test imports the production modules, and this PR's own CI log shows PASS src/app2/__tests__/review-regressions.test.ts. Credit also for: i18n mechanically clean (660 keys x 4 languages, zero drift, all literal t() usages resolve), the clean-room import boundary holds (nothing escapes src/app2/ except the SDK and existing lockfile deps), the CSP checks out against the app's actual runtime traffic (API, WC relay/verify, blob attachments, self-hosted fonts), and build-app2.sh's EXIT trap correctly restores the entry swap on failure so a broken app2 build can't poison the widget build that runs after it.

This round: one finding I'd gate merge on, a band of mediums inline, and grouped smalls below.

Gate: the WalletConnect storage cleanup is a no-op after any page reload (providers.ts:228, details inline) — the round-2 fix only holds within a single page lifetime, and any reload re-opens the shared-machine path round 2 called critical.

Inline (mediums): URL-credential bootstrap dead-ends silently on RecommendationRequired; prod-hardcoded KYC/setup handoffs break the dev artifact; 2FA setup failure is an unrecoverable spinner; account-merge/switch responses loop; the sell sheet can auto-open unprompted; the buy rate renders a dangling slash; TranslationKey collapses to string; Drawer claims aria-modal without modal behavior; the postprocess identity strips are unasserted; support retry/timeout/empty-thread delivery gaps.

Smaller, fine as a fast-follow:

  • The swap rate is computed client-side (estimatedAmount / quote.amount, fee-inclusive) although the Swap response carries exchangeRate — the one breach of the zero-client-money-math claim (FeesPanel.tsx:86); the fees.bank ?? component-sum fallback on :83 is dead per the SDK type.
  • "1,000" parses as 1.0 (amount.ts:14-22) — the mismatch is visible in the receive field, but it yields a wrong-amount quote rather than a rejection.
  • AMOUNT_TOO_LOW/HIGH opens a gate titled "Setup required" with an external app.dfx.swiss CTA when the fix is editing the amount locally (PaymentSheet.tsx:315-338); the isCard branch at :364 is dead code now that Bank is the only method.
  • Transaction states render as raw enum strings in all four languages (transactions.tsx:273); the language switch never propagates to the API user (LanguageSheet.tsx:22-26), so server-driven mails/ident sessions stay in the account's old language.
  • CSP: https://pulse.walletconnect.org is missing from connect-src (telemetry fetch blocked, console noise on every WC connect); the single static CSP ships localhost:3000/dev-API entries in the prod artifact; and a meta-delivered CSP cannot carry frame-ancestors, so /app2 clickjacking protection needs a response header at the edge (infra, not this repo).
  • app2's content-hashed bundles upload at max-age=3600 instead of immutable, and app2 PNGs upload twice (86400 then 3600) — matches the widget precedent, perf nit only.
  • WC_OPTIONAL_CHAINS claims to mirror wagmi.config.ts but omits Citrea — harmless while app2 only does personal_sign; fix the list or the comment before the first on-chain feature.
  • No global 401-to-logout: an idle tab whose JWT expires stays visually logged in until the next data call. Shared gap with the production app, noting for parity, not as a regression.

One deploy-scope flag: this commit wires app2 into prd.yml for the first time — merging now publishes to app.dfx.swiss/app2/, where the previous state was dev-only. That matches the updated PR description; just making sure it's a conscious call.

* provider means the *next* connect attempt always starts from a fresh instance instead of
* reusing (and getting stuck behind) this one. */
export async function disconnectWalletConnect(): Promise<void> {
if (!wcProviderPromise) return;

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.

The WC cleanup is a no-op after any page reload — the shared-machine leak from round 2 survives via a different path. wcProviderPromise is module-level memory, so on a fresh page load this early-return fires while the previous session still sits in localStorage under wc@2:* — nothing in app2 ever clears those keys (clearStaleSessionOnCredentialedLoad removes only the DFX token and sessionStorage, and EthereumProvider.init runs with default persistent storage). Sequence: WC login -> reload -> logout (this function no-ops) -> next visitor clicks WalletConnect -> init() restores the persisted session and enable() resolves as the previous wallet, with no QR ever shown.

The round-2 fix holds within a single page lifetime only. Fix: when no live provider exists, clear the wc@2:*/@walletconnect storage keys directly (or unconditionally init-then-disconnect) — both here and on the logout path.

showToast(`${t('connected')} · ${shortAddress(creds.address)}`);
} catch (error) {
if (!stillCurrent()) return;
if (needsRecommendation(error)) {

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.

This branch dead-ends for the URL-credential bootstrap: the effect at :445 calls signInWith(...) with the connect sheet closed, and this catch sets the recommend view but never setSheetOpen(true) — and returns before any toast. An invite-gated user arriving via ?address=&signature= sees a silent no-op, and a later openConnect() resets the view to list, discarding the still-valid pending signature. setSheetOpen(true) alongside this setView fixes both.

Comment thread src/app2/screens/kyc.tsx
}

function portalKycUrl(code: string): string {
return `https://app.dfx.swiss/kyc?code=${encodeURIComponent(code)}`;

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.

Hardcoded prod origin: grep process.env src/app2 comes back empty, so the dev artifact on dev.app.dfx.swiss/app2 hands users to https://app.dfx.swiss/kyc?code=<dev-code> — a code the prod API has never seen, so the handoff dead-ends. Same for the setup-gate link in PaymentSheet.tsx:317. build-app2.sh sets a per-env PUBLIC_URL for exactly this purpose; nothing reads it. (The prod referral link in account.tsx:179 is arguably intended — referrals live on prod — but the KYC/setup handoffs need to be env-aware.)

Comment thread src/app2/screens/kyc.tsx
setPhase({ kind: 'tfa', info, alreadyEnrolled: false });
setBusy(false);
kyc
.setup2fa(code)

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.

A non-"already" setup2fa failure (network blip, 5xx, non-English server message) leaves setup: undefined, alreadyEnrolled: false — the render at :250 is then a permanent LoadingRow and the verify form stays gated behind (alreadyEnrolled || setup): no retry, no escape from the tfa phase. Separately, /already/i on a human-readable message is the same message-sniffing errors.ts just moved off of — branch on the structured code/status instead, and give the failure state a retry + back-to-overview affordance.

Comment thread src/app2/screens/kyc.tsx
</div>
<div className="sectionlabel tight">{stepNameLabel(t, step.name)}</div>
<div className="paybox-note warn" style={{ margin: '10px 0' }}>
{t('kycFailed')}

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.

Production's callKyc handles 401 with switchToCode, 409 exists/merge, and CONTACT_DATA FAILED with ACCOUNT_MERGE_REQUESTED (kyc.screen.tsx:256-270). App2 maps all of these to the generic genErr, and this FAILED branch offers only a Continue that re-runs continueKyc onto the same step — for a merge-requested account that's a closed loop with no path forward. At minimum, surface the merge/switch cases distinctly and link the portal here like the data-entry fallback does.

const rateStr = isSwap
? `${formatAmount(quote.amount ? quote.estimatedAmount / quote.amount : 0, 6, language)} ${receiveAssetCode} / ${payAssetCode}`
: mode === 'buy'
? `${formatFiat(quote.exchangeRate, currencyCode, language)} / ${payAssetCode}`

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.

Buy mode passes payAssetCode='' (home.tsx:626), so this renders € 95'000.00 / — dangling slash, no unit. The buy rate is currency-per-asset, so the unit here should be receiveAssetCode (production renders exchangeRate as currency/asset). The sell branch below it only works because payAssetCode happens to be the crypto code there.

Comment thread src/app2/i18n.tsx

type Dict = Record<string, string>;

export const en: Dict = {

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.

The explicit : Dict annotation widens en to Record<string, string>, so TranslationKey = keyof typeof en (:2736) collapses to string — the canonical key set the comment there describes doesn't actually exist, t('anyTypo') compiles, and a renamed key ships silently rendering the raw key literal. export const en = {...} as const satisfies Dict (or dropping the annotation) restores compile-time key checking. Today's key set is mechanically clean — 660 keys x 4 languages, every literal usage resolves — this is the guard that keeps it that way.

ref={ref}
className={`drawer${open ? ' on' : ''}`}
role="dialog"
aria-modal="true"

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.

aria-modal="true" with none of the modal behavior: unlike Sheet (ui.tsx: background inert walk, focus trap, Escape, focus restore), the open drawer doesn't inert the page behind it, trap focus, or close on Escape — useInertWhenClosed only inerts the drawer itself when closed. As-is the attribute misinforms screen readers (announces the background as inert when Tab happily traverses it behind the scrim). Either wire the Sheet's focus/inert machinery or drop the attribute.

html = html
.replace(/<link rel="icon" href="[^"]+"\s*\/?>(?:<link rel="icon"[^>]+>)?/, '')
.replace(/<link rel="apple-touch-icon" href="[^"]+"\s*\/?>/, '')
.replace(/<link rel="manifest" href="[^"]+"\s*\/?>/, '')

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.

These identity strips are attribute-order-sensitive and, unlike the loader/CSP strips at :93-96, unasserted — a template reformat or attribute reorder makes them silently no-op, and the built HTML then keeps the main app's <link rel="manifest" href=".../manifest.json"> before the injected ./manifest.webmanifest; first-manifest-wins means /app2 installs with the main app's PWA identity again — the exact class this PR just fixed. Cheap insurance: assert !html.includes('manifest.json'), !html.includes('fonts.googleapis.com'), and no absolute-origin apple-touch-icon, alongside the existing throws.


const send = () => startSend(composer.trim() || undefined, pendingFile, true);

const retryMessage = (failedMessage: SupportMessage) => {

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.

Three related delivery-tracking gaps: (a) a successful retry creates a new optimistic message but nothing settles or removes the original FAILED bubble — the thread permanently shows a failed copy whose still-live Retry delivers a duplicate to support; (b) on the 30s settle-timeout (:231-240) the optimistic bubble keeps rendering the "Sending…" suffix (:678) forever with no failure conversion or retry affordance; (c) setSync(!!activeUid) (:203) enables the SDK's 5s sync even for zero-message threads, and syncSupportIssue reads messages[messages.length - 1].id unguarded (support.context.js:111) — a TypeError per tick on exactly the empty threads the render at :533 acknowledges exist.

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