Skip to content

BigInt micro-credit math in order-selector (audit C1)#135

Open
brawlaphant wants to merge 1 commit into
regen-network:mainfrom
brawlaphant:fix/order-selector-bigint-math
Open

BigInt micro-credit math in order-selector (audit C1)#135
brawlaphant wants to merge 1 commit into
regen-network:mainfrom
brawlaphant:fix/order-selector-bigint-math

Conversation

@brawlaphant

Copy link
Copy Markdown
Contributor

Closes part of audit C1-HIGH (the order-selector hot path). pool.ts and evm-wallet.ts have similar issues but wider blast radius — happy to take those as separate PRs.

Problem

Audit (AUDIT.md) flagged three concrete failure modes in the greedy fill loop:

Site Issue
order-selector.ts:124 parseFloat(order.quantity) loses precision on strings with >15 significant digits
order-selector.ts:127 Math.min(remaining, available) then remaining -= take — float subtraction accumulates rounding error across iterations
order-selector.ts:131 BigInt(Math.ceil(take * 1_000_000))0.1 * 1_000_000 = 100000.00000000001, then Math.ceil rounds the spurious digit UP and over-charges by 1 micro

"Over many orders, the greedy fill loop in order-selector accumulates rounding errors. For small quantities, users might overpay or underpay by fractions of a micro-unit."

What this PR does

Rewrites the loop to use bigint micro-credits (1 credit = 1_000_000 micro) end-to-end. The single float touch is numberToMicro at the public entry point, where a JS number from the caller is rounded to the nearest micro. That's the unavoidable cost of accepting quantity: number from MCP/REST callers; everywhere else is exact.

Three small pure helpers (exported under __orderSelectorInternals for direct testing):

  • parseQuantityToMicro(decimal: string): bigint — string "10.5" → bigint 10_500_000n
  • formatMicroToQuantity(micro: bigint): string — bigint → fixed-precision string
  • numberToMicro(quantity: number): bigintMath.round(quantity * 1_000_000), error-bounded

Cost rounding now uses ceiling division ((a + scale - 1) / scale) so any sub-micro fraction is charged to the buyer rather than absorbed by the seller — matches how on-chain MsgBuyDirect settles.

Tests

14 new cases. Highlight: a regression test that sums 10 × 0.1 credits and asserts it equals exactly 1.0 (with parseFloat / Math.ceil, the previous code drifted by 1 micro).

✓ src/__tests__/order-selector.test.ts (14 tests)
  ✓ quantity micro conversion (6)
    ✓ parses integer quantities
    ✓ parses fractional quantities exactly
    ✓ truncates beyond 6 decimal places (sub-micro is not representable)
    ✓ rejects malformed input
    ✓ formatMicroToQuantity round-trips
    ✓ numberToMicro rounds half to nearest
  ✓ selectBestOrders greedy fill (audit C1) (7)
    ✓ fills exactly when supply matches request
    ✓ walks across multiple orders cheapest-first
    ✓ flags insufficient supply when total available < request
    ✓ skips orders with malformed quantity rather than blowing up
    ✓ does not accumulate float error across the greedy fill (audit C1)
    ✓ computes exact cost with fractional take and high-precision price
    ✓ rounds cost UP so the buyer covers the sub-micro remainder

Full suite: 63/63 passing (49 prior + 14 new).

Test plan

  • npm run typecheck — clean
  • npm test — 63/63 passing
  • npm run build — clean
  • Reviewer: confirm ceiling-division on cost matches your accounting expectation. Audit said "users might overpay or underpay" — I picked overpay-by-at-most-1-micro because it matches on-chain settlement. Easy to flip to floor or banker's rounding if you prefer the seller absorb sub-micro.
  • Reviewer: spot-check that no caller of selectBestOrders was relying on the old parseFloat-style total — all callers I traced (retirement.ts, retire-subscriber.ts) consume totalCostMicro as bigint and totalQuantity as a string, both of which keep working.

Out of scope (audit C1 follow-ups)

  • pool.ts:324 parseFloat(finalSelection.totalQuantity) for display
  • pool.ts:388 sub.amount_cents / totalRevenueCents for attribution fractions
  • retirement.ts:169 parseFloat(selection.totalQuantity) for display
  • evm-wallet.ts:110 Math.round(amountUsdc * 10 ** decimals)

Each of those can be a focused PR. The order-selector site was the one the audit flagged as highest-risk because of the multi-iteration accumulation.

Refs: AUDIT.md C1-HIGH

🤖 Generated with Claude Code

Closes part of audit C1-HIGH (the order-selector hot path; pool.ts
and evm-wallet.ts remain for follow-up PRs). Previously the greedy
fill loop used parseFloat() and JS numbers throughout:

  src/services/order-selector.ts:124  parseFloat(order.quantity)
  src/services/order-selector.ts:127  Math.min(remaining, available)
  src/services/order-selector.ts:131  BigInt(Math.ceil(take * 1_000_000))

