fix(kyc): existing DFX customer merge — close misroute, surface failures#466
Merged
TaprootFreak merged 3 commits intoMay 21, 2026
Merged
Conversation
This was referenced May 20, 2026
Contributor
|
Blume1977
force-pushed
the
fix/dfx-merge-registration-misroute
branch
from
d728979 to
146b707
Compare
Contributor
Author
|
Alle vier Compliance-Punkte adressiert:
Die zwei Mini-Concerns aus dem Review:
Dass diese Konventionen schon auf PR #461 angemahnt wurden und ich sie hier wiederholt habe, ist mein Fehler — die Regeln waren in meinem Memory dokumentiert. Habe sie zum Session-Start nicht abgerufen. Wird beim nächsten DFX-PR nicht mehr passieren. CI ist weiterhin |
Contributor
Author
|
Zu den Mini-Concerns: Rebase auf aktuelles develop nicht jetzt durchgeführt. Hab den Rebase versucht (auf Daher: Branch bleibt auf SHAs unverändert ( |
3 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 20, 2026
## Summary `develop` CI is red after #461 merged: `Run flutter test --coverage` reports `1342 tests passed, 17 failed`. All 17 are direct consequences of the new wiring shipped by #461 — they did not surface locally because Flutter's test runner parallelises per file and `GetIt`'s singleton state leaks across files differently than on CI. This PR repairs the test suite without touching any production code. ## Failure groups & fixes ### 1. `PaymentInfoError` variant count drifted (1 test) `test/packages/service/dfx/models/payment/buy_sell_dtos_test.dart` pins the enum to "four documented variants". #461 added `bitboxDisconnected` (now five) but did not update the pinning test. → Change `hasLength(4)` → `hasLength(5)`, add `bitboxDisconnected` to the asserted set, rename the test description. ### 2. `MockAppStore.ensureUnlocked` not stubbed (9 + 1 tests) #461 added `await appStore.ensureUnlocked()` to `RealUnitSellPaymentInfoService.confirmPayment` and the `SettingsSeedCubit._loadSeed()` path. Tests using `_MockAppStore`/`MockAppStore` without stubbing `ensureUnlocked` got `null` back from mocktail and threw `TypeError: type 'Null' is not a subtype of type 'Future<void>'`. Affected: - `test/packages/service/dfx/real_unit_sell_payment_info_service_validation_test.dart` (9 `confirmPayment validation guard` tests) - `test/screens/settings_seed/settings_seed_page_test.dart` (1 page test) → Add `when(() => appStore.ensureUnlocked()).thenAnswer((_) async {});` to each `setUp`. ### 3. Page tests don't register `DfxKycService` (4 tests) #461 added `DfxKycService` as a cubit dependency for `ensureSignatureFor`. The existing `create_wallet_page_test` and `restore_wallet_page_test` only register `WalletService` in `setUpAll`, so `BlocProvider(create:)` fails with `Bad state: GetIt: Object/factory with type DfxKycService is not registered` when the cubit is first read. → Add `MockDfxKycService` and register it in `setupDependencyInjection` of both files. ### 4. `settings_seed_cubit_test` race on slow CI (1 test) `toggleShowSeed flips showSeed and keeps seed unchanged` relies on `wait: const Duration(milliseconds: 10)` between `act` and `verify`. The async `_loadSeed()` chain (`ensureUnlocked` → seed emit) takes longer than 10 ms on CI, so the toggle races with the post-`_loadSeed` emit and `showSeed` ends up `false` instead of `true`. → Replace the static `wait:` with an explicit `await c.stream.firstWhere((s) => s.seed.isNotEmpty)` inside `act` so the test only toggles after `_loadSeed` has settled. Same fix on the sibling "toggleShowSeed twice" test. ### 5. `bitbox_credentials_test` queue test relabels error (1 test) `queue continues after a sign throws (slot released in finally)` expects the original `Exception('first sign explodes')` to surface. #461's new `_runOrThrowDisconnect` wrapper probes `manager.devices` on a thrown sign and relabels the error as `BitboxNotConnectedException` if the device list is empty. The mock did not stub `devices`, so it defaulted to `null` → `_deviceLost()` returned `true` → the original exception got relabelled. → Add a one-line stub `when(() => manager.devices).thenAnswer((_) async => [_FakeDevice()])` so the disconnect probe returns "still connected" and the original exception survives. ## Verification - Local repro of CI: `flutter test --coverage` matches the CI's failure profile (1342 / 17) on `develop`. - Post-fix expected on CI: 1359 / 0. - No production code touched. ## Why this needs to land fast `develop` CI is currently red, which blocks the auto-tag + release pipelines and propagates to any PR (e.g. #466) that runs against this branch. All seven changes are scoped to `test/` only. ## Test plan - [ ] `flutter analyze` — clean - [ ] `flutter test --coverage` — green (1359/0) - [ ] CI on this PR — green
TaprootFreak
force-pushed
the
fix/dfx-merge-registration-misroute
branch
from
146b707 to
a125de2
Compare
Contributor
🔍 Tiefer Verifikations-Review — Folge zum ersten Compliance/Tech-ReviewHabe den PR nach den Compliance-Fixes ein zweites Mal durchgegangen und die Behauptungen aus meinem ersten Review systematisch gegen den Code ( 🚨 Merge-Blocker: vergessener Rename in
|
| # | Punkt | Status |
|---|---|---|
| 1 | docs/testing.md:56 markBitboxConfirmed → markRegistrationSignProduced |
🚨 vor Merge fixen |
| 2 | Widget-Test für markRegistrationSignProduced()-Call in kyc_email_page |
empfohlen im selben PR |
| 3 | i18n-Text neutraler (siehe Vorschlag oben) | empfohlen im selben PR |
| 4 | Commit-Message-Wording zu Bug A (Sicherheit → UX/Daten-Integrität) | optional, kosmetisch |
| 5 | isClosed-Schutz in KycEmailVerificationCubit (Generation-Pattern wie KycCubit) |
Folge-Ticket |
| 6 | In-Flight-Guard / Mutex für checkEmailVerification |
Folge-Ticket |
| 7 | Echter Retry-Pfad für null-userData-Race | Folge-Ticket |
| 8 | Adress-Nummer-Validator (kyc_registration_address_step.dart:58-65) |
Folge-Ticket, out-of-scope |
| 9 | Idempotenz /register/wallet backend-side bei Wallet-bereits-registriert |
Folge-Ticket, out-of-scope |
Was solide bleibt
- Beide Kern-Fixes sind logisch korrekt und an der einzig richtigen Stelle platziert
- Rename ist im Dart-Code vollständig
- Geänderte Cubit-Tests assertieren echtes Verhalten statt es einzufrieren (der alte
verify state is Successwar der Smoking Gun für den Doppel-Emit-Bug — sehr saubere Aufarbeitung) _runGeneration-Guard aus fix(kyc): prevent stale state leak in KycCubit on retry #317 unberührt, Gate-Pattern aus fix: gate sensitive KYC steps behind BitBox EIP-712 sign #312 strukturell intakt_completeRegistrationist privat und hat nur einen Caller — Signatur-Change aufFuture<bool>sicher- Doc-Comments sind echte Re-Writes, nicht oberflächlich umbenannt
Empfehlung
Bug A und Bug B sind beide reale, ernstzunehmende Bugs (Bug B ist sogar gravierender — Wallet-nicht-registriert wurde als Erfolg verkauft). Der Fix ist substantiell und gut umgesetzt. Nach Fix des docs/testing.md-Renames mergebar. Test-Coverage für die neue Markierungs-Logik plus i18n-Anpassung wäre wünschenswert im selben PR, sonst sauberes Folge-Ticket.
Existing DFX customers who registered in realUnit with the same email as their DFX account were routed to the empty personal-data form after the merge had already succeeded server-side. The form's submit would have produced a fresh EIP-712 sign over user-typed data, attaching potentially-divergent values (diacritics, address abbreviations, phone format) to the existing DFX account — a data-integrity risk, plus a miserable onboarding UX on top. Root cause: `KycCubit` gates past the disclaimer step on `_bitboxConfirmed`, a flag tracking "EIP-712 registration sign produced this session". The new-registration path set the flag in `kyc_registration_page.dart` after a successful form submit. The merge path produces the same EIP-712 signature via `KycEmailVerificationCubit._completeRegistration` → `RealUnitRegistrationService.registerWallet`, but never marked the flag — so after merge confirm + disclaimer accept the gate fired and sent the user back to the form. Changes: - Rename `_bitboxConfirmed` → `_registrationSignProduced` and `markBitboxConfirmed()` → `markRegistrationSignProduced()` to match the wallet-agnostic semantics. The BitBox 13-step ceremony is one visible manifestation of the underlying EIP-712 sign; software wallets produce the same signature silently. The old name routinely misled readers. - Call `markRegistrationSignProduced()` from `kyc_email_page.dart` on a successful merge confirmation pop, before re-running `checkKyc`. - Update doc comments around the gate to focus on the per-session sign-gate semantics rather than the hardware-wallet UX surface. - Update `docs/testing.md` code snippet to use the new method name — the previous version would not compile if copied. - Update `kyc_cubit_test.dart` references and group name. - Add widget tests in `kyc_email_page_test.dart` that verify the setter is called when the merge-confirmation pop returns `true`, and not called on `false` / `null`. Without these a future refactor could remove the setter call again without any test failing. Closes RealUnitCH#464.
… retry path
`KycEmailVerificationCubit.checkEmailVerification` previously emitted
`KycEmailVerificationSuccess` unconditionally whenever the JWT
account-id changed, even if the subsequent `_completeRegistration`
call failed. Two failure modes were silently swallowed:
1. `getWalletStatus` returning `null` `realUnitUserDataDto`
(likely a propagation race between the auth service and the
user-data service after merge)
2. `registerWallet` throwing for any reason
Both emitted `RegistrationFailure` from inside the catch block, but
the outer Success emit immediately overwrote it. The snackbar shown
by the verification page flashed briefly and was then dismissed by
the `context.pop(true)` that Success triggers. The user perceived a
successful merge while the wallet was, in fact, not signed and
registered. Tests asserted this overwrite behaviour as if it were
intended; they were the smoking gun for the bug.
Changes:
- `_completeRegistration` now returns `Future<bool>`.
`checkEmailVerification` only emits `Success` when registration
genuinely succeeded — otherwise it leaves the cubit in
`RegistrationFailure` so the verification page stays open and the
user can retry.
- Add `_runGeneration` cancellation-token (mirrors `KycCubit`):
guards every emit against stale calls and against `isClosed`,
prevents a late HTTP response from emitting after a retry, and
lets a double-tap supersede the in-flight call instead of racing
two emit sequences.
- Add `_mergeDetected` short-circuit: once the JWT account-id change
has confirmed the merge, the auth side is settled. A retry after a
null-userData race should not re-run the account-id comparison
(which would emit `Failure` because `getAuthToken` keeps returning
the merged account). Subsequent retries skip the auth check and go
straight to `_completeRegistration`, where the propagation race
usually resolves.
- Rewrite the misleading `registerEmailVerificationRegistrationFailed`
i18n strings (DE + EN). The old text claimed the wallet had been
assigned to the account, which is only true on the `registerWallet`
throw path — for the null-userData branch nothing had been assigned.
New text is neutral and actionable for both failure modes.
- Update two existing tests that asserted the silent-Success
behaviour to assert the new, correct behaviour: failures surface
to the UI.
- Add a new test for the retry path: two consecutive calls with
a null-userData first response and a populated second response
end in Success, exercising `_mergeDetected`.
Refs RealUnitCH#465.
The `addressNumberCtrl` field in `KycRegistrationAddressStep` had no validator. An empty value would pass `_formKey.currentState?.validate()` and the registration submit would send a malformed address to the backend. Surfaced in the RealUnitCH#466 review as a follow-up to the merge-misroute fix because the registration form is now the only path an existing-DFX customer never reaches — but for new users it is the primary entry, so the gap matters. Mirrors the validator pattern used by street, postal code, city, and country fields in the same step.
Blume1977
force-pushed
the
fix/dfx-merge-registration-misroute
branch
from
a125de2 to
4d44c21
Compare
Contributor
Author
|
Update zum zweiten Review von heute morgen: Alle Items vollständig in diesen PR integriert. Branch wurde force-gepusht mit neuer Basis auf aktuellem develop (
Adressiert aus dem Review
Lokal verifiziert
Force-pushNotwendig wegen Rebase auf den aktuellen develop ( |
TaprootFreak
approved these changes
May 21, 2026
3 tasks
TaprootFreak
added a commit
to DFXswiss/api
that referenced
this pull request
May 21, 2026
) * fix(realunit): make register/wallet idempotent on signature match `completeRegistrationForWalletAddress` (and the same-wallet branch of `completeRegistration`) threw `BadRequestException("RealUnit registration already exists for this wallet")` whenever a KycStep already existed for the current wallet. That breaks legitimate client retries after a lost response: the first call succeeds server-side, the network drops the reply, the client re-sends with the same EIP-712 signature and gets a hard 400 — surfaced in the realunit-app KYC merge flow (RealUnitCH/app#466) as a user-stuck failure with no recovery path. Replace the hard throw with a signature-aware idempotency check: - Same wallet + same signature → return the existing registration's status (`COMPLETED` if the KycStep is completed, otherwise `FORWARDING_FAILED`). No new KycStep, no re-forward to Aktionariat. - Same wallet + different signature → keep the 400, but with the more precise message "RealUnit registration already exists for this wallet with a different signature". A fresh sign over the same wallet means the client produced a new payload — that is a real conflict, not a retry. Tests cover all three branches and assert that `createCustomKycStep` is never called on the idempotent paths. * refactor(realunit): destructure findRegistrationStep result and log idempotent retries - Use destructuring at both `findRegistrationStep` call sites so the register/complete and register/wallet paths read identically. - Pass `userData` explicitly into `idempotentRegistrationResult` rather than relying on TypeORM's back-population of `step.userData`, which is not guaranteed for steps loaded via the `userData.kycSteps` relation. - Emit an info log when the idempotent branch fires so production retry frequency is observable and stuck-user reports can be correlated to a kycStep id. - Document the three KycStep statuses REALUNIT_REGISTRATION actually reaches (INTERNAL_REVIEW / MANUAL_REVIEW / COMPLETED) so the binary `isCompleted` mapping does not silently break if the lifecycle is ever extended. * docs(realunit): broaden idempotent-status comment to cover admin overrides The previous comment claimed the step is in one of three states (INTERNAL_REVIEW / MANUAL_REVIEW / COMPLETED). That is true under the normal service flow, but the generic `kyc-admin.updateKycStep` endpoint can push REALUNIT_REGISTRATION into any non-failed/non-canceled status (ON_HOLD, OUTDATED, etc.) — and `findRegistrationStep` only filters FAILED and CANCELED. Runtime behavior is unchanged (anything that is not `isCompleted` maps to FORWARDING_FAILED, which is the safe default), but the comment now accurately reflects the reachable state set. * fix(realunit): match idempotency signatures case-insensitively EIP-712 signatures are 0x-prefixed hex strings. Lower-case and upper-case representations of the same signature are semantically identical, but the previous strict `!==` would have treated them as different and rejected legitimate retries that happen to flip hex case (can occur when a wallet library updates its hex-encoding default). Use `Util.equalsIgnoreCase` for the comparison, matching the convention already used for wallet-address comparisons in `findRegistrationStep`. Added a unit test that stores an upper-case signature and retries with a lower-case one — it now returns COMPLETED instead of throwing 400.
5 tasks
Blume1977
added a commit
to Blume1977/realunit-app
that referenced
this pull request
May 21, 2026
…dit alignment The first revision shipped the rule and the inventory but left the plan and audit unable to be cross-referenced: the plan mentioned V1–V19 IDs that the audit never assigned, and the summary tables miscounted. Several real violations were also missing from the audit, and the plan didn't close all audit items. Audit changes (`docs/api-authority-audit.md`) - Assign explicit V<N> anchors to every bulleted finding (V1–V40). The plan cites these. Composite items use V6a–d / V10a–c / V13b/c. - Add four previously-missed grep targets: `_changeStepNames` in `settings_user_data_cubit.dart:18-22` (V6d); the inReview interpretation in `settings_edit_address_cubit.dart:22` (V6c); `Currency.values` in `settings_currencies_page.dart:26` (V10c); `Language.values` in `settings_languages_page.dart:24` (V13c). - Add V11 (bank-account default selection in `sell_bank_account_field.dart`) and V34 (`softwareTermsAccepted` boot-time gate in `main.dart:120`), which the plan already references but the audit had not enumerated. - Tag the four boundary cases (V13b BitBox backup, V25 401-refresh, V28 network mode, V30 default assets, V33 BIP-39) as documented exceptions in-line so reviewers don't try to "fix" them. - Replace the Summary table with canonical counts derived from a recount of the file itself: 15 P0 + 10 P1 + 16 P2 = 41 distinct P0–P2 violations, 36 actionable after subtracting the 5 boundary cases. - Use `RealUnitCH#466` and `DFXswiss/api#3731` consistently in P4 instead of mixing `PR RealUnitCH#466` / `#3731` short forms. Plan changes (`docs/api-authority-plan.md`) - Rewrite the Executive summary table with per-wave V-ID lists. Totals add up: 11 + 6 + 7 + 4 + 8 = 36 actionable. - Wave 1 description: replace the inconsistent "8 violations" / "5 P0/P1 items" with "6 items closing 11 audit findings", matching W1.* heading count + each W1.x's Closes line. - W1.2: drop the false claim that `BankAccountDto` parsing needs adjusting — the app's DTO already exposes `isDefault` (verified in `lib/packages/service/dfx/models/bank_account/dto/bank_account_dto.dart`). Selector switch is now a one-line change. - W1.3, W1.4, W1.6: extend Closes lines so the V-IDs added to the audit are actually covered. W1.6 now explicitly references V34 (drops "tail item from Wallet/Services scan" wording). - W2.1/W2.2 Closes: add V5 (routing chain), V21 + V22 (session-gate positions). W3.1/W3.2 Closes: add V6c (edit-address) and V6d (`_changeStepNames`) + the new `canEditAddress` capability + `category` field on `KycStepDto`. - Add a new Wave 5 that closes V20 (server-side auto-register), V23 (JWT merge), V24 (transaction statusLabel), V26+V27 (polling orchestration), V29 (asset config endpoint), V31+V32 (account-bounds for date pickers). Wave 5 also documents which audit items remain explicitly out of scope (V13b, V25, V28, V30, V33) and why. - Redraw the Sequencing diagram so the wave-internal API→App arrow and the "waves are independent of each other" property are unambiguous (no more vertical connectors that imply sequential). CONTRIBUTING.md - Rename the rule-test heading to "### The test (Wer entscheidet?)" so the German phrase referenced from the PR body and the plan footer actually appears in the file.
TaprootFreak
added a commit
that referenced
this pull request
May 21, 2026
…cal sets (#494) ## Summary **Closes the 2026-05-21 ident-misroute report.** Wave 2.2 of the API-as-Decision-Authority audit ([`docs/api-authority-plan.md`](docs/api-authority-plan.md), foundation rule in #491). Companion to API PR [DFXswiss/api#3732](DFXswiss/api#3732) (`feat/kyc-decision-fields`). The cubit used to re-implement the backend's own routing rule locally: - `_requiredStepNames` set — duplicate of `requiredKycSteps(userData)` on the API - `_minLevelForActions = 30` threshold — duplicate of the level check - `actionableStatuses` / `pendingStatuses` sets — duplicate of how the backend tags step status - A 30-line iteration over `kycSteps` that derived `KycCompleted` / `KycPending` / `_continueKyc` from filter results - `_continueKyc` repeated the same manual filter over `kycSteps` after a realunit registration completed (parallel code path with the same anti-pattern) That setup is exactly what misroutes a high-level user when `checkDfxApproval` re-issues their Ident step on the backend (user_data 338759: kycLevel 53 + an Outdated Ident step + a sequence-1 Ident step in `InProgress` → the app sends them back to `KycIdentPage` even though they didn't ask to re-verify). After API PR #3732 the backend returns those decisions directly. **The cubit now renders them.** ## Changes ### Cubit logic - `KycLevelDto.processStatus` (new field, mirrors `KycProcessStatus` on the API) drives the top-level switch: - `Completed` → emit `KycCompleted` - `PendingReview` → emit `KycPending(<the required step in review>)` - `InProgress` → call `_continueKyc` (unchanged routing semantics, new implementation) - `Failed` → emit `KycFailure` - `KycStepDto.isRequired` (new field) selects which step to surface in the pending case. - `UserKycDto.canTrade` is parsed from `/v2/user` for downstream callers (Wave 3 will let buy/sell flows render it directly instead of guessing from `level >= 30`). - `_continueKyc` now reads `KycSessionDto.currentStep` directly instead of iterating over `kycSteps` to find `isCurrent`. A missing `currentStep` surfaces `KycUnsupportedStepFailure(null)` instead of leaking a bare `StateError` stack-trace into the user-facing i18n message. - `KycPageManager` drops the `requiredLevel` parameter and its router plumbing — the cubit no longer needs a threshold. `canTrade` / `processStatus` speak directly to the buy/sell question. The cubit body shrinks from ~95 lines of iteration logic to a five-arm switch on `processStatus`. The diff removes 6 of the 10 audit-flagged violations on this file in one go. ### What stays local `_legalDisclaimerAccepted` / `_registrationSignProduced` remain per-cubit-instance security gates. They do not encode business routing — they enforce a per-session ceremony on this device before any signed call. Position in the flow is unchanged from before. ### DTOs - `KycLevelDto` + `KycSessionDto` carry `processStatus`, default to `inProgress`. - `KycStepDto` carries `isRequired`, default `false`. - `UserKycDto` carries `canTrade`, default `false`. - New `KycProcessStatus` enum mirrors the API enum 1:1. ## Backwards compatibility All three new fields default to safe values when the API response omits them: - `processStatus = inProgress` → falls through to `_continueKyc`, identical to the legacy behaviour on the unhappy path - `isRequired = false` → `KycPending` falls back to `KycCompleted` (no required step found) - `canTrade = false` → downstream callers are conservative A pre-#3732 backend keeps the app functional; an app build that consumes the new fields and a backend that hasn't shipped them yet doesn't crash. ## Tests `kyc_cubit_test.dart` rewritten to drive the cubit via API fixtures (`processStatus` + `isRequired`) instead of the old level-based setup. The previous level-based + step-iteration cases collapse to five fixtures: - `processStatus: Completed` → emit `KycCompleted` - `processStatus: PendingReview` + required ident in `InReview` → emit `KycPending(ident)` - `processStatus: PendingReview` + required dfxApproval in `InReview` → emit `KycPending(dfxApproval)` - `processStatus: InProgress` → `_continueKyc` → emit `KycSuccess(currentStep)` - `processStatus: Failed` → emit `KycFailure` The 403/TFA_REQUIRED matrix, generation-token regression, and sign-gate sequencing are preserved. ## Verification - [x] `flutter analyze` — clean - [x] `flutter test` — **1435 / 1435 passing** ## Manual test plan (post-merge of #3732) - [ ] **Re-run the 2026-05-21 reproduction** on user_data 338759 (or a fresh test wallet on the same merged account): open the app → expect `Dashboard` if `canTrade: true`, or `KycPending(ident)` only if `processStatus: PendingReview` (no more spurious `KycIdentPage` from a re-issued Ident step). - [ ] **New customer flow** still works end-to-end: register email → disclaimer → registration → progresses through `_continueKyc` for each required step. - [ ] **Existing DFX customer merge flow** (PR #466 path): merge confirm → disclaimer → registration sign → lands on dashboard if the merged user is `canTrade: true`. ## Closes (from audit, P0) - V1 — `_requiredStepNames` set - V2 — `_minLevelForActions = 30` + `level < _requiredLevel` checks - V3 — `actionableStatuses` / `pendingStatuses` sets and the iteration over kycSteps - V5 — manual filter + routing chain over `kycSteps` (process-status-driven routing replaces the entire next-step selection algorithm) - V45 — `_continueKyc`'s parallel iteration over `kycSteps`; the cubit now reads `KycSessionDto.currentStep` directly - Routing dependency on iteration logic Tracked in [`docs/api-authority-audit.md`](docs/api-authority-audit.md). ## Follow-ups - The auto-email-registration branch when `level < 10` is still in the cubit; it's a Wave-3 candidate — the API could perform this server-side once the email is set. - `KycStep → KycStepName` map (`_mapStepName`) is still local; if the API ever exposes a `uiHint` field it can go too. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
4 tasks
TaprootFreak
pushed a commit
that referenced
this pull request
May 23, 2026
…stability (#525) ## Summary Implements the high-priority items from the test engineering audit (#506). **4 commits:** - `test: add 45 tests from codebase audit (#506)` — regression guards and bug-proving tests - `fix: add isClosed guards to 8 cubits to prevent emit-after-close` — fixes the 8 bugs those tests exposed - `fix: cancel subscription in FilterCubit + isClosed guards in BuyConverterCubit` — 2 more cubits - `test: malformed JSON response tests for remaining 5 DFX services` — completes Phase 3 ## Rebase note This branch is 24 commits behind develop. Rebase has merge conflicts in `buy_payment_info_cubit_test.dart` and `sell_payment_info_cubit_test.dart` because develop refactored those cubits (removed `DFXPriceService` dependency, moved min-amount validation to API). The conflicts are in test files only — the resolution is: take develop's test structure and re-add the 3 new tests per file (BitboxNotConnected, emit-after-close, negative amount/comma). ## Bug fixes (10 cubits) Added `if (isClosed) return;` after every `await` in async methods that call `emit()`. Without these guards, navigating away from a screen while an HTTP request is in-flight throws `StateError: Cannot emit new states after calling close`. | Cubit | Trigger | |---|---| | `SellConfirmCubit` | User swipes modal during payment confirmation | | `SellBankAccountsCubit` | User taps back before bank accounts load | | `SellPaymentInfoCubit` | User navigates away during payment info fetch | | `SellConverterCubit` | Debounce timer fires after screen disposal | | `BuyPaymentInfoCubit` | User taps back during payment info fetch | | `BuyConverterCubit` | Same debounce pattern as sell | | `TransactionHistoryMultiReceiptCubit` | User leaves during PDF generation | | `TransactionHistoryReceiptCubit` | User scrolls away during receipt download | | `SettingsTaxReportCubit` | User navigates away during tax report generation (5-30s) | | `TransactionHistoryFilterCubit` | Stream subscription leaked on close (now stored + cancelled) | ## Source change (non-behavioral) - `AppDatabase.forTesting(QueryExecutor)` — `@visibleForTesting` constructor for in-memory DB tests. Zero production impact. ## Tests added (53 total) | Category | Count | Purpose | |---|---|---| | Emit-after-close | 10 | Proves/guards isClosed bugs | | Crypto regression | 1 | Non-ASCII signing (#289) | | Equality regression | 3 | BuyPaymentInfo field equality (#207) | | BitboxNotConnectedException | 1 | Missing exception path in BuyPaymentInfoCubit | | DashboardBloc error survival | 3 | Unhandled exceptions in 3 event handlers | | Financial boundaries | 4 | Negative amounts, comma normalization, Infinity/NaN | | JSON parsing (malformed responses) | 13 | All 11 DFX services + balance + registration | | SecureStorage corruption | 2 | Missing colon separator, empty ciphertext | | DB schema/migration | 4 | Table creation, wallet insert, FK integrity | | Parse fixed edge cases | 5 | Empty, multi-dot, dot, negative, zero | | Settings/URI pins | 2 | hideAmounts session-only, PaymentURI encoding | | Wallet persistence failure | 2 | Repository exception during create/restore | | Filter subscription cleanup | 1 | Stream subscription cancelled on close | | BuyConverterCubit close | 1 | Debounce timer + isClosed | ## Remaining from #506 (not in this PR) ### Phase 4.1-4.3: Storage atomicity (separate PR needed) These are real bugs but change database behavior and need careful review: - **`deleteWallet` doesn't cascade** — only deletes `walletAccountInfos`, not `walletInfos` (encrypted seed persists). Already tracked as #498. - **`insertDfxTransaction` non-atomic** — two separate inserts without a Drift `transaction()` wrapper. If the DFX details insert fails, the Transaction row is committed as an orphan. - **`saveBalance`/`saveAsset` TOCTOU races** — check-then-act pattern without atomicity. Needs `INSERT OR REPLACE` or Drift upsert. ### Phase 2: Security design decisions (need discussion) - **EIP-712 `signRegistration()` domain missing `chainId`** — unlike `signDelegation()` which includes it. Registration signatures are theoretically replayable across chains. Needs backend coordination. - **WebView accepts any URI scheme** — `javascript:`, `data:`, `file://` pass through without validation. Currently unreachable (both call sites hardcode empty amount), but should be guarded. ### Phase 6: Integration tests (separate scope) 5 cross-layer flows that require full DI container setup: 1. PIN verify -> wallet load -> dashboard (regression for `f9b89ea`) 2. Create wallet -> background -> resume -> seed cleared (regression for #485, #489) 3. Buy flow: switch currency -> payment details update (regression for #207) 4. KYC flow: existing DFX customer merge (regression for #466) 5. BitBox disconnect mid-sign -> reconnect -> retry (regression for #461) ## Test plan - [x] All 53 new tests pass (emit-after-close tests pass after isClosed fix) - [x] All existing tests pass (10 pre-existing loading failures unchanged) - [x] `flutter analyze` clean on changed source files - [x] No behavioral change to production code (only isClosed guards + subscription cleanup) Closes #506
This was referenced May 23, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Fixes two related bugs in the KYC merge flow for existing DFX customers plus a missing validator in the registration form. Closes #464, refs #465.
KycCubit._registrationSignProduced(formerly_bitboxConfirmed) was never marked after a successful DFX-merge email confirmation, even though the merge path produces the same EIP-712 signature as the new-registration path. After accepting the disclaimer, the user was routed to an empty personal-data form for data already on file at DFX. Submit would have produced a fresh EIP-712 sign over user-typed data, attaching potentially-divergent values (diacritics, address abbreviations, phone format) to the existing DFX account.KycEmailVerificationCubit.checkEmailVerificationemittedSuccessunconditionally after_completeRegistration, silently swallowing both the null-userData case (likely a backend propagation race) andregisterWalletthrows. The user perceived the merge as successful while the wallet was not actually signed and registered.KycRegistrationAddressStep.addressNumberCtrlhad no validator at all; empty values would pass form validation and reach the backend. Mirrors the validator pattern used by the other fields in the same step.Commits
3d98c8d—fix(kyc): close existing-customer merge misroute, rename sign gate flage71f71e—fix(kyc-email-verification): surface failures, add generation guard + retry path4d44c21—fix(kyc): require street number in registration address formWhat changed
Bug A
_bitboxConfirmed→_registrationSignProduced,markBitboxConfirmed()→markRegistrationSignProduced(). The flag is wallet-agnostic — silent local sign for software wallets, 13-step ceremony for BitBox. The old name routinely misled readers.markRegistrationSignProduced()fromkyc_email_page.darton a successful merge confirmation pop, before re-runningcheckKyc. Without this, the existing-DFX-customer flow would still hit the gate after the disclaimer step.docs/testing.md:56— the code example referenced the renamed method (would not compile if copied).kyc_cubit_test.dartreferences and group name.kyc_email_page_test.dartthat verify the setter is called when the merge-confirmation pop returnstrueand is not called onfalse/null. Without these a future refactor could remove the setter call again without any test failing.Bug B
_completeRegistrationnow returnsFuture<bool>indicating whether the wallet was actually registered.checkEmailVerificationonly emitsSuccesswhen_completeRegistrationreturnedtrue; on failure the cubit stays inRegistrationFailureso the verification page stays open and the user can retry._runGenerationcancellation-token (mirrorsKycCubit): guards every emit against stale calls and againstisClosed, prevents a late HTTP response from emitting after a retry, and lets a fast double-tap supersede the in-flight call instead of racing two emit sequences._mergeDetectedshort-circuit so the retry path actually retries. Once the JWT account-id change has confirmed the merge, the auth side is settled — a retry after a null-userData race should not re-run the account-id comparison (which would emitFailurebecausegetAuthTokenkeeps returning the merged account). Subsequent retries skip the auth check and go straight to_completeRegistration, where the propagation race usually resolves.registerEmailVerificationRegistrationFailedi18n strings (DE + EN). Old text claimed the wallet had been assigned to the account — only true on theregisterWalletthrow path, not on the null-userData branch. New text is neutral and actionable for both failure modes.Address-number validator
addressNumberCtrlinkyc_registration_address_step.dart, matching the existing validators on street, postal code, city, country.What did not change
/kycentry still constructs a freshKycCubitand the gate still requires a fresh sign before sensitive steps. The fix encodes a fact the merge path was already producing, it does not loosen the gate.KycStep.registration(genuinely new users without DFX data) the flow is identical.kyc_email_verification_page.dartalready handled all three states (Failure,RegistrationFailure,Success) correctly — Failure shows a snackbar and stays open, Success pops withtrue. No changes needed.Local verification
flutter analyze— clean (No issues found! (ran in 1.6s))flutter test— 1398 / 1398 passed (1395 develop baseline + 3 new tests)Test plan
registerWalletfailure (network kill, backend 5xx) → user sees the snackbar and the verification page stays open → user can retryItems deliberately deferred to a separate PR
Surfaced during review of #466 but kept out-of-scope to keep this PR coherent:
3d98c8d. Bug B's mechanism is the more severe of the two (wallet not registered presented as success); Bug A is misroute + data-integrity risk via form submit./register/walletwhen the wallet is already registered — out of scope for the app repo.Related
fix: gate sensitive KYC steps behind BitBox EIP-712 sign) — the merge case appears to have been overlooked at that timefix(kyc): prevent stale state leak in KycCubit on retry) — extends the same_runGenerationpattern toKycEmailVerificationCubit