KeeperHub / KeeperHub/keeperhub

math/aggregate silently truncates fractional inputs when any sibling value exceeds the safe-integer range

Open
#2,496 1 comment 0 reactions 0 assignees View on GitHub
bug confirmed
Dominant language
TypeScript
Stars
24
Forks
93
Avg merge
1d 4h
Merged PRs (30d)
253

Description

Found while triaging #2483, where `math/aggregate` is the node a workflow would compose to compute a ratio. Three faults on that path, all on `staging`. The first is the one that matters: it returns a wrong number without erroring.

**1. A fractional input is silently truncated to an integer whenever any sibling input is large.**

`plugins/math/steps/aggregate.ts:188-197` classifies a value as `kind: "bigint"` only when it is an integer string outside the safe-integer range. `:545` then sets `needsBigInt` if *any* parsed value carries that kind, and `:548-553` routes the whole set through `convertNumericValuesToBigInts`, which at `:295-298` does `BigInt(Math.trunc(v.value))` on every `kind: "number"` entry.

So summing a wei balance with a fractional rate truncates the rate to zero. Nothing throws and nothing warns; the step returns `success: true` with a wrong result. Any workflow mixing token amounts in wei with a percentage, a multiplier or a per-second rate is exposed, and the failure is invisible in the run history.

**2. Post-operation arithmetic drops out of fixed point.**

`:565-567` converts the aggregated bigint to `Number` before applying the post-operation, with the comment stating the intent. The aggregation keeps its precision and the post-operation discards it, so `sum` of wei followed by `divide` returns a float. `plugins/math/steps/decimal-core.ts` already provides `parseDecimal`, `formatScaled` and `divideScaled`, and `compare-tolerance` and `format-number` both use them - `aggregate` is the one action that does not.

**3. Divide-by-zero is a hard failure where zero is a legitimate value.**

`:404-408` throws on a zero operand. For a ratio whose denominator is a rate of change, zero is the healthy case - nothing is being consumed - and the workflow dies instead of reporting it. `modulo` at `:409-412` has the same shape. Whether these should return null, Infinity or a flagged result is a design call; throwing is the one option that loses the information.

Reason, scope and plan below.

**Reason.** A workflow that aggregates on-chain amounts alongside any fractional figure gets a silently wrong number, which is worse than an error because nothing downstream can detect it. Fault 1 is the defect; 2 and 3 are the reasons the precision-preserving path is not reachable even when a user works around 1.

**Scope.** `plugins/math/steps/aggregate.ts` only. `compare-tolerance` and `format-number` already use `decimal-core` and are not affected. No schema change, no response-shape change for correct inputs, no migration. Out of scope: whether `aggregate` should gain new operations, and the placement question under discussion on #2483.

**Plan.** Not settled, and the choice between two shapes is the thing to decide first. Either promote the mixed-kind path to a common fixed-point scale using `decimal-core` so a fraction and a wei value can be summed without either losing precision, or refuse the mix outright with an error naming the offending value. The second is much smaller and strictly better than today; the first is what a user summing a balance and a rate actually wants. Faults 2 and 3 follow whichever is chosen. I would want a test that pins the mixed-magnitude case specifically, because it is the one that passes silently today.

Contributor guide

Open the contributing guide

Research direction

Start in plugins/math/steps/aggregate.ts at the numeric-kind handling, needsBigInt routing, post-operation conversion, and divide/modulo checks. Read plugins/math/steps/decimal-core.ts and compare its use in compare-tolerance and format-number. First decide whether mixed values should use common fixed-point arithmetic or be rejected; done means the chosen behavior preserves or explicitly refuses precision-sensitive inputs and has a regression test for the mixed-magnitude case.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, blockchain
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.