2026 03 10 audit#169
Merged
thedavidmeister merged 5 commits intomainfrom Mar 23, 2026
Merged
Conversation
Full 6-pass audit with triage. Fixes applied:
- A08-7 MEDIUM: Guard large positive exponents in toDecimalString
- A01-1/A01-2: CLAUDE.md docs for scripts and deployment
- A01-3: Replace require string with ScientificMinNotLessThanMax custom error
- A01-4/A01-5: Uncomment log10/pow10 deployed-parity tests
- A01-6/A01-7: Add format(Float,bool) and format(Float) deployed-parity tests
- A01-8: Add ScientificMinNotLessThanMax revert path test
- A01-10: Fix sub NatSpec ("a from b" -> "b from a")
- A01-11: Clarify div param NatSpec (dividend/divisor)
- A01-12: Fix pow10 NatSpec to say 10^a
- A02-4: Add CoefficientOverflow direct test
- A06-2: Add packLossy lossy-but-packable path test
- A06-6: Fix NatSpec reference to non-existent fromFixedDecimalLossyMem
- A06-7: Fix packLossy NatSpec (PackedFloat -> Float, add @return lossless)
- A06-11: Fix NatSpec references to non-existent divide/power10
- A06-12: Add missing @return NatSpec tags across LibDecimalFloat.sol
- A06-16: Replace dead (lossless); expression with (Float result,) destructuring
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- A06-20: Document why pow uses implementation internals directly - A07-01: Remove 7 unused imports in LibDecimalFloatDeploy.sol - A08-1/p3: Add @param scientific to toDecimalString NatSpec - A08-5/p4: Fix redundant import path in LibFormatDecimalFloat.sol - A08-6: Remove dead countSigFigs function and its test file - A09-1/p1: Add tests for exponent overflow in unabsUnsignedMulOrDivLossy - A09-1/p3, A09-5, A09-9: Add NatSpec to div, inv, compareRescale - A09-11: Add internals test file for 6 untested internal functions - A09-12: Add ExponentOverflow test for minus - A09-13: Add Log10Zero and Log10Negative revert tests - A09-14: Add MulDivOverflow revert test - A09-16/p4, A09-17/p4: Replace magic numbers 100/2000 with named constants - A10-1/p1: Add overflow check on exponent += eValue in parseDecimalFloatInline - A10-1/p4: Consolidate split imports in LibParseDecimalFloat.sol - A10-3: Document 67-digit limit and overflow check semantics - A10-7: Add ParseDecimalFloatExcessCharacters test for wrapper - A10-8: Add ParseDecimalPrecisionLoss and ExponentOverflow tests for wrapper - Update deploy constants for changed bytecode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add NatSpec to DEPLOYMENT_SUITE_TABLES and DEPLOYMENT_SUITE_CONTRACT explaining expected env var values and default behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughThis PR introduces a production-ready Changes
Sequence DiagramsequenceDiagram
participant Client
participant DecimalFloat
participant Libraries as Format/Parse/Impl<br/>Libraries
participant LogTables as Log Table<br/>Data Contract
participant Deployer
Deployer->>LogTables: Deploy via Zoltu proxy<br/>(deterministic address)
Deployer->>DecimalFloat: Deploy via Zoltu proxy<br/>(deterministic address)
Client->>DecimalFloat: format(Float, scientificMin, scientificMax)
DecimalFloat->>DecimalFloat: Validate scientificMin < scientificMax<br/>(custom error)
DecimalFloat->>Libraries: LibFormatDecimalFloat.toDecimalString
Libraries-->>DecimalFloat: string result
DecimalFloat-->>Client: formatted string
Client->>DecimalFloat: log10(Float)
DecimalFloat->>Libraries: LibDecimalFloatImplementation.log10
Libraries->>LogTables: extcodecopy table data
LogTables-->>Libraries: log table bytes
Libraries-->>DecimalFloat: (coefficient, exponent)
DecimalFloat-->>Client: Float result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Run forge fmt on 5 test files that had formatting drift. Add view modifier to two test functions that only perform staticcalls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/parse/LibParseDecimalFloat.sol (1)
198-202:⚠️ Potential issue | 🟠 MajorPreserve the rounded-zero result on exponent underflow.
packLossyalready distinguishes the two exponent boundary cases: positive overflow reverts, while negative underflow returnsFLOAT_ZEROwithlossless == false. Mapping every!losslessresult toParseDecimalPrecisionLossturns underflowed literals into parse failures instead of rounding them toward zero.Proposed fix
- if (!lossless) { - return (ParseDecimalPrecisionLoss.selector, Float.wrap(0)); + if (!lossless) { + if (Float.unwrap(result) == Float.unwrap(LibDecimalFloat.FLOAT_ZERO)) { + return (0, result); + } + return (ParseDecimalPrecisionLoss.selector, Float.wrap(0)); } else {This will also require flipping the new negative-underflow expectation in
test/src/lib/parse/LibParseDecimalFloat.t.sol.As per coding guidelines,
src/lib/**/*.sol: Exponent underflow must silently round toward zero; exponent overflow must revert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibParseDecimalFloat.sol` around lines 198 - 202, The code currently maps every packLossy non-lossless case to ParseDecimalPrecisionLoss, which incorrectly treats exponent underflow (which packLossy represents as FLOAT_ZERO with lossless==false) as a parse failure; change the branch after LibDecimalFloat.packLossy(signedCoefficient, exponent) to only return ParseDecimalPrecisionLoss when lossless==false AND the returned result is not the rounded-zero (i.e., result != Float.wrap(0)), otherwise return the rounded-zero result; also update the related test expectation in LibParseDecimalFloat.t.sol to expect successful rounding-to-zero for negative exponent underflow cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@audit/2026-03-10-01/pass2/DecimalFloat.md`:
- Line 133: Update the audit note that claims "pow10(Float) --
DecimalFloat.pow10.t.sol has the entire testPow10Deployed body commented out" to
reflect current state: remove the stale claim and mark the finding as resolved
or as a pass-2 historical snapshot because testPow10Deployed in
DecimalFloat.pow10.t.sol is active; also make the same update for the
corresponding duplicate entry later in the document (the other pow10/Deployed
parity note).
In `@audit/2026-03-10-01/pass2/LibDecimalFloatImplementation.md`:
- Around line 44-51: The Markdown has missing blank lines between headings and
their adjacent tables/lines causing MD022/MD058; edit the markdown around the
listed table rows (entries referencing `maximize`, `maximizeFull`,
`compareRescale`, `withTargetExponent`, `intFrac`, `mantissa4`,
`lookupAntilogTableY1Y2`, `unitLinearInterpolation`) and the other indicated
heading locations (around lines noted for 95, 131, 135) to insert a single blank
line before and after each heading and before the table so that every heading is
separated from the table/content by a blank line, ensuring heading spacing rules
are satisfied.
In `@audit/2026-03-10-01/pass2/LibFormatDecimalFloat.md`:
- Around line 44-51: The markdown is missing required blank lines around
headings and tables (MD022/MD058) — locate the "### Test:
`DecimalFloat.format.t.sol`" heading and the following table block and ensure
there is an empty line before the heading and an empty line after the table;
apply the same fix to the later heading referenced at line 95 by inserting a
blank line before and after that heading as well so every heading and table is
surrounded by a single blank line.
In `@audit/2026-03-10-01/pass4/impl_format_parse_table.md`:
- Line 50: Fix the typo "copmiled" to "compiled" in the audit note text within
impl_format_parse_table.md (the occurrence referenced as A11-1 / the audit note
on pass 4); update that single word so the sentence reads correctly while
leaving surrounding wording unchanged.
In `@audit/2026-03-10-01/pass4/LibDecimalFloat.md`:
- Line 214: Update the summary sentence that currently claims “no dead code” to
acknowledge the earlier finding of a single dead expression statement in the inv
function: change the wording to something like “no other dead code aside from a
single dead expression statement in inv” (or similar concise phrasing) so the
top-level summary no longer contradicts the specific LOW finding about inv; also
ensure the summary uses the same terminology (“dead expression statement” /
“dead code”) as the detailed finding for consistency.
In `@audit/2026-03-10-01/pass5/format_parse_table.md`:
- Around line 271-278: Update the explanatory text to fix the power-of-ten
numeric errors and clarify the two distinct failure modes: state that 10^68 ==
1e68 (not ~2.68e67), that 10^77 fits in uint256 but 10^78 does not, and
explicitly describe that the `if (scale > 67)` precision guard, the
`mulDidOverflow` check (rescaledIntValue / int256(scale) != signedCoefficient)
detects int256-level multiplication overflow, while the `mulDidTruncate` check
(int224(rescaledIntValue) != rescaledIntValue) detects values outside the int224
coefficient range; also adjust the `ParseDecimalPrecisionLoss` wording to
distinguish these two failure modes and reference `scale`, `signedCoefficient`,
`rescaledIntValue`, `mulDidOverflow`, and `mulDidTruncate`.
In `@audit/2026-03-10-01/pass5/LibDecimalFloat.md`:
- Around line 109-111: Fix the Markdown fence violations by adding a language
identifier (solidity) to the three code fences that contain NatSpec snippets
(the blocks showing "/// Subtract float a from float b.", the pow10 NatSpec, and
the log10 NatSpec) and ensure there is a blank line before each fenced block;
locate the NatSpec comment blocks (look for the literal "/// Subtract float a
from float b." and the `pow10` and `log10` NatSpec descriptions) and change
their triple-backtick fences to start with ```solidity and insert an empty line
immediately before each fenced block so spacing follows MD031/MD040 rules.
In `@audit/2026-03-10-01/pass5/LibDecimalFloatImplementation.md`:
- Around line 102-108: The markdown in LibDecimalFloatImplementation.md has
fenced code blocks that lack surrounding blank lines (MD031); edit the file
around the "Verification:" section and any other places using triple-backtick or
fenced blocks to ensure there is an empty line before and after each fenced code
block so they render correctly—add a blank line above the opening ``` and below
the closing ``` for each block.
In `@src/lib/LibDecimalFloat.sol`:
- Around line 291-299: Update the NatSpec comment for the function packLossy so
it does not assert data loss as certain; change wording like "so there will be
data loss" to "so the operation may be lossy" (or similar) to reflect that
packLossy can return lossless == true and to keep the API invariant used by
packLossless consistent; ensure the summary mentions it "may be lossy" and
retains references to signedCoefficient and exponent and the returned values
(float, lossless).
In `@src/lib/table/LibLogTable.sol`:
- Around line 30-35: Replace the hardcoded size literals used in the various
toBytes overloads with the newly declared constants: use
ALT_SMALL_LOG_TABLE_SIZE_BYTES instead of 100 (occurrences used for the alt
small log table), ANTILOG_TABLE_SIZE_BYTES instead of 2000 (occurrences used for
the antilog table), and replace the 1000 literal with the appropriate constant
or computed value (if it represents a table size derive or declare a new
constant) inside the toBytes functions; update the toBytes(...) overload
implementations so they reference ALT_SMALL_LOG_TABLE_SIZE_BYTES and
ANTILOG_TABLE_SIZE_BYTES (and add a new named constant if the 1000 value maps to
a distinct table) to eliminate magic numbers and improve maintainability,
ensuring function names toBytes(...) are the exact locations to modify.
In `@test/src/lib/implementation/LibDecimalFloatImplementation.internals.t.sol`:
- Around line 98-103: The comments in the test
testCompareRescaleDifferentExponents are inconsistent about the rescaled values:
update the inline comments around the call to
LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3) so they consistently
describe the same rescaling (either "5e2 vs 3e3 => rescale to compare: 500 vs
3000" or "rescale mantissas to same exponent: 5 vs 30" with matching explanatory
text), and ensure the follow-up comment before assertTrue(a < b) matches that
description; adjust only the comments (no code changes) and reference
compareRescale and testCompareRescaleDifferentExponents to locate the lines.
---
Outside diff comments:
In `@src/lib/parse/LibParseDecimalFloat.sol`:
- Around line 198-202: The code currently maps every packLossy non-lossless case
to ParseDecimalPrecisionLoss, which incorrectly treats exponent underflow (which
packLossy represents as FLOAT_ZERO with lossless==false) as a parse failure;
change the branch after LibDecimalFloat.packLossy(signedCoefficient, exponent)
to only return ParseDecimalPrecisionLoss when lossless==false AND the returned
result is not the rounded-zero (i.e., result != Float.wrap(0)), otherwise return
the rounded-zero result; also update the related test expectation in
LibParseDecimalFloat.t.sol to expect successful rounding-to-zero for negative
exponent underflow cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae0e40cc-99f1-4dde-af47-3b50818cbf9a
📒 Files selected for processing (52)
.gitignoreCLAUDE.mdaudit/2026-03-10-01/pass0/process.mdaudit/2026-03-10-01/pass1/DecimalFloat.mdaudit/2026-03-10-01/pass1/LibDecimalFloat.mdaudit/2026-03-10-01/pass1/LibDecimalFloatDeploy.mdaudit/2026-03-10-01/pass1/LibDecimalFloatImplementation.mdaudit/2026-03-10-01/pass1/LibFormatDecimalFloat.mdaudit/2026-03-10-01/pass1/LibLogTable.mdaudit/2026-03-10-01/pass1/LibParseDecimalFloat.mdaudit/2026-03-10-01/pass1/LogTablesPointers.mdaudit/2026-03-10-01/pass1/errors.mdaudit/2026-03-10-01/pass1/scripts.mdaudit/2026-03-10-01/pass2/DecimalFloat.mdaudit/2026-03-10-01/pass2/LibDecimalFloat.mdaudit/2026-03-10-01/pass2/LibDecimalFloatImplementation.mdaudit/2026-03-10-01/pass2/LibFormatDecimalFloat.mdaudit/2026-03-10-01/pass2/errors_gen_deploy.mdaudit/2026-03-10-01/pass2/parse_and_tables.mdaudit/2026-03-10-01/pass3/DecimalFloat.mdaudit/2026-03-10-01/pass3/LibDecimalFloat.mdaudit/2026-03-10-01/pass3/impl_format_parse.mdaudit/2026-03-10-01/pass3/misc.mdaudit/2026-03-10-01/pass4/LibDecimalFloat.mdaudit/2026-03-10-01/pass4/concrete_errors_deploy.mdaudit/2026-03-10-01/pass4/impl_format_parse_table.mdaudit/2026-03-10-01/pass5/LibDecimalFloat.mdaudit/2026-03-10-01/pass5/LibDecimalFloatImplementation.mdaudit/2026-03-10-01/pass5/concrete_errors_deploy.mdaudit/2026-03-10-01/pass5/format_parse_table.mdaudit/2026-03-10-01/triage.mdscript/Deploy.solsrc/concrete/DecimalFloat.solsrc/error/ErrDecimalFloat.solsrc/lib/LibDecimalFloat.solsrc/lib/deploy/LibDecimalFloatDeploy.solsrc/lib/format/LibFormatDecimalFloat.solsrc/lib/implementation/LibDecimalFloatImplementation.solsrc/lib/parse/LibParseDecimalFloat.solsrc/lib/table/LibLogTable.soltest/src/concrete/DecimalFloat.format.t.soltest/src/concrete/DecimalFloat.log10.t.soltest/src/concrete/DecimalFloat.pow10.t.soltest/src/lib/LibDecimalFloat.pack.t.soltest/src/lib/deploy/LibDecimalFloatDeploy.t.soltest/src/lib/format/LibFormatDecimalFloat.countSigFigs.t.soltest/src/lib/format/LibFormatDecimalFloat.toDecimalString.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.internals.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.log10.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.minus.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.unabsUnsignedMulOrDivLossy.t.soltest/src/lib/parse/LibParseDecimalFloat.t.sol
💤 Files with no reviewable changes (1)
- test/src/lib/format/LibFormatDecimalFloat.countSigFigs.t.sol
|
|
||
| ### Functions with COMMENTED-OUT test (effectively untested at the concrete-contract level): | ||
| - `log10(Float)` -- `DecimalFloat.log10.t.sol` has the entire `testLog10Deployed` body commented out (lines 15-26) | ||
| - `pow10(Float)` -- `DecimalFloat.pow10.t.sol` has the entire `testPow10Deployed` body commented out (lines 15-26) |
Contributor
There was a problem hiding this comment.
Stale pow10 coverage finding should be updated to resolved/historical.
Line 133 and A01-5 still state pow10(Float) deployed-parity is commented out, but test/src/concrete/DecimalFloat.pow10.t.sol now has an active testPow10Deployed (Line 16-Line 27). Please mark this as resolved or explicitly label it as a pass-2 historical snapshot.
Also applies to: 181-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass2/DecimalFloat.md` at line 133, Update the audit note
that claims "pow10(Float) -- DecimalFloat.pow10.t.sol has the entire
testPow10Deployed body commented out" to reflect current state: remove the stale
claim and mark the finding as resolved or as a pass-2 historical snapshot
because testPow10Deployed in DecimalFloat.pow10.t.sol is active; also make the
same update for the corresponding duplicate entry later in the document (the
other pow10/Deployed parity note).
Comment on lines
+44
to
+51
| | 15 | `maximize` | 957 | Yes | | ||
| | 16 | `maximizeFull` | 1011 | Indirect (via maximize tests) | | ||
| | 17 | `compareRescale` | 1047 | No | | ||
| | 18 | `withTargetExponent` | 1127 | Yes | | ||
| | 19 | `intFrac` | 1169 | Yes | | ||
| | 20 | `mantissa4` | 1197 | No | | ||
| | 21 | `lookupAntilogTableY1Y2` | 1227 | No | | ||
| | 22 | `unitLinearInterpolation` | 1267 | No | |
Contributor
There was a problem hiding this comment.
Address markdownlint heading/table spacing warnings.
Headings and adjacent tables around Line 44/45/50/51 (plus heading-spacing around Line 95/131/135) need blank lines to satisfy MD022/MD058.
💡 Suggested markdown fix
### Error Paths
+
| Error | Used In | Tested? |
|-------|---------|---------|
...
### Test File Contents Summary
+
...
---
### A09-13: `log10` error paths (`Log10Zero`, `Log10Negative`) completely untested (LOW)Also applies to: 95-95, 131-131, 135-135
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 50-50: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass2/LibDecimalFloatImplementation.md` around lines 44 -
51, The Markdown has missing blank lines between headings and their adjacent
tables/lines causing MD022/MD058; edit the markdown around the listed table rows
(entries referencing `maximize`, `maximizeFull`, `compareRescale`,
`withTargetExponent`, `intFrac`, `mantissa4`, `lookupAntilogTableY1Y2`,
`unitLinearInterpolation`) and the other indicated heading locations (around
lines noted for 95, 131, 135) to insert a single blank line before and after
each heading and before the table so that every heading is separated from the
table/content by a blank line, ensuring heading spacing rules are satisfied.
Comment on lines
+44
to
+51
|
|
||
| ### Test: `DecimalFloat.format.t.sol` (43 lines) | ||
|
|
||
| | Test Function | Lines | Coverage | | ||
| |---------------|-------|----------| | ||
| | `testFormatDeployed` (fuzz) | 18-31 | Fuzz: arbitrary Float + scientific bounds. Compares library call vs deployed contract call. Error parity checked via try/catch. | | ||
| | `testFormatConstants` | 33-42 | Verifies FORMAT_DEFAULT_SCIENTIFIC_MIN == (1, -4) and FORMAT_DEFAULT_SCIENTIFIC_MAX == (1, 9). | | ||
|
|
Contributor
There was a problem hiding this comment.
Fix markdownlint spacing around headings and tables.
Line 44/50 headings and Line 45/51 tables are missing required surrounding blank lines (MD022, MD058), and Line 95 has the same heading-spacing issue.
💡 Suggested markdown fix
-### Test: `DecimalFloat.format.t.sol` (43 lines)
-| Test Function | Lines | Coverage |
+### Test: `DecimalFloat.format.t.sol` (43 lines)
+
+| Test Function | Lines | Coverage |
|---------------|-------|----------|
| `testFormatDeployed` (fuzz) | 18-31 | Fuzz: arbitrary Float + scientific bounds. Compares library call vs deployed contract call. Error parity checked via try/catch. |
| `testFormatConstants` | 33-42 | Verifies FORMAT_DEFAULT_SCIENTIFIC_MIN == (1, -4) and FORMAT_DEFAULT_SCIENTIFIC_MAX == (1, 9). |
## Coverage Analysis
### Lines/Branches Covered
| Source Line(s) | Branch/Path | Test Coverage |Also applies to: 95-95
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 50-50: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass2/LibFormatDecimalFloat.md` around lines 44 - 51, The
markdown is missing required blank lines around headings and tables
(MD022/MD058) — locate the "### Test: `DecimalFloat.format.t.sol`" heading and
the following table block and ensure there is an empty line before the heading
and an empty line after the table; apply the same fix to the later heading
referenced at line 95 by inserting a blank line before and after that heading as
well so every heading and table is surrounded by a single blank line.
| - **Library:** `LibLogTable` (line 35) | ||
| - **Functions (10):** 5 `toBytes` overloads (lines 41, 75, 109, 142, 175), `logTableDec` (line 206), `logTableDecSmall` (line 414), `logTableDecSmallAlt` (line 512), `antiLogTableDec` (line 530), `antiLogTableDecSmall` (line 638) | ||
| - **Magic numbers in `toBytes` overloads:** `1000` (lines 113, 132 -- `uint8[10][100]` table size), `100` (lines 146, 165 -- `uint8[10][10]` table size), `2000` (lines 179, 198 -- `uint16[10][100]` table size). The `uint16[10][90]` and `uint8[10][90]` overloads use the named constants `LOG_TABLE_SIZE_BYTES` and `LOG_TABLE_SIZE_BASE` respectively, but the other three overloads inline the table sizes. | ||
| - **Typo:** "copmiled" on line 203 (already noted in pass 3 as A11-1) |
Contributor
There was a problem hiding this comment.
Fix typo in the audit note.
Line 50 has “copmiled”; should be “compiled”.
💡 Suggested fix
-- **Typo:** "copmiled" on line 203 (already noted in pass 3 as A11-1)
+- **Typo:** "compiled" on line 203 (already noted in pass 3 as A11-1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Typo:** "copmiled" on line 203 (already noted in pass 3 as A11-1) | |
| - **Typo:** "compiled" on line 203 (already noted in pass 3 as A11-1) |
🧰 Tools
🪛 LanguageTool
[grammar] ~50-~50: Ensure spelling is correct
Context: ...ads inline the table sizes. - Typo: "copmiled" on line 203 (already noted in pass 3 as...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass4/impl_format_parse_table.md` at line 50, Fix the
typo "copmiled" to "compiled" in the audit note text within
impl_format_parse_table.md (the occurrence referenced as A11-1 / the audit note
on pass 4); update that single word so the sentence reads correctly while
leaving surrounding wording unchanged.
| | LOW | 2 | A06-16, A06-20 | | ||
| | INFO | 5 | A06-15, A06-17, A06-18, A06-19, A06-21 | | ||
|
|
||
| The file is clean from a code quality perspective. There are no bare `src/` imports, no commented-out code, no dead code, and no unused imports. The two LOW findings are: (1) the `inv` function's unique `(lossless);` dead expression pattern that diverges from the 13 other call sites, and (2) the `pow` function's direct use of implementation internals breaking the otherwise consistent abstraction layer, including a missing forge-lint annotation. The INFO findings are cosmetic: inconsistent slither comment spacing, inconsistent local variable naming, two minor typos, and a constant that lives in the library despite being unused within it. |
Contributor
There was a problem hiding this comment.
Resolve summary contradiction on dead code.
Line 214 states “no dead code,” but earlier (Line 122) the report explicitly identifies a dead expression statement. Please align the summary wording.
💡 Suggested wording update
-The file is clean from a code quality perspective. There are no bare `src/` imports, no commented-out code, no dead code, and no unused imports.
+The file is clean from a code quality perspective. There are no bare `src/` imports, no commented-out code, and no unused imports.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The file is clean from a code quality perspective. There are no bare `src/` imports, no commented-out code, no dead code, and no unused imports. The two LOW findings are: (1) the `inv` function's unique `(lossless);` dead expression pattern that diverges from the 13 other call sites, and (2) the `pow` function's direct use of implementation internals breaking the otherwise consistent abstraction layer, including a missing forge-lint annotation. The INFO findings are cosmetic: inconsistent slither comment spacing, inconsistent local variable naming, two minor typos, and a constant that lives in the library despite being unused within it. | |
| The file is clean from a code quality perspective. There are no bare `src/` imports, no commented-out code, and no unused imports. The two LOW findings are: (1) the `inv` function's unique `(lossless);` dead expression pattern that diverges from the 13 other call sites, and (2) the `pow` function's direct use of implementation internals breaking the otherwise consistent abstraction layer, including a missing forge-lint annotation. The INFO findings are cosmetic: inconsistent slither comment spacing, inconsistent local variable naming, two minor typos, and a constant that lives in the library despite being unused within it. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass4/LibDecimalFloat.md` at line 214, Update the summary
sentence that currently claims “no dead code” to acknowledge the earlier finding
of a single dead expression statement in the inv function: change the wording to
something like “no other dead code aside from a single dead expression statement
in inv” (or similar concise phrasing) so the top-level summary no longer
contradicts the specific LOW finding about inv; also ensure the summary uses the
same terminology (“dead expression statement” / “dead code”) as the detailed
finding for consistency.
Comment on lines
+109
to
+111
| ``` | ||
| /// Subtract float a from float b. | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdownlint fence violations (language + blank-line spacing).
The fenced blocks at Line 109, Line 151, and Line 157 should include a language (MD040), and the latter two need proper blank-line separation (MD031).
🧹 Proposed markdown fix
-```
+```solidity
/// Subtract float a from float b.@@
-The pow10 NatSpec says:
- +The `pow10` NatSpec says: + +solidity
/// Same as power10, but accepts a Float struct instead of separate values.
@@
-The `log10` NatSpec says:
-```
+The `log10` NatSpec says:
+
+```solidity
/// Same as log10, but accepts a Float struct instead of separate values.
</details>
Also applies to: 151-153, 157-159
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @audit/2026-03-10-01/pass5/LibDecimalFloat.md around lines 109 - 111, Fix the
Markdown fence violations by adding a language identifier (solidity) to the
three code fences that contain NatSpec snippets (the blocks showing "///
Subtract float a from float b.", the pow10 NatSpec, and the log10 NatSpec) and
ensure there is a blank line before each fenced block; locate the NatSpec
comment blocks (look for the literal "/// Subtract float a from float b." and
the pow10 and log10 NatSpec descriptions) and change their triple-backtick
fences to start with ```solidity and insert an empty line immediately before
each fenced block so spacing follows MD031/MD040 rules.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+102
to
+108
| **Verification:** | ||
| - Uses the standard Chinese Remainder Theorem approach from OpenZeppelin. | ||
| - `mulmod(a, b, not(0))` computes `(a*b) mod (2^256 - 1)`. | ||
| - `mul(a, b)` computes `(a*b) mod 2^256`. | ||
| - `high = mm - low - borrow` reconstructs the high word. | ||
| - This is a well-audited algorithm identical to OpenZeppelin's Math.sol. | ||
| - Result: **CORRECT** |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor markdown formatting: Add blank lines around fenced code blocks.
Static analysis indicates fenced code blocks should be surrounded by blank lines for proper markdown rendering (MD031).
Proposed fix
**Verification:**
- Uses the standard Chinese Remainder Theorem approach from OpenZeppelin.
+
- `mulmod(a, b, not(0))` computes `(a*b) mod (2^256 - 1)`.🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 104-104: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audit/2026-03-10-01/pass5/LibDecimalFloatImplementation.md` around lines 102
- 108, The markdown in LibDecimalFloatImplementation.md has fenced code blocks
that lack surrounding blank lines (MD031); edit the file around the
"Verification:" section and any other places using triple-backtick or fenced
blocks to ensure there is an empty line before and after each fenced code block
so they render correctly—add a blank line above the opening ``` and below the
closing ``` for each block.
Comment on lines
+291
to
+299
| /// Pack a signed coefficient and exponent into a single `Float`. | ||
| /// Clearly this involves fitting 64 bytes into 32 bytes, so there will be | ||
| /// data loss. | ||
| /// @param signedCoefficient The signed coefficient of the floating point | ||
| /// representation. | ||
| /// @param exponent The exponent of the floating point representation. | ||
| /// @return float The packed representation of the signed coefficient and | ||
| /// exponent. | ||
| /// @return lossless True if the conversion was lossless, false otherwise. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reword the packLossy summary to match the API.
This block still says packing “will” lose data, but the function can return lossless == true and packLossless depends on that invariant. Rephrase the summary to “may be lossy” so the NatSpec is internally consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/LibDecimalFloat.sol` around lines 291 - 299, Update the NatSpec
comment for the function packLossy so it does not assert data loss as certain;
change wording like "so there will be data loss" to "so the operation may be
lossy" (or similar) to reflect that packLossy can return lossless == true and to
keep the API invariant used by packLossless consistent; ensure the summary
mentions it "may be lossy" and retains references to signedCoefficient and
exponent and the returned values (float, lossless).
Comment on lines
+30
to
+35
| /// @dev The size in bytes of the alt small log table (uint8[10][10] = 100). | ||
| uint256 constant ALT_SMALL_LOG_TABLE_SIZE_BYTES = 100; | ||
|
|
||
| /// @dev The size in bytes of the antilog table (uint16[10][100] = 2000). | ||
| uint256 constant ANTILOG_TABLE_SIZE_BYTES = 2000; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using these constants in the toBytes functions within this file.
The new constants ALT_SMALL_LOG_TABLE_SIZE_BYTES (100) and ANTILOG_TABLE_SIZE_BYTES (2000) are well-documented. However, the toBytes overloads in this same file still use hardcoded magic numbers:
- Line 119, 138:
1000(different table, but same pattern) - Line 152, 171:
100(should useALT_SMALL_LOG_TABLE_SIZE_BYTES) - Line 185, 204:
2000(should useANTILOG_TABLE_SIZE_BYTES)
For consistency and maintainability, consider replacing these magic numbers with the corresponding constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/table/LibLogTable.sol` around lines 30 - 35, Replace the hardcoded
size literals used in the various toBytes overloads with the newly declared
constants: use ALT_SMALL_LOG_TABLE_SIZE_BYTES instead of 100 (occurrences used
for the alt small log table), ANTILOG_TABLE_SIZE_BYTES instead of 2000
(occurrences used for the antilog table), and replace the 1000 literal with the
appropriate constant or computed value (if it represents a table size derive or
declare a new constant) inside the toBytes functions; update the toBytes(...)
overload implementations so they reference ALT_SMALL_LOG_TABLE_SIZE_BYTES and
ANTILOG_TABLE_SIZE_BYTES (and add a new named constant if the 1000 value maps to
a distinct table) to eliminate magic numbers and improve maintainability,
ensuring function names toBytes(...) are the exact locations to modify.
Comment on lines
+98
to
+103
| function testCompareRescaleDifferentExponents() external pure { | ||
| // 5e2 vs 3e3 => rescale to compare: 5 vs 30 | ||
| (int256 a, int256 b) = LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3); | ||
| // After rescaling, a < b should hold (500 < 3000) | ||
| assertTrue(a < b); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor comment inconsistency.
The inline comment on line 99 says "rescale to compare: 5 vs 30" (implying exponent 2), but line 101 says "500 < 3000" (implying exponent 0). While the test assertion a < b is correct regardless of the internal rescaling, the comments are slightly confusing.
Consider updating for clarity:
📝 Suggested comment fix
function testCompareRescaleDifferentExponents() external pure {
- // 5e2 vs 3e3 => rescale to compare: 5 vs 30
+ // 5e2 vs 3e3 => 500 vs 3000
(int256 a, int256 b) = LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3);
- // After rescaling, a < b should hold (500 < 3000)
+ // After rescaling to common exponent, a < b should hold
assertTrue(a < b);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function testCompareRescaleDifferentExponents() external pure { | |
| // 5e2 vs 3e3 => rescale to compare: 5 vs 30 | |
| (int256 a, int256 b) = LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3); | |
| // After rescaling, a < b should hold (500 < 3000) | |
| assertTrue(a < b); | |
| } | |
| function testCompareRescaleDifferentExponents() external pure { | |
| // 5e2 vs 3e3 => 500 vs 3000 | |
| (int256 a, int256 b) = LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3); | |
| // After rescaling to common exponent, a < b should hold | |
| assertTrue(a < b); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/lib/implementation/LibDecimalFloatImplementation.internals.t.sol`
around lines 98 - 103, The comments in the test
testCompareRescaleDifferentExponents are inconsistent about the rescaled values:
update the inline comments around the call to
LibDecimalFloatImplementation.compareRescale(5, 2, 3, 3) so they consistently
describe the same rescaling (either "5e2 vs 3e3 => rescale to compare: 500 vs
3000" or "rescale mantissas to same exponent: 5 vs 30" with matching explanatory
text), and ensure the follow-up comment before assertTrue(a < b) matches that
description; adjust only the comments (no code changes) and reference
compareRescale and testCompareRescaleDifferentExponents to locate the lines.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
DecimalFloatcontract exposing decimal-float arithmetic and formatting operations.Bug Fixes
Documentation