Repository navigation
test: pin every flare fork test at block 37796420 - #218
Merged
Merged
Conversation
`BLOCK_NUMBER` moves 31843105 -> 37796420, which is already `FLRETH_BLOCK_NUMBER`, so the whole suite forks flare at one block and nothing reads state ~23 months back any more. `FtsoRegistry`, `FtsoV2` and `FeeCalculator` resolve to the same addresses at both blocks; only the values they serve changed, so every block-relative expectation is re-derived from chain at 37796420: ETH/USD 2525.74849 -> 2729.116 BTC/USD 67694.11308 -> 98162.81 XRP/USD 0.53163 -> 2.6189 USDT/USD 0.99919 -> 0.99997 sFLR/FLR 0.877817288626455057 -> 0.775006975937778042 FtsoV2 ETH/USD LTS feed 2522.575 -> 2729.116, timestamp 1729795768 -> 1740156916 `testFtsoV2LTSGetFeedPaid` needed more than a new literal. It warps forward by the governance timelock (3600s) to enact the fee change, then reads the feed with a 3600s staleness timeout — so the timelock consumed the entire freshness budget and left nothing for the gap between the feed timestamp and the fork block's own timestamp. At 31843105 that gap was exactly zero (block.timestamp == 1729795768 == the feed timestamp), which is the only reason it ever passed; at 37796420 the block is one second after the feed update and the read reverts StalePrice by that one second, before any fee is charged. The timeout now covers the timelock it warps through, which is what the test always meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rainix pin the sol workflows run moved on 2026-09-29; rain.flare last
pushed to main on 2026-09-14, so nothing has exercised the static job
since and `forge lint -D warnings` is red on main independently of any
open branch. Two separate causes, and the first hides the second.
solc aborts first. `forge lint` only sees the files slither's build
skipped, so in CI it compiles test/ fresh and `-D warnings` denies solc's
own diagnostics: two `Function state mutability can be restricted to
view` on the flrETH expectRevert wrappers. Locally they never appear,
because a previous `forge build` already cached those artifacts — the
only faithful local reproduction is `forge clean` first.
With those fixed solc passes and the ten forge-lint findings behind it
become reachable, each getting the scoped disable plus its reason that
the workflow's own comment asks for rather than a wholesale silence:
block-timestamp x2 comparing against block.timestamp IS the staleness
check these two libraries implement
unsafe-typecast x6 bytes32("<word name>") on ASCII literals far under
32 bytes, and the literal is what the test asserts
unsafe-typecast x2 bytes21() over a 21-byte allocation, and bytes1()
whose whole job is to keep the leading byte
No behavior changes: 85 passed / 0 failed, and `forge clean` followed by
`forge lint -D warnings` is now silent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`slither .` exits 255 on this branch and the 255 is this branch's doing.
`SlitherCore.has_ignore_comment` reads exactly `min(lines) - 1` for a finding,
so `//slither-disable-next-line timestamp` only suppresses while it is the line
immediately above the flagged code. Two of the ten scoped
`//forge-lint: disable-next-line(block-timestamp)` comments the previous commit
added landed between an existing slither comment and its `if`, pushing the
slither comment to `min(lines) - 2` and silently unsuppressing both `timestamp`
findings. Two results under the default `--fail-on pedantic` is `sys.exit(-1)`.
Slither's region form carries no adjacency requirement — `has_ignore_comment`
matches on `start < lines[0] and end > lines[-1]` — so both sites take
`//slither-disable-start timestamp` / `//slither-disable-end timestamp` around
the `if` and leave the forge-lint comment where forge lint needs it. That is
the shape `src/interface/IDineroFlrEth.sol` already uses for the same
collision. No detector is excluded and `slither.config.json` is untouched.
The two `ERROR:Slither:` lines on the same CI run are an older, separate bug
that pre-dates both branches. `parse_ignore_comments` does
`end_match[0].split(",")`, so a bare `//slither-disable-end` asks for the range
of a check literally named `""`, which no `-start` ever opened: it logs
"slither-disable-end without slither-disable-start", returns, and leaves
`naming-convention` open from line 6 to infinity, after which the second pass
over the file logs "Consecutive slither-disable-starts". Hence the end comment
now names its detector. Closing the range surfaces nothing new, because
`tokensPerLST` is already valid mixedCase; only `LSTPerToken` ever needed it.
Unpiped exits in the devshell at the CI pin, after `slither .`'s own
`forge clean` so `forge lint` compiles test/ fresh the way CI does:
slither . 0 (0 result(s) found, no ERROR lines)
forge fmt --check 0
forge lint -D warnings 0
pre-commit --all-files 0
rainix-sol-single-contract 0
comment-loc-cap 0 (src 1631 against a cap of 1874)
What separates the two causes: with only the `-end` detector name fixed and the
two timestamp sites left as they were, `slither .` is still 255 with the same 2
results and no ERROR lines at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…k change #217 fixes `forge lint -D warnings` and `slither .` being red on `main` for reasons unrelated to the fork block. Carried here so the static job on this branch reports on the fork change rather than on #217's breakage; it drops out of this diff the moment #217 merges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (15)
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 |
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.
test/fork/ForkConstants.sol'sBLOCK_NUMBERmoves 31843105 -> 37796420, whichis the value
FLRETH_BLOCK_NUMBERalready had. The suite now forks flare at asingle block, and nothing in it reads account state ~23 months back.
That matters outside this repo. rainix's
rpc_preflightnetwork table pinsflare's
archive_blocksto31_000_000because of this repo's old block — itsown comment says so: "rain.flare forks at 31_843_105 in ~19 test files (~23
months back) — the deepest historical read in the org outside rain.deploy's
genesis search, and Flare public nodes are the most prune-happy." The
RPC_URL_FLARE_FORKcandidates are not archive that deep. Once this merges thatentry can come forward, as its own rainix PR after this one: probing shallower
than a live pin is exactly what the field exists to prevent, so the order is not
optional. The rest of the org was re-checked against default branches before
opening this: every
vm.createSelectForkwith a flare block pin is in thisrepo;
rain.tofu.erc20-decimalsforks flare withcreateSelectFork(network)and no block, i.e. at latest;
rainlanguage/flowonly plumbsFLARE_RPC_URLthrough a workflow; and raindex's
RAINDEX_START_BLOCK_FLAREis 63011168, farahead of 37796420.
What changed
FtsoRegistry,FtsoV2andFeeCalculatorresolve to identical addresses atboth blocks, so no plumbing changed. Only the values they serve did, and every
block-relative expectation is re-derived from chain at 37796420 — not from
whatever the code produced:
The four pair expectations (ETH/BTC, BTC/ETH, ETH/FLR, FLR/ETH) are the
quotients of those same chain reads; they carry full coefficients because the
quotients do not terminate. FLR/USD at the new block is 0.02039, which is what
makes ETH/FLR 133845.80676802354.
testFtsoV2LTSGetFeedPaidwas not a stale literalIt warps forward by the governance timelock to enact the fee change, then read
the feed with a staleness timeout of a bare
3600— so the timelock consumedthe entire freshness budget, leaving nothing for the gap between the feed's
timestamp and the fork block's own timestamp.
At 31843105 that gap was exactly zero:
block.timestamp == 1729795768 ==thefeed timestamp. That is the only reason the test ever passed. At 37796420 the
block is one second after the feed update —
cast block 37796420 -f timestampis 1740156917 against a feed timestamp of 1740156916 — so
block.timestamp + timelock > feedTimestamp + 3600and the read revertsStalePrice(1740156916, 3600)before any fee is charged.The fix makes the dependency explicit rather than re-pinning it: the timeout is
now
timelock + 3600, wheretimelockis the value the test already reads fromGovernanceSettings.getTimelock()on chain, so nothing new is hardcoded and afuture timelock change cannot re-break it the same way. What the test measures
is the fee behaviour it was written to measure: underpayment reverts with no
revert data (
OutOfFunds), exact payment returns the feed value, overpayment isstranded in the consumer. Nothing was weakened, nothing deleted, and the
staleness boundary itself is still pinned by the four dedicated boundary tests
(M04/M05 below), which keep their own exact
3600and0timeouts.Housekeeping
The first two commits here are #217 —
forge lint -D warningsandslither .both being red on
mainfor reasons unrelated to the fork block (aviewkeyword, ten scoped forge-lint disables, and then the slither suppressions those
disables displaced). They are merged in so this branch's own static job reports
on the fork change instead of on #217's breakage; they drop out of this diff the
moment #217 merges, and this PR is still self-contained if it does not. Review
#217 there, not here.
QA
Discriminating tests: no new tests — this PR re-derives the values existing
fork tests already assert, and each assertion is shown discriminating by the
matrix below. Verified failing on base, re-run here rather than quoted:
holding this branch's new literals at the old block —
BLOCK_NUMBERback to31843105, nothing else touched — gives
forge testexit 1 and72 tests passed, 13 failed, across 8 distinct test names(
testFlareFtsoWordsFtsoCurrentExchangeRateHappyFork,testFlareFtsoWordsFtsoCurrentPricePairHappyFork,testFlareFtsoWordsFtsoCurrentPricePairHappyPrecisionFork,testFlareFtsoWordsFtsoCurrentPriceUsdHappyFork,testFtsoV2LTSGetFeed,testGetSFLRPerFLR18,testRunCurrentPricePairForkHappy,testRunForkCurrentPriceHappy). So every value this PR moves is load-bearingfor at least one assertion.
testFtsoV2LTSGetFeedPaidwas the one thatfailed with
StalePrice(1740156916, 3600)before the timeout fix and nowpasses. Measured on
c325fb5in thedevshell at the CI rainix pin, exit statuses read unpiped:
forge testexit0,
Ran 16 test suites: 85 tests passed, 0 failed, 0 skipped— the samecount as the baseline at the old block. CI on
c325fb5:rainix-solandGit is cleanboth green.Mutations applied (
mutation-probeonc325fb5against.mutation-test/mutants.toml, exit 0, baseline green at 85 passed, 8mutants, 8 killed / 0 survived / 0 no-run / 0 harness errors). Killer
names below are transcribed verbatim from that run; the tool reports up to
five
[FAIL]matches per mutant, so a list is the first five it saw and nota complete killer set, and a name repeated in a list is repeated in the
tool's own output:
LibFtsoCurrentPriceUsd.solreturn (price, decimals)->(price + 1, decimals)testBytecodeHashMatchesDeployedCode,testFlareFtsoWordsFtsoCurrentPriceUsdHappyFork,testFlareFtsoWordsFtsoCurrentPricePairHappyFork,testFlareFtsoWordsFtsoCurrentPricePairHappyPrecisionFork,testRunCurrentPricePairForkHappyLibOpFtsoCurrentPriceUsd.soluint8(decimals)->uint8(decimals) + 1testBytecodeHashMatchesDeployedCode,testIntegrityPointers,testOpcodePointers,testSubParserOperandHandlers,testFlareFtsoWordsFtsoCurrentPricePairHappyForkLibFtsoV2LTS.solreturn value->return value + 1testFtsoV2LTSGetFeed,testFtsoV2LTSGetFeedPaid,testFtsoV2LTSGetFeed,testFtsoV2LTSGetFeedPaidLibFtsoV2LTS.solstaleness>->>=testFtsoV2LTSGetFeedExactTimeoutNotStale,testFtsoV2LTSGetFeedTimeoutZeroExactNotStale,testFtsoV2LTSGetFeedExactTimeoutNotStale,testFtsoV2LTSGetFeedTimeoutZeroExactNotStaleLibFtsoV2LTS.solfeedtimestamp+= 1testFtsoV2LTSGetFeedJustStale,testFtsoV2LTSGetFeedStale,testFtsoV2LTSGetFeedTimeoutZeroStale,testFtsoV2LTSGetFeedJustStale,testFtsoV2LTSGetFeedStaleLibSceptreStakedFlare.solreturn rate->return rate + 1testBytecodeHashMatchesDeployedCode,testGetSFLRPerFLR18,testFlareFtsoWordsFtsoCurrentExchangeRateHappyFork,testBytecodeHashMatchesDeployedCode,testFlareFtsoWordsFtsoCurrentExchangeRateHappyForkLibOpFtsoCurrentPricePair.solnumerator.div(denominator)->denominator.div(numerator)testBytecodeHashMatchesDeployedCode,testFlareFtsoWordsFtsoCurrentPricePairHappyFork,testFlareFtsoWordsFtsoCurrentPricePairHappyPrecisionFork,testRunCurrentPricePairForkHappy,testRunPairDerivationExactLibFtsoCurrentPriceUsd.solstaleness>->>=testBytecodeHashMatchesDeployedCode,testRunFtsoNotActiveA,testRunDecimalOverflow,testRunDecimalsBoundary,testRunHappyM04 and M05 are the ones that matter for the paid-fee fix: the staleness
boundary and the exact feed timestamp are still pinned after the timeout
change, by tests that did not change.
testBytecodeHashMatchesDeployedCodeappears in five of the eight lists and is kept here rather than filtered out,
because filtering it would be editing the tool's output: it asserts
address(FlareFtsoWords).codehash == BYTECODE_HASH, so it fires on anysrc/edit that moves bytecode and is not the discriminating killer for any of
these behaviours. M03/M04/M05 are killed only by the LTS tests, with no
bytecode-hash contribution at all.
Oracle: the chain at block 37796420, read independently of the libraries
under test. The value table above was derived in the commit that changed it
(
FlareContractRegistry.getContractAddressByName->FtsoRegistry.getFtsoBySymbol(sym)->IFtso.getCurrentPriceWithDecimals()for ETH/BTC/XRP/USDT/FLR, all 5 decimals;
FtsoV2.getFeedByIdInWei(ETH/USD)for the LTS value and its timestamp;
IStakedFlr.getSharesByPooledFlr(1e18)for the sFLR rate; pair values checked as quotients of those reads rather
than read back out of the
Float), and every one of those literals isre-confirmed by the 85-test green run above, since each is an
assertEqagainst a live read. The one number the fix itself turns on — the fork
block's own timestamp, 1740156917, one second past the feed's 1740156916 —
was re-read here directly with
cast block 37796420 -f timestamp. Thetimelock is not asserted as a literal anywhere: the test reads it from
GovernanceSettings.getTimelock().Category check: the ask was (a) one fork block for rain.flare, (b) re-derive
every block-relative expectation from chain, (c) fix the paid-fee failure
without weakening a test; covered a, b, c. Not included and deliberately
separate: the rainix
archive_blocksmove, which must follow this merge, andfix: get forge lint -D warnings green again #217, which is carried but reviewed on its own PR. No
audit/mutation-test-scans.jsonentry is added — this was a re-run of an existingledger for this PR's evidence, not a new scan, and the repo has no scan
record to append to.
🤖 Generated with Claude Code