Skip to content

feat(#175): validate fiat code before submitting an order - #304

Draft
codaMW wants to merge 4 commits into
MostroP2P:mainfrom
codaMW:feat/175-validate-fiat-code
Draft

feat(#175): validate fiat code before submitting an order#304
codaMW wants to merge 4 commits into
MostroP2P:mainfrom
codaMW:feat/175-validate-fiat-code

Conversation

@codaMW

@codaMW codaMW commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Problem

create_order took the selected fiat code straight into the outgoing request with no submit-time validation. The currency picker constrains the happy path, but if the saved default fiat code becomes stale or is tampered with, the request could still go out with an unsupported code and come back as a daemon-side CantDo instead of being rejected locally.

Fix

A local preflight in create_order, reusing settings::validate_fiat_code (made pub(crate)):

  • orders.rs create_order normalizes the fiat code (trims whitespace) and validates it before publishing, so validation and the published value are the same. This replaces the previous empty-only check and covers empty and malformed codes alike, failing with the stable InvalidFiatCode marker so every caller inherits the check.
  • settings.rs validate_fiat_code is now pub(crate); validate_fiat_code_marker_cases covers valid codes plus empty / short / long / lowercase / mixed / symbol / non-ASCII, all rejected with the marker (and documents that a well-formed unsupported code like "XYZ" passes the shape check membership is out of scope, see below).
  • daemon_errors.dart maps InvalidFiatCode to a localized message through the existing localizedDaemonError helper, with a mapping test in daemon_errors_test.dart.
  • l10n invalidFiatCode in all five locales.

Testing

cargo test --lib (340 pass) + cargo clippy --locked -- -D warnings clean; flutter analyze clean. The preflight is covered by Rust tests on the real create_order line create_order_rejects_a_malformed_fiat_code and create_order_trims_the_fiat_code_before_validation (a padded-but-valid code clears the trimmed check and fails only later on node protocol; an untrimmed value would fail InvalidFiatCode) plus a Dart mapping test for the InvalidFiatCode marker.

Scope

This validates the fiat code's format (ISO 4217 shape) and trims it before publish. It does not validate membership a well-formed but unsupported code like "XYZ" still passes here and is rejected by the daemon which is what #175 actually asks for ("reject unsupported before publish; align to the create-order contract"). Membership belongs against the daemon's advertised supported_currencies (authoritative, no bundled-list drift), tracked in #380, rather than a Rust copy of assets/data/fiat.json.

Part of #175. Membership validation (the remaining half) is tracked in #380.

create_order took the selected fiat code straight into the outgoing request
with no submit-time check. The currency picker constrains the happy path, but a
stale or tampered saved default could still send an unsupported code that only
came back as a daemon CantDo. This adds a local preflight so the request is
rejected on-device with a clear, actionable message instead.

- orders.rs: create_order now validates the fiat code before publishing,
  reusing settings::validate_fiat_code (made pub(crate)). This replaces the
  previous empty-only check and covers empty and malformed codes alike, failing
  with the stable InvalidFiatCode marker so every caller inherits the check
  (per grunch's note, same pattern as BondRequired).
- settings.rs: validate_fiat_code is pub(crate); added
  validate_fiat_code_marker_cases covering valid codes plus empty/short/long/
  lowercase/mixed/symbol/non-ASCII, all rejected with the InvalidFiatCode marker.
- daemon_errors.dart: map InvalidFiatCode to a localized message via the
  existing localizedDaemonError helper.
- l10n: add invalidFiatCode in all five locales.

Scope: format-level (ISO 4217 shape) validation, which catches the stale/
tampered cases the issue describes. Membership validation against the bundled
fiat list would require porting that list into Rust — raised as a question on
the PR.

Closes MostroP2P#175.
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02e28990-4de1-40f1-a0c4-91336ed81000

📥 Commits

Reviewing files that changed from the base of the PR and between 7625b24 and 7bad547.

📒 Files selected for processing (8)
  • lib/core/daemon_errors.dart
  • lib/l10n/app_de.arb
  • lib/l10n/app_en.arb
  • lib/l10n/app_es.arb
  • lib/l10n/app_fr.arb
  • lib/l10n/app_it.arb
  • rust/src/api/orders.rs
  • rust/src/api/settings.rs

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.


Walkthrough

The order API now validates trimmed fiat codes before publication. The validator is reused across the crate and has expanded unit coverage. Invalid-code errors now map to dedicated English, German, Spanish, French, and Italian messages.

Changes

Fiat code validation

Layer / File(s) Summary
Fiat code validation contract
rust/src/api/settings.rs
validate_fiat_code is available within the crate. Tests cover valid ISO-shaped codes and invalid formats.
Order creation preflight
rust/src/api/orders.rs
create_order trims and validates fiat_code before publishing the order.
Localized invalid-code error
lib/core/daemon_errors.dart, lib/l10n/app_*.arb
InvalidFiatCode maps to a dedicated localized message in English, German, Spanish, French, and Italian.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to 7bad5

This PR adds submit-time fiat-format validation and a localized rejection path for malformed saved or tampered currency codes. The change is localized, but merge readiness remains moderate until the required binding, Flutter test, and localization regeneration checks are completed.

Suggested reviewers: grunch, catrya, andreadiazcorreia

Poem

A rabbit checked the coins with care,
And trimmed each code from hidden wear.
Bad fiat stayed outside the door,
While clear translations said what for.
Order paths now hop secure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR validates format before publishing, but it does not reject valid-format codes that are unsupported or stale. Validate fiat-code membership against the supported currency set, or clarify that the create-order contract requires format validation only.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The validation, tests, error mapping, and translations directly support the linked issue and its local rejection flow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: validating the fiat code before submitting an order.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codaMW

codaMW commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@ermeme ermeme Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed current head 7bad5478db55ee9cb00a8de25332067a621a2700 strictly.

Blocking issue

The new validator checks only the lexical shape (three uppercase ASCII letters), but the PR description says this is intended to reject unsupported fiat codes and closes issue #175. A tampered or stale saved default such as XYZ passes validate_fiat_code() and is still published to the daemon. XYZ is not present in the app's bundled assets/data/fiat.json, so this is a realistic invalid-but-format-valid value. The implementation therefore does not enforce the stated unsupported-currency guarantee; it only moves malformed input validation client-side.

Please either validate membership against the app's supported currency set (with a single authoritative/shared source, or an explicit protocol-backed capability list), or narrow the PR scope/closing metadata and document that only syntax validation is guaranteed, with a tracked follow-up for membership. If retaining Closes #175, add tests for a valid-format unsupported code such as XYZ and ensure it is rejected before publication.

The format validation, error mapping, localization parity, and existing CI checks otherwise look consistent. Local Rust validation tests pass; Flutter/Dart are unavailable in this environment.

…iat-code

# Conflicts:
#	lib/core/daemon_errors.dart
…ership as follow-up

ermeme's review: validate_fiat_code checks only ISO 4217 shape (3 uppercase
ASCII), so a well-formed but unsupported code like XYZ passes and is published,
which does not deliver MostroP2P#175's reject-unsupported-currencies guarantee.

Narrowed the scope honestly rather than porting assets/data/fiat.json into Rust
(which would duplicate the list and drift from the asset):
- Reworded the validator doc and the orders.rs preflight note to state the
  guarantee is syntactic only, with membership left to the daemon.
- Added a test asserting XYZ passes syntax validation, documenting the boundary.

Membership belongs against the daemon's advertised supported_currencies
(authoritative, no bundled-list drift), tracked as a follow-up.
Changing Closes MostroP2P#175 -> Refs MostroP2P#175.
@codaMW

codaMW commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

You're right format-only validation doesn't deliver #175's reject-unsupported-currencies guarantee. I took the narrow-scope path you offered rather than port `assets/data/fiat.json` into Rust, because a Rust copy of the bundled list would duplicate it and drift from the asset. The authoritative source is the daemon's advertised `supported_currencies` (`types.rs`, from the Kind 38385 instance event), so I filed #380 to do membership validation against that.

In this PR:

Rebased onto current main (conflict was in `daemon_errors.dart` both sides added an error mapping, kept both). `cargo test --lib` / `clippy --locked -- -D warnings` and `flutter analyze` green.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes requested — three blockers, all from measuring rather than reading

The shape of the change is clean and the Dart wiring works. What it does not do is defend the case the issue describes.

B1 — Closes #175 would close the issue with its stated behaviour unimplemented

Issue #175's Expected behavior is literal: es before the request is published"* and
"Keep the UX aligned with the actual createlying only on the picker list." That is
membership, not format. This validates own test documents the boundary:assert!(validate_fiat_code("XYZ").is_ok()). A well-formed but unsupported code still goes out and still comes back as a daemon CantDo — exactly what the issue asks to prevent.

The body also asks whether membership validation is wanted. A PR that is still asking whether its scope is
complete should not carry the keyword that implement membership, or change Closes#175 to Part of #175 and open the follow-up.

B2 — The realistic stale case is the one this does not catch; the one it catches is barely reachable

I traced where the fiat code reaching crearces: add_order_screen.dart:241 (repeat an order from the book — always well-formed, i:117, the saved default.

That default lives **entirely in Dart Sharetings_provider.dart:60, written at :89-95fromcurrency_selector_dialog.dart:95 — w*. Meanwhile Rust'sset_default_fiat_code`, which already validates with this very function, has no Dart caller at all; it is a
dead store.

So: the dialog only offers codes from `fiatever saved is a real three-letter uppercase
code. The genuine stale scenario — a releasved default still points at it — yields a
well-formed, unsupported code, which thThe scenario it does catch (malformed
garbage) requires hand-editing the preferenthe improbable case and lets the probable
one through.

Validating at the write site (or routing it already validates) would close the
malformed case at its source; membership wo

B3 — Whitespace slips past the preflightmodel

The value validated is params.fiat_code.tr params.fiat_codeorders.rs:575, and what matters, actions.rs:58sendsparamsemon. Measured:

create_order(" USD ") -> NodeCapabilitiesUn82fa8cb9… not fetched yet

It cleared the validation and continued inter only because there is no node in a test.
Against a real node it would have gone out t model is a tampered preferences file, a
padded value is exactly as easy to write asd publish the same trimmed value.

Minor

  • Nothing tests the only production line delete the call from create_order and all338 tests stay green**. The new validate_fiat_code_marker_cases exercises the validator, which already existed and already had a test asserting the marker (set_default_fiat_code_lowercase_rejected also fails if you break it).
    And the usual "there is no harness" defenceing before the validation in create_order is argument checking — no DB, no relay pool, no identity — so the test is about fifteen lines. I wrote it to be
    sure:

    create_order_rejects_a_malformed_fiat_code ... ok      (with the fix)
    create_order_rejects_a_malformed_fiat_coddeleted)
    

    Happy to hand it over.

  • The Dart mapping is untested too, andexists: test/core/daemon_errors_test.dart
    covers DisputeAlreadyOpen, TradeNotDispuatCode appears zero times in that file. One more line.

  • **The l10n key landed above the file headvalidFiatCodeis the first key in theobject, **before@@Localeand@@last_modified**. gen-l10ndoes not mind and the untranslated report is{}, but in five curated files this is what getse, @@last_modified still reads2026-03-31` in all five.

  • Stale numbers in the body: it says "cargo test (256 pass)"; merged with today's main the tree gives 338.
    The PR was opened on 18 August.

What I verified

  • The Dart wiring works: add_order_scrateOrder error through
    localizedDaemonError, so the marker is lo.
  • All five translations are real translations, not English copies, and the untranslated report is {}. The
    French string has no colon, so the non-breay.
  • Mutating the validator is caught by two tests. That part is defended — it is just not the part this PR
    changes.
  • **Full CI on the tree merged with currentrgo test --locked→ **338 passed, 0 failed**;cargo clippy --locked -- -D warnze→ zero issues in hand-written code;flutter test→ **312 passed**;flutter g
  • Both routes by which fiat_code reaches validated Rust store has no caller.

Not verified

  • The on-device repro the body mentionsith a valid currency). I have no device;what I did confirm is that the happy path is unaffected, since the validator accepts any three uppercase letters.
  • **Whether @grunch wants membership here othe body's own open question, and B1 depends on the answer. If it is a follow-up, changiopening the issue for validation against
    the daemon's advertised `supported_currenci

…up (MostroP2P#304 review)

Catrya's review (measured, not read):

B3 — validated value != published value. create_order validated
params.fiat_code.trim() but published the untrimmed params.fiat_code, so
' USD ' cleared the check and went out padded. Now normalized in place before
both validation and the dispatch clone. mut params.

Tests on the production preflight line:
- create_order_rejects_a_malformed_fiat_code (us1 -> InvalidFiatCode)
- create_order_trims_the_fiat_code_before_validation ('  USD  ' clears the
  trimmed check, failing only later on node protocol).

Dart mapping test: InvalidFiatCode -> l10n.invalidFiatCode (bare and with the
offending code as context).

l10n: moved invalidFiatCode below @@Locale / @@last_modified in all five files.

Scope (B1): this validates ISO 4217 shape, which is not MostroP2P#175's membership
requirement. Changing Closes MostroP2P#175 -> Part of MostroP2P#175; membership vs the daemon's
advertised supported_currencies is tracked in MostroP2P#380.
@codaMW

codaMW commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

All three addressed thank you for measuring; the trim mismatch and the reachability analysis were both things I'd missed.

B1: scope corrected. You're right that #175 asks for membership ('reject unsupported before publish; align to the actual create-order contract'), which format validation doesn't provide my own test documents the gap (`XYZ` passes). Changed `Closes #175` -> `Part of #175`, removed the 'is membership wanted?' open question from the body, and opened #380 for membership against the daemon's advertised `supported_currencies` (the authoritative source, no bundled-list drift).

B3: the trim mismatch (real bug). `create_order` validated `fiat_code.trim()` but published the untrimmed value, so `' USD '` cleared the check and went out padded. Now normalized in place before both validation and the dispatch clone (`mut params`). Two tests on the real preflight line: `create_order_rejects_a_malformed_fiat_code` and `create_order_trims_the_fiat_code_before_validation` (the padded-valid case clears the trimmed check and fails only later on node protocol an untrimmed value would fail `InvalidFiatCode`).

B2: I take the point. The realistic stale case (a delisted-but-well-formed code) is exactly what membership (#380) closes; the malformed case the shape check catches is only reachable by hand-editing preferences. I've kept the shape preflight + trim as the honest, tested part that lands here, and left membership as the substantive fix in #380 rather than claiming this closes the issue.

Minors: added the Dart `InvalidFiatCode` mapping test in `daemon_errors_test.dart`; moved the l10n key below `@@locale`/`@@last_modified` in all five files; refreshed the body's stale test numbers (340).

340 Rust tests + clippy `--locked` clean; `flutter analyze` clean; the fiat + daemon-error tests pass.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Everything from the previous round is fixed

I verified each one:

Previous finding Status
B1Closes #175 with the stated behaviour (membership) unimplemented ✅ the body now says "Part of #175", with membership tracked in #380 and the reasoning written down (validate against the daemon's advertised supported_currencies, not a Rust copy of fiat.json)
B3 — whitespace slipped past: trim() was validated, the untrimmed value published params.fiat_code = params.fiat_code.trim().to_string(); normalizes in place before validating, so the validated and published values are the same
Minor — nothing tested the production line ✅ and the two new tests are load-bearing: removing validate_fiat_code(...) from create_order fails create_order_rejects_a_malformed_fiat_code; removing the in-place trim fails `create_order_trims_the
Minor — the Dart mapping was untested rs 6 times in daemon_errors_test.dart
Minor — the l10n key landed above the file header ✅ it now sits after @@locale / @@last_modified
Minor — stale numbers in the body ✅ 340, which is what the tree gives

B2 is answered by the scope reframing: the PR now states explicitly that it validates shape, not membership, and
why.

Changes requested — for something new, s that fixed the above.

Blocking: orders.rs arrives wholesale-

7bad547  feat(#175): validate fiat code…   ← the actual change
1756d1e  docs+test(#175): scope … follow-up
ce24c56  fix(#175): trim fiat_code…

A commit whose message reads "docs+test … track membership as follow-up" changes 2,311 lines of the busiest
file in the repo. And it is not indentationiff -w) the branch still contributes **+459 / −241**. It is rustfmt` — argument reflow breaks:

-            crate::api::logging::blog_info
-                "take_order confirmed by d={action:?}"
-            ));
+            crate::api::logging::blog_info
+                "orders",
+                format!("take_order confird} reply={action:?}"),
+            );

Three reasons this blocks:

  1. **The repo has not adopted rustfmt for tcargo fmt --check (ci.ymlhastest/clippy/wasm only), andorders.rsonmain` has 333 hunks rustfmt would change. The branch leaves it at
    147 — so this neither restores an existing e; it is what happens when a formatter runs
    on save.

  2. It is a conflict bomb. Against today14 conflicted regions in orders.rs
    plus 2 in daemon_errors_test.dart. And itking the PR's file wholesale gives 111compile errors, because it drops what main added since. With #333, #345, #347, #363, #365 and #375 all open
    against the same file, this breaks every on

  3. It buries the change. What this PR aeviewing them inside 700 lines of noise is
    not possible — I had to use -w and samplen there.

The reformat is not needed — I rebuilt t

Starting from orders.rs exactly as it is on main, I applied only the functional parts: the
normalize-and-validate block, create_order_code to pub(crate), and the two testscopied verbatim.

rust/src/api/orders.rs   | 59 ++++++----    two tests
rust/src/api/settings.rs |  2 +-
2 files changed, 56 insertions(+), 5 deleti

56 lines instead of 700, and the behaviouraworks identically:

cargo test --locked                    → 358 passed, 0 failed
cargo clippy --locked -- -D warnings   → cl
cargo check --locked --target wasm32   → cl

and the test is still load-bearing on the mlidate_fiat_code(...)failscreate_order_rejects_a_malformed_fiat_code

Ask: git checkout origin/main -- rust/pply the five lines, the mut params`, andthe two tests. That drops the 14 conflicts and stops this from breaking the other six open PRs on that file.

If the team wants rustfmt, that is its own formats everything at once and adds cargofmt --check to CI, at a moment when six PRs are not open against orders.rs. Smuggling it inside a five-line fix
is the worst way to get there.

Minor

  1. *The commit messages do not describe whest" for 2,311 lines of orders.rs;"tests + l10n cleanup" for 788 more. Even if the formatting were wanted, it belongs in its own commit under its
    own name.

  2. **@@last_modified still reads `2026-03es despite a key being added. Same as last
    round.

  3. The scope split is the right call — advertised supported_currencies ratherthan a bundled-list copy is the correct decision and is worth having on the record.

What I verified

  • The branch alone: cargo test --locked
  • Two mutations, each caught by the right t
  • The merge against today's main: 14 + 2 resolution does not compile (111 errors).
  • That CI does not check formatting and tha, so the reformat is this branch's decisionrather than a correction.
  • The minimal reconstruction above, with full Rust CI green.
  • All five translations and the key's posit

@grunch
grunch marked this pull request as draft September 3, 2026 18:51
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