Uh oh!
There was an error while loading. Please reload this page.
fix: reject fractional wei in toWei - #303
Open
gomesalexandre wants to merge 2 commits into
Open
Conversation
unitLengths computed each unit's baseLength as `value.length - 1 || 1`.
wei's raw value is '1' (length 1), so `1 - 1` is `0`, and the `|| 1`
fallback (meant defensively for units that legitimately need it, but
none do) silently coerced wei's baseLength from the correct 0 up to 1.
toWei('0.5', 'wei') returned 5n instead of throwing "too many decimal
places", since a single fractional digit fit inside the inflated
baseLength of 1.
Dropping the `|| 1` alone isn't enough: toWei() also unconditionally
defaulted a missing fraction to the string '0' before checking its
length against baseLength, which was harmless while baseLength was
always >= 1 but throws on ordinary whole-wei input (toWei(0, 'wei'),
toWei('5', 'wei')) once baseLength is correctly 0. Fixed by only
running the length check/pad when the input actually supplied a
fraction; a genuinely absent fraction is always 0 regardless of the
unit's precision.
One existing test needed updating: toWei('0.0', 'wei') was asserted to
return 0n, but that was the same bug in disguise - wei has zero decimal
places, so an explicit fractional digit (even a literal zero) should be
rejected exactly like it would be for any other unit given more
fractional digits than it supports.
Guard-validated: reverting the source change makes the new
"should reject fractional wei" test fail; restoring it passes again.
Co-Authored-By: Claude <noreply@anthropic.com>
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 freeto 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.
No filed issue, found by inspection.
unitLengthscomputed each unit's decimal precision asvalue.length - 1 || 1.Wei's raw value is
'1'(length 1), so1 - 1is0- the correct precision forwei, since it's the base unit with zero decimal places. But the
|| 1fallbacksilently coerced that
0up to1, sotoWei('0.5', 'wei')returned5ninstead of throwing "too many decimal places" the way it does for any other unit
given more fractional digits than it supports.
Fix, and why it's more than dropping
|| 1Dropping
|| 1alone breaks 5 existing tests (toWei(0, 'wei'),toWei('5', 'wei'), etc. start throwing). Root cause:toWei()unconditionallydefaulted a missing fraction to the string
'0'before comparing its lengthagainst
baseLength- harmless whilebaseLengthwas always >= 1, but brokenonce wei's is correctly
0. Restructured so the length-check/pad only runs whenthe input actually supplied a fraction; a genuinely absent fraction is always
0regardless of the unit's precision.
Caller-visible behavior change - flagging explicitly
toWei('0.0', 'wei')used to return0n. It now throws. This is a real,intentional change, not incidental: the library already rejects an all-zero
fraction that exceeds a unit's precision for every other unit -
toWei('0.' + '0'.repeat(19), 'ether')throws on unmodifiedmain, nozero-value special case exists anywhere in the file. Wei was the only unit
accepting an over-precision fraction, and only because of this bug. Matches how
ethers'
parseUnits/parseFixedalso reject fractional digits beyond a unit'sdecimals regardless of whether those digits are zero. Happy to add a zero-fraction
special case across all units instead if maintainers would prefer that (it would
be a bigger, separate behavior change to ether/gwei/etc, not just wei), but
rejecting fractional wei consistently with every other over-precision case seemed
like the more defensible default.
One more pre-existing quirk this PR doesn't touch:
unitLengths.noetheralsoflips from
1to0under the same fix, but it's provably dead code - bothtoWeiandfromWeiearly-return onbase === zerobeforebaseLengthis everread for that unit, so there's no behavior change there.
Testing
fromWeiis unaffected: wei'sbaseis1n, sowei % baseis always0n,making the fraction computation's
baseLengthirrelevant to its output -confirmed identical results with and without the fix.
Guard-validated: reverted the source fix, confirmed the new "should reject
fractional wei" test fails, restored, confirmed it passes. Full test suite:
1985/1986 pass (1 pre-existing unrelated timing flake in
bytes.test.ts,unrelated to
unitsConversion.ts).unitsConversion.tsat 100%statement/branch/function/line coverage.
tsc --noEmit: 7 pre-existing errors,all in
node_modules/web3-*typings (confirmed viagit stashcomparison againstunmodified
main- identical count).eslint: clean.Note
Cursor Bugbot is generating a summary for commit dd0e234. Configure here.