phase 0 — cover the redeem downside branch [BS-4845] - #674
Open
iamsahu wants to merge 2 commits into
Open
Conversation
…, gate contract sizes, diagnose Certora Groundwork for the "LsETH Rewards while Redeeming" work. No production code changes. Redeem downside coverage. Today's redeem payout is min(rate_at_settlement, rate_at_request): maxRedeemableEth is recorded at request time and the claim path only ever clamps the payout DOWN to it (RedeemManager.1.sol:430-436), with no symmetric branch. So a redeemer bears the full loss when the pool rate falls between request and settlement. testClaimMultiRate only ever settles at rates >= the request rates, so that branch had zero coverage. Adds two tests: - testClaimSettlementRateBelowRequestRateChargesFullLossToRedeemer: pins that the request-time value is a cap and never a floor. - testPartialClaimBelowRequestRateDriftsImpliedCapRate: pins that maxRedeemableEth is a decrementing ETH budget, not a rate — after a partial claim below the request rate the implied per-LsETH cap ratchets from 1.0 to 50.5. Anything that later reads maxRedeemableEth / amount as "the rate at request time" would over-credit. Contract size gate. RiverV1 is at 22,970 runtime bytes — 1,606 free — and every cheap lever is already spent (foundry.toml: optimizer_runs = 3, via_ir, bytecode_hash = none). Plain `forge build --sizes` cannot be used as a gate because it exits non-zero on AccountingRiverV1 and RiverV1ForceCommittable, two test harnesses that are over the limit by design. Adds scripts/check_contract_sizes.sh, scoped to contracts declared under contracts/src, plus a `make size` target and a CI step. Also sets contractSizer.strict in hardhat.config.ts: hardhat compiles only contracts/src, with looser settings than foundry.toml (runs 100 vs 3, default ipfs bytecodeHash), so it produces larger bytecode than forge — a green forge build is not evidence that a change deploys. strict is disabled for `hh coverage`, since solidity-coverage instruments past EIP-170. Certora diagnosis. Every conf except AllowlistV1 fails at the solc stage, so the workflow has been verifying nothing for those contracts since before a3bb388 (2026-06-04). Because certoraRun fails on compilation rather than on a rule, it does not look like a verification failure. certora/README.md records the exact breakage and splits it into two independent repairs: RiverV1Harness is coupled to the pre-extraction architecture (and has drifted behaviourally — it still calls the one-arg _commitBalanceToDeposit, missing the slashing-containment parameter), and OperatorsRegistryV1Harness uses OperatorAllocation, which the V3 ETH-based refactor removed. No conf change here: repointing RedeemManagerV1.conf at production River/OperatorsRegistry is very likely the fix, but it cannot be validated without CERTORAKEY and a prover run.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
iamsahu
changed the title
iamsahu
marked this pull request as ready for review
August 17, 2026 12:33
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.
Description
This pull request introduces automated checks to ensure that all deployable production contracts remain within the EIP-170 runtime bytecode size limit, both in local development and continuous integration (CI). It adds a new script and CI step for Foundry builds, strengthens Hardhat configuration for stricter enforcement, and documents the rationale and usage of these safeguards. Additionally, it introduces new tests and documentation for contract behavior and formal verification coverage.
Contract Size Enforcement:
scripts/check_contract_sizes.sh, which checks all deployable production contracts against the EIP-170 runtime bytecode limit during Foundry builds. This script is scoped to production contracts and allows for a configurable minimum margin..github/workflows/Tests.yaml, ensuring that oversized contracts are caught before deployment.Makefileto include asizetarget that runs the new size-check script, and marked it as a phony target. [1] [2]Hardhat Configuration Improvements:
hardhat.config.tsto setcontractSizer.stricttotruefor local and CI builds (except during coverage runs), causing Hardhat compilation to fail if any contract exceeds the EIP-170 limit. This ensures that the Hardhat build, which produces larger bytecode than Foundry, is also gated. [1] [2]Testing and Documentation:
RedeemManager.1.t.solto verify correct handling of redemption requests when the settlement rate falls below the request rate, and to ensure the cap logic behaves as intended in edge cases.README.mdtocertora/explaining the current state of formal verification, documenting known breakages, and outlining steps for future repairs.Notice
Pull Request Type
Breaking changes (if applicable)
Testing
Manual tests (if applicable)
Additional comments