The audit flagged three concrete failure modes:
  - sell-order quantity strings with >15 significant digits lose precision
    when parsed through `parseFloat`
  - float subtraction in `remaining -= take` accumulates rounding error
    across iterations of the greedy fill
  - `take * 1_000_000` produces results like 100000.00000000001, then
    Math.ceil rounds the spurious digit UP and over-charges by 1 micro

This rewrites the loop to use bigint micro-credits end-to-end. Adds
three small pure helpers:
  - parseQuantityToMicro(decimal: string): bigint
  - formatMicroToQuantity(micro: bigint): string
  - numberToMicro(quantity: number): bigint

The single float touch is `numberToMicro` at the entry, where a JS
`number` from the caller is rounded to the nearest micro-credit.
That's the unavoidable cost of accepting `quantity: number` at the
public API; everywhere else the math is exact.

Cost rounding now uses ceiling division so any sub-micro fraction
is charged to the buyer rather than absorbed by the seller (matches
how on-chain MsgBuyDirect would settle).

Tests: 14 new vitest cases. Coverage:
  - parseQuantityToMicro: integer / fractional / sub-micro truncation
    / malformed input rejection / 0.1 + 0.2 = 0.3 exactness
  - formatMicroToQuantity round-trip
  - numberToMicro half-rounding behavior
  - selectBestOrders: exact fill, multi-order cheapest-first walk,
    insufficient supply, malformed-quantity skip-don't-fail
  - REGRESSION: 10 × 0.1 sums to exactly 1.0 (no float drift)
  - exact cost for fractional take with high-precision price
  - cost rounds UP not down

Full suite: 63/63 passing (49 prior + 14 new).

The internal helpers are exported under __orderSelectorInternals
for direct unit testing — they're pure, simple, and worth covering.

Refs: AUDIT.md C1-HIGH (order-selector portion)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request replaces floating-point arithmetic with BigInt micro-credits in the order selection logic to prevent rounding errors and ensure exact calculations. Key changes include the introduction of helper functions for parsing, formatting, and converting quantities to micro-credits, as well as updating the greedy fill loop to use these helpers. Feedback focuses on refining the insufficientSupply check to be more precise now that BigInt is used, improving the robustness of the decimal parsing regex to handle common input formats like leading or trailing dots, and addressing a behavioral inconsistency between string truncation and numeric rounding when handling sub-micro quantities.

Comment on lines +219 to 221
if (remainingMicro > 1n) {
insufficientSupply = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The condition remainingMicro > 1n allows the selection to be up to 1 micro-credit short without flagging insufficientSupply. While this might have been a workaround for floating-point inaccuracies in the previous implementation, it is no longer necessary with BigInt. If the requested quantity cannot be fully met (even by 1 micro), insufficientSupply should be true to ensure the caller is aware of the shortfall.

Suggested change
if (remainingMicro > 1n) {
insufficientSupply = true;
}
if (remainingMicro > 0n) {
insufficientSupply = true;
}

*/
function parseQuantityToMicro(decimal: string): bigint {
const trimmed = decimal.trim();
if (!/^-?\d+(\.\d+)?$/.test(trimmed)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The regex /^-?\d+(\.\d+)?$/ is more restrictive than standard decimal parsing as it rejects strings with leading dots (e.g., .5) or trailing dots (e.g., 1.). Consider using a more inclusive regex to match common user input formats. Note that if you update this, you should also handle empty whole parts in the BigInt conversion on line 35.

Suggested change
if (!/^-?\d+(\.\d+)?$/.test(trimmed)) {
if (!/^-?(\d+(\.\d*)?|\.\d+)$/.test(trimmed)) {

const negative = trimmed.startsWith("-");
const body = negative ? trimmed.slice(1) : trimmed;
const [whole, frac = ""] = body.split(".");
const fracPadded = (frac + "0".repeat(QUANTITY_EXPONENT)).slice(0, QUANTITY_EXPONENT);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a behavioral inconsistency between parseQuantityToMicro and numberToMicro. parseQuantityToMicro truncates digits beyond the 6th decimal place, while numberToMicro rounds to the nearest micro-credit using Math.round. For example, the string "0.0000006" parses to 0n, but the number 0.0000006 converts to 1n. It is recommended to use a consistent strategy for both to avoid unexpected discrepancies depending on the input type.

const body = negative ? trimmed.slice(1) : trimmed;
const [whole, frac = ""] = body.split(".");
const fracPadded = (frac + "0".repeat(QUANTITY_EXPONENT)).slice(0, QUANTITY_EXPONENT);
const micro = BigInt(whole) * QUANTITY_SCALE + BigInt(fracPadded || "0");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If the regex on line 28 is updated to allow leading dots (like .5), the whole variable will be an empty string. BigInt("") throws a SyntaxError, so it is safer to provide a fallback to "0".

Suggested change
const micro = BigInt(whole) * QUANTITY_SCALE + BigInt(fracPadded || "0");
const micro = BigInt(whole || "0") * QUANTITY_SCALE + BigInt(fracPadded || "0");

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.

1 participant