feat(#175): validate fiat code before submitting an order - #304
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe 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. ChangesFiat code validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to 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: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
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.
|
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
left a comment
There was a problem hiding this comment.
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_code—orders.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_orderand all338 tests stay green**. The newvalidate_fiat_code_marker_casesexercises the validator, which already existed and already had a test asserting the marker (set_default_fiat_code_lowercase_rejectedalso fails if you break it).
And the usual "there is no harness" defenceing before the validation increate_orderis 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
coversDisputeAlreadyOpen,TradeNotDispuatCodeappears zero times in that file. One more line. -
**The l10n key landed above the file headvalidFiatCode
is 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_modifiedstill reads2026-03-31` in all five. -
Stale numbers in the body: it says "cargo test (256 pass)"; merged with today's
mainthe tree gives 338.
The PR was opened on 18 August.
What I verified
- The Dart wiring works:
add_order_scrateOrdererror 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_codereaches 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.
|
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
left a comment
There was a problem hiding this comment.
Everything from the previous round is fixed
I verified each one:
| Previous finding | Status |
|---|---|
B1 — Closes #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:
-
**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. -
It is a conflict bomb. Against today14 conflicted regions in
orders.rs
plus 2 indaemon_errors_test.dart. And itking the PR's file wholesale gives 111compile errors, because it drops whatmainadded since. With #333, #345, #347, #363, #365 and #375 all open
against the same file, this breaks every on -
It buries the change. What this PR aeviewing them inside 700 lines of noise is
not possible — I had to use-wand 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
-
*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. -
**
@@last_modifiedstill reads `2026-03es despite a key being added. Same as last
round. -
The scope split is the right call — advertised
supported_currenciesratherthan 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
Problem
create_ordertook 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-sideCantDoinstead of being rejected locally.Fix
A local preflight in
create_order, reusingsettings::validate_fiat_code(madepub(crate)):orders.rscreate_ordernormalizes 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 stableInvalidFiatCodemarker so every caller inherits the check.settings.rsvalidate_fiat_codeis nowpub(crate);validate_fiat_code_marker_casescovers 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.dartmapsInvalidFiatCodeto a localized message through the existinglocalizedDaemonErrorhelper, with a mapping test indaemon_errors_test.dart.invalidFiatCodein all five locales.Testing
cargo test --lib(340 pass) +cargo clippy --locked -- -D warningsclean;flutter analyzeclean. The preflight is covered by Rust tests on the realcreate_orderlinecreate_order_rejects_a_malformed_fiat_codeandcreate_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 failInvalidFiatCode) plus a Dart mapping test for theInvalidFiatCodemarker.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 advertisedsupported_currencies(authoritative, no bundled-list drift), tracked in #380, rather than a Rust copy ofassets/data/fiat.json.Part of #175. Membership validation (the remaining half) is tracked in #380.