update forge #162
update forge #162
Conversation
WalkthroughReplaces characteristic/mantissa with intFrac across implementation and callers; adds Zoltu deterministic-deploy integration and WriteError; normalizes zero parsing and returns precision-loss on lossy full-consume parses; advances several submodules, updates .gitignore, and expands tests (intFrac, floor/ceil, parse, deploy). Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Forge Test
participant Fork as Forked RPC
participant Deployer as LibDecimalFloatDeploy
participant Zoltu as Zoltu Proxy
participant DF as DecimalFloat Contract
Test->>Fork: fork RPC (CI/*_FORK_*)
Test->>Deployer: call decimalFloatZoltu()
Deployer->>Zoltu: request deterministic deploy via 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 issues
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://d.iamgay.cn/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.
| } | ||
|
|
||
| /// xeX != yeY if x != y | ||
| /// xeX != yeY if x != y (assumiing maximized representation) |
There was a problem hiding this comment.
Fix typo: "assumiing" → "assuming".
📝 Proposed fix
- /// xeX != yeY if x != y (assumiing maximized representation)
+ /// xeX != yeY if x != y (assuming maximized representation)📝 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.
| /// xeX != yeY if x != y (assumiing maximized representation) | |
| /// xeX != yeY if x != y (assuming maximized representation) |
🤖 Prompt for AI Agents
In `@test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol` at 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.
| function checkFloorEq(int256 x, int256 exponent, int256 expectedFrac, int256 expectedFracExponent) internal pure { | ||
| Float a = LibDecimalFloat.packLossless(x, exponent); | ||
| (x, exponent) = a.floor().unpack(); | ||
| console2.log("x", x); | ||
| console2.log("exponent", exponent); | ||
| console2.log("expectedFrac", expectedFrac); | ||
| console2.log("expectedFracExponent", expectedFracExponent); | ||
| assertTrue((LibDecimalFloatImplementation.eq(x, exponent, expectedFrac, expectedFracExponent))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parameter shadowing reduces debuggability.
The variables x and exponent are reused after unpacking, so the logged values show the output of floor(), not the input. If a test fails, the original input values aren't visible in the logs.
Consider renaming the unpacked variables:
♻️ Suggested improvement for clarity
function checkFloorEq(int256 x, int256 exponent, int256 expectedFrac, int256 expectedFracExponent) internal pure {
Float a = LibDecimalFloat.packLossless(x, exponent);
- (x, exponent) = a.floor().unpack();
- console2.log("x", x);
- console2.log("exponent", exponent);
+ (int256 floorX, int256 floorExponent) = a.floor().unpack();
+ console2.log("input x", x);
+ console2.log("input exponent", exponent);
+ console2.log("floor x", floorX);
+ console2.log("floor exponent", floorExponent);
console2.log("expectedFrac", expectedFrac);
console2.log("expectedFracExponent", expectedFracExponent);
- assertTrue((LibDecimalFloatImplementation.eq(x, exponent, expectedFrac, expectedFracExponent)));
+ assertTrue((LibDecimalFloatImplementation.eq(floorX, floorExponent, expectedFrac, expectedFracExponent)));
}🤖 Prompt for AI Agents
In `@test/src/lib/LibDecimalFloat.floor.t.sol` around lines 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.
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;
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.