fix(core): validate decimal precision by value, not by scale (unblocks app-session Withdraw) - #880
Conversation
ValidateDecimalPrecision rejected amounts whose *scale* exceeded the asset's decimals, even when the value carried no significant digits beyond them. Decimal scale is widened by ordinary arithmetic and by storage round-trips. A session balance read back via SUM() over a NUMERIC column arrives with an 18-place scale; decimal.Sub propagates the wider scale of its operands, so a derived amount such as the withdraw delta in handleWithdrawIntent inherits it and is rejected — even when the amount is exactly representable in the asset's decimals. Compare against the truncated value instead. Values that genuinely need more precision than the asset allows are still rejected; values that merely carry a wide scale are now accepted. Adds regression coverage for the trailing-zero and subtraction-derived cases, plus a case asserting genuine excess precision is still rejected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDecimal precision validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/core/utils.go (1)
120-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Go doc comment for
ValidateDecimalPrecision.The new comments document the implementation inside the function. They do not document the exported API. Add a comment above the declaration that describes the value-based precision rule.
Suggested comment
+// ValidateDecimalPrecision returns an error when amount is not exactly +// representable with maxDecimals fractional digits. func ValidateDecimalPrecision(amount decimal.Decimal, maxDecimals uint8) error {As per coding guidelines, Go code must include doc comments on all exported names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/core/utils.go` around lines 120 - 126, ِAdd a Go doc comment immediately above the exported ValidateDecimalPrecision declaration describing its value-based precision rule, clarifying that validation compares the decimal’s truncated value rather than its exponent or scale. Keep the existing implementation comments unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/core/utils.go`:
- Line 127: Update ValidateDecimalPrecision before the amount.Truncate
comparison to guard against exponent gaps that could trigger unbounded
rescaling, rejecting extreme exponents safely. Alternatively, replace the
Truncate-based precision check with a bounded coefficient/trailing-zero
calculation, while preserving existing validation behavior for normal decimal
values.
---
Nitpick comments:
In `@pkg/core/utils.go`:
- Around line 120-126: ِAdd a Go doc comment immediately above the exported
ValidateDecimalPrecision declaration describing its value-based precision rule,
clarifying that validation compares the decimal’s truncated value rather than
its exponent or scale. Keep the existing implementation comments unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fab2ae86-db41-4c25-b7eb-fe1c46e621ac
📒 Files selected for processing (2)
pkg/core/utils.gopkg/core/utils_test.go
| // come back as 0.030000000000000000 (exponent -18); subtracting from it | ||
| // propagates that scale, and the result would be rejected here even though | ||
| // the value is exactly representable in 6 decimals. | ||
| if !amount.Equal(amount.Truncate(int32(maxDecimals))) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 '\bValidateDecimalPrecision\s*\(' --glob '*.go'
rg -n -C 8 '\b(DecimalToUint256|DecimalToInt256|decimalToBigInt)\s*\(' --glob '*.go'
rg -n -C 8 'NewFromString|UnmarshalJSON|decimal\.Decimal' --glob '*.go'Repository: layer-3/nitrolite
Length of output: 155
🏁 Script executed:
#!/usr/bin/env bash
set -ueo pipefail
echo "Go files:"
git ls-files '*.go' | sed -n '1,120p'
echo
echo "Locate utils.go:"
fd -a 'utils\.go$' . | sed 's#^\./##' | sed -n '1,120p'
echo
echo "Search ValidateDecimalPrecision:"
rg -n -C 8 '\bValidateDecimalPrecision\s*\(' . --glob '*.go' || true
echo
echo "Search decimal conversion helpers:"
rg -n -C 8 '\b(DecimalToUint256|DecimalToInt256|decimalToBigInt)\b' . --glob '*.go' || true
echo
echo "Search decimal inputs:"
rg -n -C 8 '\b(NewFromString|UnmarshalJSON|decimal\.Decimal)\b' . --glob '*.go' || trueRepository: layer-3/nitrolite
Length of output: 50374
🏁 Script executed:
#!/usr/bin/env bash
set -ueo pipefail
echo "pkg/core/utils.go relevant section:"
sed -n '100,150p' pkg/core/utils.go
echo
echo "pkg/core/utils_test.go relevant precision tests:"
sed -n '160,190p' pkg/core/utils_test.go
sed -n '215,245p' pkg/core/utils_test.go
echo
echo "Read-only behavioral probe of current validation logic with shopsspring/decimal v1.4.0 source semantics:"
python3 - <<'PY'
# This script only mirrors the source shape; it does not run repository code.
# Extract the expression string and classify it as rescaling/truncating.
path = "pkg/core/utils.go"
needles = []
with open(path) as f:
for i,line in enumerate(f, start=1):
if "amount.Truncate(int32(maxDecimals))" in line or "ValidateDecimalPrecision" in line:
needles.append((i,line.rstrip()))
for needle in needles:
print(f"{path}:{needle[0]}: {needle[1]}")
PY
echo
echo "Go.mod dependency:"
sed -n '1,80p' go.mod 2>/dev/null || cat go.mod | sed -n '1,80p'Repository: layer-3/nitrolite
Length of output: 8633
🌐 Web query:
github.com/shopspring/decimal v1.4.0 Truncate rescale exponent 10^ power_of_10 decimal.go
💡 Result:
In the shopspring/decimal Go library, the Truncate and rescale methods are closely related, with Truncate relying on rescale to adjust the decimal's exponent [1][2][3]. 1. rescale: This internal method changes the exponent of a Decimal while keeping its value represented as an arbitrary-precision integer [1][4]. - It effectively multiplies or divides the underlying integer value by 10^(abs(diff)), where diff is the difference between the current exponent and the target exponent [1][4]. - When decreasing precision (increasing the exponent), it uses big.Int.Quo, which truncates the value toward zero [1][4]. - Documentation explicitly states: "NOTE: this will truncate, NOT round" [1][4]. 2. Truncate(precision int32): This method allows you to truncate digits from a number [5]. - Prior to recent updates (e.g., in version 1.4.0), the Truncate method typically required a non-negative precision (precision >= 0) and used rescale to shift the exponent [1][2][5]. - Developments subsequent to v1.4.0 (such as those discussed in pull request #426) have updated Truncate to support negative precision [3]. This allows for truncation of the integer part toward zero (e.g., truncating 5432 with a precision of -2 results in 5400) [3]. - The mechanism remains a call to rescale, which inherently performs the required truncation via big.Int.Quo [1][3]. If you are using version v1.4.0 specifically, note that the library was in a state where Truncate often enforced a non-negative precision check, which was later relaxed in development versions of the library to support negative precision [2][3].
Citations:
- 1: https://github.com/shopspring/decimal/blob/v1.4.0/decimal.go
- 2: Truncate should parse negative values too, or return error shopspring/decimal#406
- 3: fix: normalize exponent in rounding functions and support negative precision in Truncate shopspring/decimal#426
- 4: https://github.com/shopspring/decimal/blob/master/decimal.go
- 5: https://pkg.go.dev/github.com/shopsprint/decimal
🌐 Web query:
github.com/shopspring/decimal v1.4.0 decimal.go rescale func(rescale)
💡 Result:
In the shopspring/decimal library version 1.4.0, the rescale function is a method on the Decimal type that returns a new Decimal with a specified exponent [1]. Key characteristics of the rescale function include: Functionality: It adjusts the decimal to the target exponent provided by the user [1]. If the target exponent is greater than the current exponent, the function performs a truncation (reducing precision); if the target exponent is smaller, it scales the coefficient up by the appropriate power of 10 [1]. Important Behavior: The function explicitly performs truncation rather than rounding when decreasing precision [1]. Implementation: - If the target exponent matches the current exponent, it returns a copy of the original Decimal [1]. - It utilizes math.Abs to calculate the difference between the current and target exponents to determine the required scaling factor [1]. - It uses big.Int operations (Quo or Mul) to perform the scaling [1]. Example Usage (as documented in the source): go d := decimal.New(12345, -4) // Represents 1.2345 d2 := d.rescale(-1) // Results in 1.2 d3 := d2.rescale(-4) // Results in 1.2000 Note: While rescale is a core internal method used for operations like addition and subtraction (often via the RescalePair helper), it is worth noting that it is not typically exported for direct package-level use in the same way as public API methods [1]. Users often rely on higher-level operations or Round for precision management [1].
Citations:
🏁 Script executed:
#!/usr/bin/env bash
set -ueo pipefail
echo "Remote v1.4.0 Truncate and rescale implementations:"
curl -sSL 'https://raw.githubusercontent.com/shopspring/decimal/v1.4.0/decimal.go' | sed -n '240,285p;530,575p'
echo
echo "Local occurrences of amounts parsed/constructed before ValidateDecimalPrecision:"
rg -n -C 4 '\bValidateDecimalPrecision\s*\(' pkg --glob '*.go' || true
echo
echo "Search for decimal.NewFromString outside tests:"
rg -n '\bdecimal\.NewFromString\s*\(' . --glob '*.go' --glob '!**/*_test.go' | sed -n '1,200p'Repository: layer-3/nitrolite
Length of output: 10867
🏁 Script executed:
#!/usr/bin/env bash
set -ueo pipefail
echo "pkg/core/state_advancer.go relevant validation":
sed -n '1,90p' pkg/core/state_advancer.go
echo
echo "pkg/core/state_advancer.go amount field/usages":
rg -n -C 4 'newTransition\.Amount|Amount\s+.*decimal|Decimal|Transition' pkg/core/state_advancer.go --glob '*.go' | sed -n '1,220p'
echo
echo "Remote shopspring decimal v1.4.0 rescale implementation:"
url='https://raw.githubusercontent.com/shopspring/decimal/v1.4.0/decimal.go'
curl -sSL "$url" | perl -nle '
if (/^\s*(func|type|var|const)\b/ && /rescale|Decimal/) { $in=1; $start=$. }
if ($in) { print "$.: $_"; if (/^\s*}/ && /rescale/){ $in=0 } }
'Repository: layer-3/nitrolite
Length of output: 50373
Prevent unbounded rescaling for extreme exponents.
ValidateDecimalPrecision now depends on amount.Truncate(...), and shopspring/decimal v1.4.0 rescales by computing 10^diff from the exponent gap. String parsing accepts small scientific exponents before validation, so an amount such as 1e-2147483648 can force an enormous allocation before returning an error. Add an extreme-exponent limit before this rescaling, or implement the precision test with a bounded trailing-zero/coefficient check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/core/utils.go` at line 127, Update ValidateDecimalPrecision before the
amount.Truncate comparison to guard against exponent gaps that could trigger
unbounded rescaling, rejecting extreme exponents safely. Alternatively, replace
the Truncate-based precision check with a bounded coefficient/trailing-zero
calculation, while preserving existing validation behavior for normal decimal
values.
Source: MCP tools
|
Ran the suites locally after opening this (go1.26.5): Also worth flagging: |
|
Thanks — addressed. Note there was already a doc comment on the function; rather than adding a second one I rewrote it, since the previous wording ("doesn't exceed the maximum allowed decimal places") is ambiguous now that the rule is value-based: // ValidateDecimalPrecision returns an error when amount is not exactly
// representable with maxDecimals fractional digits.
//
// The rule is value-based, not scale-based: an amount carrying trailing
// zeros beyond maxDecimals (for example 0.030000000000000000 against a
// 6-decimal asset) is valid, because no significant digit is lost by
// representing it with maxDecimals.
|
1 participant
Problem
submit_app_statewithintent=Withdrawfails for any 6-decimal asset on mainnet:The rejected value is not one the client sends.
handleWithdrawIntentderives it:The client's own allocation amount is already validated earlier in the shared path and passes — we send canonical 2-dp strings such as
0.03.Cause
ValidateDecimalPrecisiontests the decimal's exponent (its scale) rather than its actual precision:Scale is widened by ordinary arithmetic and by storage round-trips.
GetParticipantAllocationsreads balances viawhich returns an 18-place scale;
decimal.Subrescales to the smaller exponent of its operands, sowithdrawAmountinherits scale −18 and is rejected even though the value (e.g.0.030000000000000000) is exactly representable in 6 decimals.handleOperateIntentperforms the same subtraction and does not precision-check the result, which is why Operate works and only Withdraw fails.Fix
Compare against the truncated value instead of inspecting the exponent. Amounts that genuinely require more precision than the asset allows are still rejected; amounts that merely carry a wide scale are accepted.
Alternative if you prefer a narrower change: normalize
withdrawAmountto the asset's decimals insidehandleWithdrawIntentbefore validating, and leaveValidateDecimalPrecisionuntouched. Happy to reshape the PR that way.Tests
All existing
TestValidateDecimalPrecisioncases are unaffected — none of them uses a value whose scale exceeds its significant digits. Added three cases:trailing_zeros_within_precision—0.030000000000000000accepted at 6 dptrailing_zeros_from_subtraction— reproduces the withdraw path (0.060000000000000000 - 0.03)wide_scale_with_real_excess_precision_still_rejected—0.030000010000000000still rejected at 6 dpVerified locally with
go1.26.5:Reproduction
Mainnet,
wss://nitronode.yellow.org/v1/ws, Base 8453, USDT, 2-party app session. Deposit and Operate both succeed; only Withdraw fails. Two runs with different amounts produced the identical error:0x249dfd0e086eb44dd84c896f435a129abbe1e3b37c6e5c32048c0e09fe1047970x7589be438fa4a4249cf913f43121a20d893d3832bf42d07ba9e7d8819c7e89abImpact
Without this, funds can only leave an app session via
Close, so a participant cannot move a realized balance back to their home channel while the session is still open.Summary by CodeRabbit