Conversation
WalkthroughReplaces characteristic/mantissa with integer/fraction (intFrac) across core libs; adds deterministic Zoltu deployment integration and WriteError; adds log/antilog table serialization and formatting utilities; normalizes zero parsing; broadens tests and CI fork RPC env vars. Changes
Sequence Diagram(s)sequenceDiagram
actor Test as Forge Test
participant Fork as Forked RPC
participant Deployer as LibDecimalFloatDeploy
participant Zoltu as Zoltu Proxy
participant DF as DecimalFloat
Test->>Fork: fork network (CI_FORK_*)
Test->>Deployer: call decimalFloatZoltu()
Deployer->>Zoltu: deterministic deploy request (proxy)
Zoltu-->>Deployer: (success, deployedAddress) or (failure)
alt success and deployedAddress != 0
Deployer->>Test: return deployedAddress
Test->>DF: deploy direct DecimalFloat()
Test->>Test: compare codehash(zoltu) == codehash(direct)
else failure or deployedAddress == 0
Deployer-->>Test: revert with WriteError()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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
🤖 Fix all issues with AI agents
In `@lib/rain.string`:
- Line 1: The PR's submodule update includes functional changes (new errors and
modified parsing/conformance) that may be breaking; inspect the new/changed
symbols ErrConform (new error types), ErrParse, LibConformString (conformance
logic), LibParseCMask and LibParseDecimal (parsing logic), and the added tests
LibConformString.t.sol to determine behavioral changes. Produce a short
compatibility report listing: (1) any changed public/ABI-facing function
signatures or revert/error types that callers must handle, (2) changes in
parsing/conformance semantics callers rely on, and (3) recommended parent-repo
updates (error handling code paths, tests, docs, and dependency/version bump).
If breaking, add concrete migration steps (what to change where in parent code)
and a suggested version bump. Ensure the report references the exact symbols
above so maintainers can quickly locate impacted code.
| @@ -1 +1 @@ | |||
| Subproject commit 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28 | |||
| Subproject commit 488f237cd59874e4eb91b5a4f747bd57578fec7f | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Navigate to the submodule and examine changes
cd lib/rain.string 2>/dev/null || exit 1
echo "=== Checking if we can access the submodule ==="
git rev-parse HEAD
echo -e "\n=== Commit log between old and new commits ==="
git log --oneline --no-merges 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28..488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1 | head -20
echo -e "\n=== Detailed stats ==="
git diff --stat 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28..488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1
echo -e "\n=== Commit messages (security/audit keywords) ==="
git log --format="%H %s" 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28..488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1Repository: rainlanguage/rain.math.float
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Check submodule configuration and current state
cd lib/rain.string 2>/dev/null || exit 1
echo "=== Current submodule state ==="
pwd
git rev-parse HEAD
git log --oneline -5
echo -e "\n=== Check if new commit exists ==="
git cat-file -t 488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1 || echo "New commit not found locally"
echo -e "\n=== Submodule remote configuration ==="
git remote -v
echo -e "\n=== Check git reflog for history ==="
git reflog | head -10
echo -e "\n=== Try fetching new commit ==="
git fetch origin 488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1 | head -5Repository: rainlanguage/rain.math.float
Length of output: 1028
🏁 Script executed:
#!/bin/bash
# Check parent repo .gitmodules configuration
cd /home/jailuser/git
echo "=== Checking .gitmodules ==="
cat .gitmodules 2>/dev/null || echo ".gitmodules not found"
echo -e "\n=== List all submodule paths ==="
git config --file=.gitmodules --name-only --get-regexp path
echo -e "\n=== Check lib/rain.string submodule config ==="
git config --file=.gitmodules --get-regexp "submodule.lib/rain.string"
echo -e "\n=== Current submodule status ==="
git submodule statusRepository: rainlanguage/rain.math.float
Length of output: 1395
🏁 Script executed:
#!/bin/bash
# Try to fetch from the correct rain.string repository
echo "=== Fetching from rain.string repository ==="
cd /tmp
git clone --bare https://github.com/rainlanguage/rain.string.git rain-string-bare 2>&1 | head -10
cd rain-string-bare
echo -e "\n=== Check if both commits exist ==="
git cat-file -t 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28 2>&1 || echo "Old commit not found"
git cat-file -t 488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1 || echo "New commit not found"
echo -e "\n=== Commit log between commits ==="
git log --oneline 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28..488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1
echo -e "\n=== Diff stats ==="
git diff --stat 0b1ca08aed6d9c06b83fe127a7d20ee7002ead28..488f237cd59874e4eb91b5a4f747bd57578fec7f 2>&1Repository: rainlanguage/rain.math.float
Length of output: 2015
Address actual code changes in the submodule update.
The submodule update contains significant functional changes beyond dependency updates, contrary to the AI summary. The new commit (488f237) includes an audit report from Protofire (Jan 2025) and modifies string parsing and conformance logic:
- Added error types in
ErrConform.solandErrParse.sol - Enhanced string conformance logic in
LibConformString.sol(+25 lines) - Modified parsing in
LibParseCMask.solandLibParseDecimal.sol - Added comprehensive conformance tests (+67 lines in
LibConformString.t.sol) - Updated dependencies and gas snapshots
Clarify whether these changes introduce any breaking changes or require compatibility updates in the parent repository, particularly around the new error handling and string parsing modifications.
🤖 Prompt for AI Agents
In `@lib/rain.string` at line 1, The PR's submodule update includes functional
changes (new errors and modified parsing/conformance) that may be breaking;
inspect the new/changed symbols ErrConform (new error types), ErrParse,
LibConformString (conformance logic), LibParseCMask and LibParseDecimal (parsing
logic), and the added tests LibConformString.t.sol to determine behavioral
changes. Produce a short compatibility report listing: (1) any changed
public/ABI-facing function signatures or revert/error types that callers must
handle, (2) changes in parsing/conformance semantics callers rely on, and (3)
recommended parent-repo updates (error handling code paths, tests, docs, and
dependency/version bump). If breaking, add concrete migration steps (what to
change where in parent code) and a suggested version bump. Ensure the report
references the exact symbols above so maintainers can quickly locate impacted
code.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/lib/LibDecimalFloat.floor.t.sol (1)
29-33: Test will fail for negative inputs when exponent < -77.The comment "floor is 0 or -1" is accurate, but the test unconditionally expects
(0, 0). For negativexwithexponent < -77, thefloorfunction should return-1(approximately-1e67, -67after normalization), not0.The examples in lines 78-85 demonstrate this pattern: negative values consistently produce non-zero floor results (e.g.,
checkFloor(-123456789, -9, -12345679e60, -60)), while the test bounds exponent to values outside this tested range and uses unconstrained fuzz input forx.Consider bounding
xto non-negative values:function testFloorLessThanMin(int224 x, int256 exponent) external pure { exponent = bound(exponent, type(int32).min, -77); + x = int224(bound(int256(x), 0, type(int224).max)); checkFloor(x, exponent, 0, 0); }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol`:
- Line 60: Fix the typo in the comment "/// xeX != yeY if x != y (assumiing
maximized representation)" by changing "assumiing" to "assuming" so it reads
"/// xeX != yeY if x != y (assuming maximized representation)"; update the
comment in the LibDecimalFloatImplementation.eq test to keep documentation
accurate.
In `@test/src/lib/LibDecimalFloat.floor.t.sol`:
- Around line 24-32: In checkFloorEq, the parameters x and exponent are shadowed
when unpacking the floor result, so logs show outputs instead of inputs; change
the unpacked variable names (e.g., floorX, floorExponent or outX, outExponent)
in the unpack assignment from a.floor().unpack(), update the console2.log calls
to log both original inputs (x, exponent) and the new unpacked output names, and
update the final equality check to pass the unpacked output variables (e.g.,
floorX, floorExponent) to LibDecimalFloatImplementation.eq so the test logs and
assertion clearly reference input vs. output.
test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/manual-sol-artifacts.yaml:
- Around line 75-76: The workflow currently forces CI_FORK_ETH_RPC_URL and
CI_FORK_FLARE_RPC_URL to an empty string via "|| ''", which passes empty values
into vm.createSelectFork() and makes Foundry tests fail; remove the "|| ''"
fallbacks from the CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL assignments so
the env vars are truly undefined when not set, or alternatively add a guard in
the Solidity tests (e.g., in LibDecimalFloatDeploy.t.sol around the
vm.createSelectFork() calls) to detect an empty/undefined env var and skip the
test instead; update either the .github/workflows/manual-sol-artifacts.yaml
lines containing CI_FORK_ETH_RPC_URL / CI_FORK_FLARE_RPC_URL or add the
conditional skip logic in the test functions that call vm.createSelectFork().
| CI_FORK_ETH_RPC_URL: ${{ secrets.RPC_URL_ETHEREUM_FORK || vars.RPC_URL_ETHEREUM_FORK || '' }} | ||
| CI_FORK_FLARE_RPC_URL: ${{ secrets.RPC_URL_FLARE_FORK || vars.RPC_URL_FLARE_FORK || '' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'CI_FORK_(ETH|FLARE)_RPC_URL' --type yaml --type ts --type js --type shRepository: rainlanguage/rain.math.float
Length of output: 54
🏁 Script executed:
# Search more broadly for these env vars
rg -n 'CI_FORK_ETH_RPC_URL|CI_FORK_FLARE_RPC_URL'Repository: rainlanguage/rain.math.float
Length of output: 289
🏁 Script executed:
# Also search for how RPC_URL is typically used in this repo
rg -n 'RPC.*URL' .github/workflows/ | head -30Repository: rainlanguage/rain.math.float
Length of output: 912
🏁 Script executed:
cat -n test/src/lib/deploy/LibDecimalFloatDeploy.t.solRepository: rainlanguage/rain.math.float
Length of output: 1687
Tests will fail when fork RPC URLs are not configured. The env vars are set unconditionally with || '' fallback, causing vm.createSelectFork() calls to receive empty strings when secrets/vars are undefined. The tests at test/src/lib/deploy/LibDecimalFloatDeploy.t.sol:11 and line 28 will fail since Foundry requires non-empty RPC URLs.
Either remove the || '' fallback and let tests skip gracefully, or add conditional logic in tests to skip when these env vars are empty.
🤖 Prompt for AI Agents
In @.github/workflows/manual-sol-artifacts.yaml around lines 75 - 76, The
workflow currently forces CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL to an
empty string via "|| ''", which passes empty values into vm.createSelectFork()
and makes Foundry tests fail; remove the "|| ''" fallbacks from the
CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL assignments so the env vars are
truly undefined when not set, or alternatively add a guard in the Solidity tests
(e.g., in LibDecimalFloatDeploy.t.sol around the vm.createSelectFork() calls) to
detect an empty/undefined env var and skip the test instead; update either the
.github/workflows/manual-sol-artifacts.yaml lines containing CI_FORK_ETH_RPC_URL
/ CI_FORK_FLARE_RPC_URL or add the conditional skip logic in the test functions
that call vm.createSelectFork().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/rainix.yaml:
- Around line 55-58: The workflow sets CI_FORK_ETH_RPC_URL and
CI_FORK_FLARE_RPC_URL to an empty string via the "|| ''" fallback which causes
vm.envString() to return an empty value and later makes vm.createSelectFork("")
fail; remove the "|| ''" fallback so the env vars are left unset when
secrets/vars are absent (or add a conditional step that unsets the variable when
its resolved value is empty) — update the two lines that assign
CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL in the workflow to omit the
empty-string fallback so tests preserve the "var not found" behavior used by
vm.envString()/vm.createSelectFork.
| env: | ||
| ETH_RPC_URL: ${{ secrets.CI_DEPLOY_SEPOLIA_RPC_URL || vars.CI_DEPLOY_SEPOLIA_RPC_URL }} | ||
| CI_FORK_ETH_RPC_URL: ${{ secrets.RPC_URL_ETHEREUM_FORK || vars.RPC_URL_ETHEREUM_FORK || '' }} | ||
| CI_FORK_FLARE_RPC_URL: ${{ secrets.RPC_URL_FLARE_FORK || vars.RPC_URL_FLARE_FORK || '' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "rainix.yaml" -type f 2>/dev/null | head -5Repository: rainlanguage/rain.math.float
Length of output: 54
🏁 Script executed:
find . -path "*/.github/workflows/rainix.yaml" -type fRepository: rainlanguage/rain.math.float
Length of output: 103
🏁 Script executed:
git ls-files | grep -E "(rainix|workflows)" | head -20Repository: rainlanguage/rain.math.float
Length of output: 258
🏁 Script executed:
cat -n .github/workflows/rainix.yaml | sed -n '45,70p'Repository: rainlanguage/rain.math.float
Length of output: 1583
🏁 Script executed:
rg -n "CI_FORK_ETH_RPC_URL|CI_FORK_FLARE_RPC_URL" --type bash --type shell --type nix --type rust --type ts --type jsRepository: rainlanguage/rain.math.float
Length of output: 100
🏁 Script executed:
rg -rn "CI_FORK_ETH_RPC_URL|CI_FORK_FLARE_RPC_URL"Repository: rainlanguage/rain.math.float
Length of output: 245
🏁 Script executed:
fd -e nix | head -20Repository: rainlanguage/rain.math.float
Length of output: 81
🏁 Script executed:
git ls-files | grep -E "\.(nix|rs|ts|js|sh)$" | grep -v node_modules | head -30Repository: rainlanguage/rain.math.float
Length of output: 243
🏁 Script executed:
rg "CI_FORK" -A 2 -B 2Repository: rainlanguage/rain.math.float
Length of output: 1204
🏁 Script executed:
cat -n flake.nix | head -100Repository: rainlanguage/rain.math.float
Length of output: 1520
🏁 Script executed:
git ls-files | grep -E "test|spec" | grep -E "\.(sol|ts|js|sh|nix)$" | head -20Repository: rainlanguage/rain.math.float
Length of output: 930
🏁 Script executed:
cat -n test/src/lib/deploy/LibDecimalFloatDeploy.t.solRepository: rainlanguage/rain.math.float
Length of output: 1687
🏁 Script executed:
rg "vm.envString|vm.envOpt" -A 2 -B 2Repository: rainlanguage/rain.math.float
Length of output: 940
🏁 Script executed:
cat -n test/src/lib/deploy/LibDecimalFloatDeploy.t.sol | sed -n '10,36p'Repository: rainlanguage/rain.math.float
Length of output: 1284
🌐 Web query:
Forge vm.envString() behavior empty string vs unset environment variable
💡 Result:
In Foundry/Forge cheatcodes, vm.envString("KEY") distinguishes unset vs set-to-empty like this:
- Unset / not defined env var →
vm.envString(key)reverts with “environment variable not found”. [1] - Defined but empty (e.g.
KEY=in.env, orvm.setEnv("KEY", "")) →vm.envString(key)returns the empty string ("") and does not revert (empty values are allowed bysetEnv; only NUL is disallowed). [2]
If you want “unset → default” (instead of reverting), use vm.envOr(...)-style helpers (e.g., envOr) rather than envString. [3]
Sources: [1] [2] [3]
Guard against empty fork RPC URLs to prevent failures with empty strings.
Tests use vm.envString() which reverts if unset but returns empty string if set-to-empty. With the || '' fallback, CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL become set-to-empty, causing vm.createSelectFork("") to fail with a confusing error instead of reverting early. Unset empty values to preserve the "var not found" semantics and allow tests to skip gracefully.
🔧 Suggested guard to unset empty values before running
- run: nix develop -c ${{ matrix.task }}
+ run: |
+ if [ -z "$CI_FORK_ETH_RPC_URL" ]; then unset CI_FORK_ETH_RPC_URL; fi
+ if [ -z "$CI_FORK_FLARE_RPC_URL" ]; then unset CI_FORK_FLARE_RPC_URL; fi
+ nix develop -c ${{ matrix.task }}🤖 Prompt for AI Agents
In @.github/workflows/rainix.yaml around lines 55 - 58, The workflow sets
CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL to an empty string via the "|| ''"
fallback which causes vm.envString() to return an empty value and later makes
vm.createSelectFork("") fail; remove the "|| ''" fallback so the env vars are
left unset when secrets/vars are absent (or add a conditional step that unsets
the variable when its resolved value is empty) — update the two lines that
assign CI_FORK_ETH_RPC_URL and CI_FORK_FLARE_RPC_URL in the workflow to omit the
empty-string fallback so tests preserve the "var not found" behavior used by
vm.envString()/vm.createSelectFork.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/lib/parse/LibParseDecimalFloat.t.sol (1)
52-67:⚠️ Potential issue | 🟠 MajorGuard exponent overflow before calling packLossy.
packLossy reverts on positive exponent overflow regardless of coefficient size; the current guard only covers the int224‑fit case, so fuzz cases with large coefficients can still revert inside the test.🩹 Proposed fix
- } else if (exponent != int32(exponent) && exponent > 0 && signedCoefficient == int224(signedCoefficient)) { + } else if (exponent != int32(exponent) && exponent > 0) { vm.expectRevert(abi.encodeWithSelector(ExponentOverflow.selector, signedCoefficient, exponent)); signedCoefficient = 0; exponent = 0;
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/LibDecimalFloat.sol (1)
510-512: 🧹 Nitpick | 🔵 TrivialInconsistent unused variable handling.
The
invfunction uses(lossless);to suppress the unused variable warning, while other functions in this file use the cleaner discard pattern(Float result,) = packLossy(...). Consider aligning with the pattern used elsewhere for consistency.♻️ Suggested refactor for consistency
function inv(Float float) internal pure returns (Float) { (int256 signedCoefficient, int256 exponent) = float.unpack(); (signedCoefficient, exponent) = LibDecimalFloatImplementation.inv(signedCoefficient, exponent); - (Float result, bool lossless) = packLossy(signedCoefficient, exponent); - (lossless); + (Float result,) = packLossy(signedCoefficient, exponent); return result; }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/LibDecimalFloat.sol (1)
785-794: 🧹 Nitpick | 🔵 TrivialRename
atofloatfor parameter-name consistency.This keeps the established naming convention for
Floatparameters.♻️ Proposed rename
-function isZero(Float a) internal pure returns (bool result) { +function isZero(Float float) internal pure returns (bool result) { uint256 mask = type(uint224).max; assembly ("memory-safe") { // Don't need to signextend here because we only care if the value // is zero or not. - result := iszero(and(a, mask)) + result := iszero(and(float, mask)) } }Based on learnings: "In the rain.math.float codebase, there's an established naming convention where functions accepting a
Floattype parameter consistently usefloatas the parameter name, even though it shadows the type name. This pattern is used throughoutLibDecimalFloat.soland should be maintained for consistency in related contracts likeDecimalFloat.sol."
…float into 2026-01-28-audit
|
@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:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=L 🧠 Learnings used |
2 participants
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit