Skip to content

fix(kyc): collect countryAndTINs for non-Swiss residents + tighten phone validator#604

Open
joshuakrueger-dfx wants to merge 7 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-non-swiss-registration-and-phone-validation
Open

fix(kyc): collect countryAndTINs for non-Swiss residents + tighten phone validator#604
joshuakrueger-dfx wants to merge 7 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-non-swiss-registration-and-phone-validation

Conversation

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator

Summary

Closes two registration funnel defects surfaced in the previous BitBox test session.

1. Non-Swiss registration was always rejected with 400 countryAndTINs is required when swissTaxResidence is false.

  • Registration model now carries an optional countryAndTINs list (forwarded to the existing RealUnitRegistrationRequestDto.countryAndTINs; not added to the EIP-712 envelope).
  • RealUnitRegistrationService._completeRegistration propagates the field.
  • KycRegistrationAddressStep adds a Swiss-tax-residence toggle. When off, a tax-residence country dropdown and a required TIN text field are revealed; the page collects them into a CountryAndTin entry and passes it through the cubit on submit.

2. PhoneNumberField accepted implausible national numbers (only required + digits + length >= 6) and surfaced the failure as a backend 400.

  • Replaced the generic minimum-length check with prefix-specific plausibility (+41 = 9 digits, +49 = 10-11 digits). Unknown prefixes are rejected by default rather than silently allowed.

Test plan

  • flutter analyze — no issues
  • flutter test test/packages/service/dfx/real_unit_registration_service_happy_test.dart — service-boundary regression covers non-Swiss case
  • flutter test test/widgets/form/phone_number_field_test.dart — 6 widget tests (required, digits-only, implausible +41, valid +41, implausible +49, valid +49)
  • flutter test test/screens/kyc/steps/registration/steps/kyc_registration_address_step_test.dart — 4 widget tests pin the toggle behavior and TIN validator
  • Bounded baseline slice (57 affected tests) — all green, no regressions
  • Manual device test: BitBox-paired iPhone, non-Swiss tax residence path through registration -> DFX backend acceptance

Notes

  • l10n: 5 new ARB keys (DE + EN) — swissTaxResidence, taxResidenceCountry, taxIdentificationNumber, tinHint, tinRequired.
  • Existing Swiss-only path is unchanged (toggle defaults to true).
  • Phone validator coverage is intentionally bounded (+41 and +49 only — the prefixes the field already supports). Adding more prefixes here would silently expand scope; do it explicitly when those prefixes are added.

… validator

- service: non-Swiss registration must POST countryAndTINs (currently fails at compile because Registration lacks the field)
- widget: PhoneNumberField must reject implausible national numbers for +41/+49 (currently 2/6 cases fail because the validator only checks "digits, >=6")
- KycRegistrationAddressStep: Swiss-tax-residence toggle (Switch).
  When off, reveals a tax-residence country dropdown and a required
  TIN text field. Hidden by default so the Swiss-only path stays unchanged.
- KycRegistrationPage: owns swissTaxResidenceCtrl + taxCountryCtrl + tinCtrl;
  on submit builds a CountryAndTin entry when the toggle is off, passes
  countryAndTINs into the cubit.
- KycRegistrationSubmitCubit.submit: accepts optional countryAndTINs and
  threads it into Registration so it reaches the register-complete DTO.
- Strings: swissTaxResidence, taxResidenceCountry, taxIdentificationNumber,
  tinHint, tinRequired (DE + EN), regenerated i18n.dart.
- New widget tests pin the conditional rendering, the required-TIN
  validator path, and the controller round-trip. Existing golden test
  updated for the new required params.
@TaprootFreak TaprootFreak added the tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR label May 28, 2026
@TaprootFreak
Copy link
Copy Markdown
Contributor

Conflicts! ⚠️

@TaprootFreak
Copy link
Copy Markdown
Contributor

Re API as Decision Authority (CONTRIBUTING.md:23–67):

