feat(store): arbitrary-precision amounts for EVM compatibility - #236
Open
Nexha-dev wants to merge 1 commit into
Open
feat(store): arbitrary-precision amounts for EVM compatibility#236Nexha-dev wants to merge 1 commit into
Nexha-dev wants to merge 1 commit into
Conversation
i64 stroops assume 7 decimals and 64-bit range; ETH is 18 decimals and ERC-20 amounts are uint256, so ~9.3 ETH would silently overflow. Adds octo-chain::Amount, wrapping primitive_types::U256 (chosen over alloy-primitives/ruint for MSRV compatibility and decimal Display/FromStr out of the box, without pre-deciding the EVM RPC stack from Octo-Protocol-org#217 — see the module docs in crates/chain/src/amount.rs for the full rationale). Every narrowing conversion (TryFrom<i64>, to_i64, to_u64, TryFrom<BigDecimal>) is fallible and rejects out-of-range, negative, or non-integer input instead of truncating or wrapping. Storage gains amount_base_units NUMERIC(78,0) (migration 0021_numeric_amounts.sql), dual-written alongside amount_stroops for at least one release; the >0 check is added NOT VALID + VALIDATE CONSTRAINT so it never takes a long-lived lock. amount_stroops stays the column of record for now — the public API still returns it unchanged as a JSON number; the string-typed cutover is a separate, versioned change tracked in Octo-Protocol-org#227. Amount's Serialize/Deserialize round-trips through JSON strings only — the Deserialize visitor has no visit_u64/visit_i64/visit_f64 overrides, so a bare JSON number is rejected outright rather than silently coerced, since a uint256 in a JSON number loses precision in every JavaScript client. format_amount (crates/api/src/routes/submit.rs) no longer round-trips through f64: it's now exact integer/modulo arithmetic parameterised by a decimals count, which matters once amounts exceed 2^53 (18-decimal EVM amounts routinely do, even for small balances). The wallet-core/crypto clippy::cast_possible_truncation / cast_sign_loss lint wall is extended to octo-chain, octo-store, octo-ingest, and octo-api, since these crates now handle amount conversions where an unchecked `as` cast is a fund-loss bug. The remaining `as i64` casts in the diff's crates (Duration::as_secs() as i64 for timestamps in ingest/lib.rs and api/auth.rs) are pre-existing, untouched by this change, and are Unix timestamps rather than amounts, so they are out of scope here and unaffected by the new lint wall (same-width u64->i64 is cast_possible_wrap, not cast_possible_truncation or cast_sign_loss). Property tests round-trip the full uint256 range through both BigDecimal and JSON string. Explicit boundary cases cover 0, 1, i64::MAX, i64::MAX+1, u64::MAX, and 2^256-1. A regression test locks in that a JSON number amount is rejected, not coerced. A store-level test confirms 10^18 (1 ETH) survives a full store -> API JSON round trip bit-identically, which the old i64/f64 schema could not represent. BREAKING CHANGE: none yet for public API consumers — amount_stroops is unchanged. The eventual string-typed amount field cutover across the public API is tracked separately in Octo-Protocol-org#227 and must be versioned before merge, per that issue's coordination requirement. Closes Octo-Protocol-org#215
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
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
octo-chain::Amount, an arbitrary-precision non-negative base-unit integer wrappingprimitive_types::U256, with fallibleTryFrom<i64>/to_i64()/to_u64()/TryFrom<BigDecimal>conversions — every narrowing conversion rejects out-of-range, negative, or non-integer input instead of truncating or wrapping.0021_numeric_amounts.sql:amount_base_units NUMERIC(78,0)ontransactions/withdrawals, backfilled fromamount_stroops, with aNOT VALID+VALIDATE CONSTRAINT> 0check so it never takes a long-lived lock.amount_stroopsstays the column of record for at least one release (rollback path).Amount'sSerialize/Deserializeround-trips through JSON strings only — the deserializer has novisit_u64/visit_i64/visit_f64overrides, so a bare JSON number is rejected outright, not silently coerced (auint256in a JSON number loses precision in every JS client, per AD-3).format_amount'sf64round-trip (crates/api/src/routes/submit.rs) with exact integer/modulo arithmetic parameterised by adecimalscount — matters once amounts exceed2^53, which 18-decimal EVM amounts routinely do.wallet-core/cryptoclippy::cast_possible_truncation/cast_sign_losslint wall toocto-chain,octo-store,octo-ingest, andocto-api.octo-store'srecord_deposit/record_withdrawal_transaction/create_withdrawalto dual-writeamount_base_unitsvia a fallibleAmount::try_fromconversion, andocto-ingest's amount parser docs/tests to lock in that every value it can produce fitsAmountwithout error.docs/api.md/docs/openapi.yamlto document the new internal column and make clear the public API is unchanged in this PR —amount_stroopsstill serialises as a JSON number; the string-typed cutover is a separate, versioned breaking change tracked in feat(api): Multi-chain API surface, OpenAPI, and webhooks #227.Why
primitive-typesoveralloy-primitives/ruintSee the module doc in
crates/chain/src/amount.rs. Short version: the workspace pinsrust-version = "1.84";primitive-typeshas a long track record at a low MSRV with no churn that would fight that pin.alloy-primitivesis the type thealloyEVM RPC client ecosystem is built on, but which EVM RPC client octo uses is explicitly out of scope here (#217) — pulling it in now would silently pre-decide that choice and drag its dependency tree into crates (likeocto-store) that have nothing to do with EVM RPC.primitive_types::U256'sDisplay/FromStrare decimal out of the box, matching the JSON-string, no-floats representation this type needs, wherealloy_primitives::U256defaults to hex.Amount's public API (string/BigDecimal/i64conversions) does not need to change if a later issue standardizes onruint/alloy-primitivesfor the EVM RPC path — onlyAmount's private backing type would, which is the point of the newtype.Remaining
as i64casts in touched cratescrates/ingest/src/lib.rs:164andcrates/api/src/auth.rs:644:Duration::as_secs() as i64— pre-existing, untouched by this diff, and a Unix timestamp, not an amount. Same-widthu64 -> i64isclippy::cast_possible_wrap, notcast_possible_truncation/cast_sign_loss, so it's unaffected by (and doesn't defeat the purpose of) the new lint wall.crates/chain/src/amount.rs: the twoascasts left (i64::MAX as u64into_i64's bound check, andvalue as u64inTryFrom<i64>) are each preceded by an explicit range/sign guard that makes the cast lossless, and are commented explaining why;#[allow]is scoped to the single expression, not the crate.Test plan
cargo test -p octo-chain -p octo-api -p octo-ingest --lib— includes the newAmountproperty tests (roundtrip_via_bigdecimal,roundtrip_via_json_string, full uint256 range via proptest), explicit boundary tests (0, 1,i64::MAX,i64::MAX + 1,u64::MAX,2^256 - 1), the JSON-number-rejected regression test, and theformat_amountinteger-arithmetic tests.cargo test -p octo-store --testsagainst a live Postgres — includesi64_max_stroops_round_trips_through_amount_base_units,one_eth_in_wei_round_trips_through_store_and_json_bit_identically(the case the oldi64schema could not represent),withdrawal_dual_writes_amount_base_units, and the migration idempotency/expected-version-set tests.cargo clippy --workspace --all-targets -- -D warnings— clean.cargo build --workspace --all-targets— clean.Coordination
This is a breaking-API-change preparation PR, not the cutover itself:
amount_stroopsstill serialises as a JSON number, unchanged, in this PR. Per the issue guidelines, the actual string-typed public API cutover must be versioned and coordinated with #227 before that follow-up merges.Closes #215