KeeperHub / KeeperHub/keeperhub
math/aggregate silently truncates fractional inputs when any sibling value exceeds the safe-integer range
- 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
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