Reject malformed, negative, and overflowing amounts in xlm_to_stroops

[arandomogg]

Summary

Audited StellarService::xlm_to_stroops for the precision and overflow edge cases raised in the issue. All three are real, and two more turned up alongside them. Each is now rejected rather than silently coerced, with the reasoning recorded in the doc comment and pinned by tests.

closes #405

What the old implementation did

I extracted the previous function body verbatim into a standalone binary and ran it, rather than reasoning about it on paper:

input	old result	correct value
"-5.5"	-45000000	-55000000 (that is, it returned -4.5 XLM)
"-0.5"	+5000000	-5000000 — the sign is lost entirely
"1.12345678"	11234567	should not parse
"1.2.3"	12000000	should not parse
"1.ééééééé"	panic	should not parse
i64::MAX	panic in debug, wraps negative in release	should not parse

The negative cases behave that way because the whole and fractional parts are parsed independently and recombined as whole * 10_000_000 + fractional: for -5.5 that is -50000000 + 5000000, and for -0.5 the whole part "-0" parses to 0, so nothing carries the sign at all.

The panic on "1.ééééééé" comes from padded[..7], which slices by byte index — byte 7 falls inside the fourth é. The value being sliced comes from a Horizon response body.

This matters because xlm_to_stroops is what turns the on-chain amount into the integer compared against req.amount_stroops when confirming a tip. A silently wrong value confirms or rejects the wrong payment; an error is visible and safe.

Decisions

The issue asks whether each case should error or coerce. All error, and the doc comment on the function explains why for each:

• Negative amounts — reject. Horizon never reports a negative payment amount, and as shown above this representation cannot carry a sign correctly anyway. Consistent with validation::amount::xlm_to_stroops_str, which already rejected them.
• More than 7 decimal places — reject, not truncate. Truncating understates an amount the network cannot have produced in the first place, so over-precision means the response is not what we think it is. Also consistent with the existing validation helper. Exactly 7 places remains accepted.
• Overflow — reject via checked_mul/checked_add. Total XLM supply (~10^11) sits far inside i64 stroops, so this only triggers on a malformed or hostile response, but unchecked it panics in debug and wraps to a negative amount in release.
• Anything that is not digits[.digits] — reject. This covers the second . whose tail was previously discarded, plus exponent notation, explicit +, surrounding whitespace, and non-ASCII digits. Restricting to ASCII digits is also what makes the fixed-width fractional handling byte-safe, which removes the panic path rather than papering over it.

Valid Horizon-shaped input is unaffected: "10.5000000", "0.0000001", "100", and short fractions like "10.5" all convert exactly as before.

Tests

Ten tests in a new xlm_to_stroops_tests module next to the function:

• Horizon-shaped amounts, and short/absent fractional parts ("10.5" is five tenths, not five stroops).
• Negative amounts, including an explicit test that -5.5 and -0.5 are not coerced to the wrong magnitude or sign.
• Over-precision rejected rather than truncated, with exactly 7 places still accepted as the boundary.
• Overflow: i64::MAX as an XLM amount, a value that overflows the addition rather than the multiplication (922337203685.4775808), and a value too large for i64 before scaling. Plus 922337203685.4775807, which is exactly i64::MAX stroops and must still be accepted.
• Malformed shapes, and multi-byte fractions that previously panicked.
• Agreement with validation::amount::xlm_to_stroops_str on both accepted and rejected inputs, so the two converters cannot drift — one parses our own request amounts, the other the on-chain amount they get compared against.

Verification

cargo fmt --all is clean and cargo clippy --all-targets --all-features reports no lint against any of the changed code. Note the repo-wide clippy gate is already failing independently of this PR: with -D warnings as CI runs it, unmodified main at 2a91dcd exits 101 with 188 pre-existing dead-code and unused-import errors.

cargo test --lib passes 233 and leaves only the three failures main also has locally: db::transaction::tests::savepoint_recovery_works and transaction_rollback_works (both need a live database) and the ws::integration::ping_timeout_disconnects_after_missed_pongs timing flake.

[Christopherdominic]

Thanks for picking this up, @arandomogg — this is assigned to you via the GrantFox bot for issue #405.

Before this can be merged, all required CI checks need to pass. Currently failing:

• ❌ clippy
• ❌ test

Currently passing:

• ✅ fmt
• ✅ sqlx
• ✅ docker

Please push a fix and make sure the full CI suite is green — I'll take another look once it is. Thanks for the contribution!

[arandomogg]

#405 — xlm_to_stroops hardening. @Bonizozo this is ready for review. One thing needs your action first: the latest run on this branch is sitting in action_required, so the workflow will not start until you approve it. Nothing has executed against the new commit yet.

A note on the two red jobs, because neither was caused by this PR.

test — fixed here. The seven failures in advanced_integration_tests were all the same thing: duplicate key value violates unique constraint "creators_username_key" while each test created its own creator. Cargo runs the test functions in a binary on parallel threads, and they all share one database, but TestContext::cleanup called common::cleanup_test_db, which TRUNCATEs creators and tips outright. A test finishing mid-run therefore deleted the rows every other in-flight test was still using, and with fixed usernames shared across tests an insert would race a neighbour's truncate and collide on the unique index. This PR gives each TestContext a random namespace, derives every creator username and email from it, and deletes only that namespace's rows on cleanup instead of truncating shared tables. The tests keep their parallelism and no longer depend on each other's data or on leftovers from a previous run. Callers that read the username back out of the create response, such as tip_flows::test_bulk_tip_processing, are unaffected.

clippy — pre-existing, and not something this PR should carry. cargo clippy --all-targets --all-features -- -D warnings currently reports roughly 2400 lint instances across 98 files, about 1700 of them dead_code and 470 unused_imports. None are in the code this PR touches. For what it is worth on the history: the gate arrived in #356, which was merged with no checks reported on its own branch, so it never ran against itself; the pipeline's first execution was the push to main three seconds after that merge, and it failed. Every CI run in this repository so far has failed, on main included.

I deliberately did not paper over that. Dropping -D warnings to force a green tick would have been a one-line change, but it weakens the gate for everyone and that is your call, not mine. Clearing the debt properly is a real piece of work that deserves its own PR rather than riding along inside an unrelated change. So clippy will still be red here, exactly as red as it is on main.

Happy to open that cleanup PR separately if you want it, and equally happy to split the test-isolation commit out of this one if you would rather keep this PR to a single concern.