1. swissTaxResidence toggle + countryAndTINs collection
Replaces the previous hardcoded swissTaxResidence: true (a clear violation: the app was deciding the user's tax residence) with a user-toggled value forwarded through an existing API field. App collects, API decides whether the value is valid. Correct layer.

2. PhoneNumberField prefix-specific length validator ⚠️ borderline
_isPlausibleNationalNumber hardcodes the per-country rule (+41 = 9 digits, +49 = 10–11 digits, all other prefixes silently rejected). Strict reading: this is "Min/max … hardcoded — must come from the API" applied to phone formats — when DE/AT/CH product expansion adds a new prefix the app rejects all numbers from those countries until updated. Lenient reading: it's "UI input validation (format, required field, length)" on the OK list, and you flagged the bounded-scope risk in the PR body yourself.

If the API doesn't already return phone-format info per supported country (e.g. on /v1/country), worth opening an issue in DFXswiss/api so the source of truth for "which prefixes does our user base register from" lives on the backend; the app then iterates the API's list. Not blocking, but worth aware as a known shortcut.

Fix itself looks correct. The non-Swiss path closing the swissTaxResidence-hardcoded violation is the more important win here.

@TaprootFreak TaprootFreak changed the base branch from develop to staging June 1, 2026 14:35
@TaprootFreak TaprootFreak marked this pull request as draft June 1, 2026 23:06
@TaprootFreak
Copy link
Copy Markdown
Contributor

Converted to draft — needs a rebase on the current staging head before this can land. Two reasons:

1. KYC merge conflicts on lib/screens/kyc/steps/registration/

A rebase attempt failed on commit b6fbc46 feat(kyc): collect tax-residence country and TIN for non-Swiss residents — content conflicts in:

  • lib/screens/kyc/steps/registration/kyc_registration_page.dart
  • lib/screens/kyc/steps/registration/steps/kyc_registration_address_step.dart

Both files were touched on staging after this branch was last synced, mainly by:

The resolution requires judgement calls about interaction (does Swiss-char-validation apply when tax-residence toggle is on? how does the wallet-status prefill interact with the tax-residence default?), so it's not mechanical — leaving it for you rather than guessing.

2. Tier 3 cache-key fix needs to be on the branch

After the org transfer to RealUnitCH/app, the Tier 3 Build iOS simulator app step started failing with mtime changed (... was built: ...) after every macos-latest runner image rotation. Fix landed on staging/develop/main as #631 (c08bca9 fix(ci): pin Tier-3 iOS cache key to Xcode version) — pins the actions/cache key to xcodebuild -version so cached .pcm modules can't be restored across an Xcode bump.

GitHub Actions reads the workflow file from the PR head, so this PR's own Tier-3 run won't pick up the fix until the rebase brings c08bca9 into the branch. The tier3:full label is set, so once rebased, the Maestro run will use the fixed cache key.

No urgency — when you next pick this PR up, a fresh rebase on staging will catch both points.

… rule

The phone validator's _isPlausibleNationalNumber returned false in the
default branch, so any prefix without a dedicated case (e.g. one added to
[prefixes] later, or a future API-driven list) would silently reject every
number from that country as too short until the switch was updated. Enforce
the length rule only for prefixes we have an explicit format for and fail
open otherwise; +41/+49 behaviour is unchanged. Add a regression test for an
unruled prefix.
@joshuakrueger-dfx joshuakrueger-dfx removed the tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR label Jun 2, 2026
@joshuakrueger-dfx joshuakrueger-dfx marked this pull request as ready for review June 2, 2026 07:54
@TaprootFreak
Copy link
Copy Markdown
Contributor

Note: rebase onto staging skipped — content conflicts in lib/screens/kyc/steps/registration/kyc_registration_page.dart and lib/screens/kyc/steps/registration/steps/kyc_registration_address_step.dart that overlap with recent KYC registration changes on staging. Author should rebase locally to resolve — automated resolution would risk picking the wrong side.

PortfolioChartCubit defaults to the threeMonths period, which filters price
points relative to DateTime.now(). The 'flat-value series still spreads lines
via the 5% floor' test pinned its points to fixed 2026-Q1 dates without
selecting a period, so once wall-clock time moved more than three months past
them the default window filtered every point out, the series became empty, and
horizontalLineValues collapsed to [] — a deterministic, calendar-triggered
failure that began on 2026-06-02 and reddens every branch's Analyze & Test.

Select TimePeriod.all (the all-time window, minX = first price time, no
now-dependency) so the fixed-date points are always visible, mirroring the
sibling flat-series tests. The 5%-floor behaviour under test is period-
independent, so the assertions are unchanged.
@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

Stacked the fix from #639 onto this branch (cherry-picked eea7c7b, now 0ecf12b) so Analyze & Test goes green without waiting for #639 to merge first. #604's Analyze & Test was red only because of a repo-wide time-bomb test (portfolio_chart_cubit_test flat-value/5%-floor case) that started failing on every branch on 2026-06-02 — fixed in #639 by selecting TimePeriod.all. Whichever of #604/#639 merges first, the other's rebase on staging drops the duplicated commit cleanly.

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