State Structure for Tests #134
Conversation
The ones labelled .outdated will be replaced by this branch. The ones labelled .suppressed will be replaced by a different PR
They now revert on every function call. I will expand the functionality as needed
Having both version abstract seems weird to me. It also makes it harder to test. I just copied the values from the test suite to the default ProverManagerConfig. We should discuss whether these are sensible parameters.
Before this commit, there was an edge case where a zero fee could be outbid (with zero).
This means we don't have to keep deconflicting in the ERC20 scenarios
Users should call claimVacancy instead
This should cover all the scenarios that we will test. We still need to expand the test suites for the relevant functions
This prover will eventually need to ensure old publications are proven but the prover might be joining at a time when there are lots of unproven publications, and he should not be evicted because of them
WalkthroughThis update introduces significant enhancements and refactoring to the Prover Manager protocol and its associated test suite. Core protocol interfaces and contracts are revised for improved period tracking, delayed publication handling, and vacancy claiming. The test suite is extensively expanded and modularized, covering a wide range of period states, transitions, and invariants for both ETH and ERC20 currencies. Mock contracts are reworked for better control and clarity in testing. Changes
Sequence Diagram(s)Proving Period Vacancy Claim FlowsequenceDiagram
participant Prover as Prover
participant ProverManager as ProverManager
participant Period as Period
Prover->>ProverManager: claimProvingVacancy(fee)
ProverManager->>Period: Close current period (set end, deadline)
ProverManager->>Prover: Deduct liveness bond
ProverManager->>Period: Initialize next period (prover, stake, fee, delayedFeePercentage)
ProverManager-->>Prover: Emit ProverVacancyClaimed event
Proving Transition (Simplified)sequenceDiagram
participant ProverManager as ProverManager
participant CheckpointTracker as CheckpointTracker
participant Verifier as Verifier
ProverManager->>CheckpointTracker: proveTransition(start, end, proof)
CheckpointTracker->>Verifier: verifyProof(..., intermediateCheckPoint, numDelayedPublications, proof)
Verifier-->>CheckpointTracker: Verification result
CheckpointTracker->>CheckpointTracker: Save commitment, update provenPublicationId, delayedPublications
CheckpointTracker-->>ProverManager: Return (numPublications, numDelayedPublications)
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (21)
src/protocol/ETHProverManager.sol (2)
11-19: Constructor duplicates_livenessBondguard – consider delegating toBaseProverManager.
BaseProverManageralready re-checks that_initialDeposit(heremsg.value) is ≥livenessBond.
Keeping a secondrequirebloats byte-code and gas. If there is no scenario whereBaseProverManagercan be bypassed, the extra check is redundant.
21-31: Missing zero-value guard indeposit()/receive().A 0-ETH call credits the sender via
_deposit, needlessly increasing storage slots and wasting gas.
Add an early-exit or revert whenmsg.value == 0.+ if (amount == 0) return; // or: revert("Zero deposit");src/protocol/ERC20ProverManager.sol (2)
18-35: Zero-amount deposits should be rejected.
deposit(uint256 amount)updates internal balances before performingsafeTransferFrom.
Passing0incurs needless storage writes and event noise while transferring nothing.- _deposit(msg.sender, amount); + require(amount > 0, "Zero deposit"); + _deposit(msg.sender, amount);
24-35: Order of state update vs. token transfer – minor optimisation.If the
safeTransferFromreverts (insufficient allowance/balance), the state write from_deposit
is rolled back, so correctness is preserved.
However, writing state first and then reverting costs extra gas for storage-refund.
Consider performing the transfer first, then bookkeeping.test/ProverManager/InvariantTest.t.sol (1)
49-54: Possible false-failure when previous period is still open.
assertGt(block.timestamp, previous.end)will fail ifprevious.end == 0(open period).
Add a guard to skip the check whenprevious.end == 0.- assertGt(vm.getBlockTimestamp(), previous.end, "Previous period is not past"); + if (previous.end != 0) { + assertGt(vm.getBlockTimestamp(), previous.end, "Previous period is not past"); + }test/mocks/MockCheckpointTracker.sol (1)
6-19: Stub now reverts on all calls – confirm no positive-flow tests rely on it.Replacing logic with
NotImplementedsimplifies the mock, but any future test that expects
proveTransitionorgetProvenCheckpointto succeed will now revert.If you need read-only behaviour, consider returning dummy data rather than reverting in
getProvenCheckpoint.test/ProverManager/CurrentPeriodIsActiveTest.t.sol (3)
11-23: Add event assertions to strengthen the fee-payment test
payPublicationFeeemitsPublicationFeePaid(or an equivalent) in BaseProverManager.
Capturing that event here guards against silent regressions where the call might no-op or be short-circuited in future refactors.vm.expectEmit(); -// missing: event selector & arguments +emit BaseProverManager.PublicationFeePaid( + proposer, + proverManager.currentPeriodId(), + /* isDelayed = */ false, + period.fee +);
25-38: Mirror the regular-fee event assertion for the delayed-fee pathSame reasoning as above; add
vm.expectEmitbefore the delayed-fee call.
40-62: Minor nit: collapse duplicate escrow-balance checksThe two escrow-balance tests differ only by the
isDelayedflag. Merging them into a single parametrised test cuts compile time without losing coverage.test/ProverManager/UniversalTest.t.sol (1)
45-48: Consider usingvm.prank(address(0xdead))to make the caller explicitRelying on the test contract’s own address hides intent. An explicit, non- privileged address clarifies that anyone except the inbox must revert.
test/ProverManager/CurrentPeriodIsOverTest.t.sol (1)
49-71: Combine regular & delayed escrow-balance checksSame duplication comment as for
CurrentPeriodIsActiveTest.src/protocol/BaseProverManager.sol (2)
38-40: Doc comment missing for newPublicationBeforeCurrentPeriodA short NatSpec blurb (similar to neighbouring errors) will surface in generated docs and help integrators understand the revert reason.
270-273: Ambiguous revert reason whenfee == 0
_ensureSufficientUnderbidnow reverts withOfferedFeeTooHigh()even when the real issue is trying to outbid a zero fee.
Consider introducing a dedicated error (CannotOutbidZeroFee) or re-usingNoProvingVacancyfor clearer UX.test/ProverManager/InitialState.t.sol (1)
55-61: Avoid recomputing the domain separator on every call
_randomAddresshashes_domainSeparator()each time it is invoked. Because the separator is a fixed literal, you can cache it in an immutable /bytes32 constantto save gas and avoid duplicate keccak256 runs:- function _domainSeparator() internal pure returns (bytes32) { - return keccak256("ProverManager"); - } + bytes32 private constant _DOMAIN_SEPARATOR = keccak256("ProverManager"); + + function _domainSeparator() internal pure returns (bytes32) { + return _DOMAIN_SEPARATOR; + }test/ProverManager/InitialStateTest.t.sol (1)
56-59: Wrong assertion message refers to period 0 instead of period 1The test is validating period 1 but the failure message still says “Period 0 has missed deadline”, which is misleading.
- assertEq(period.pastDeadline, false, "Period 0 has missed deadline"); + assertEq(period.pastDeadline, false, "Period 1 has missed deadline");test/ProverManager/CurrencyScenario.t.sol (1)
67-73:assertcan abort the whole test run – userequireor early-return
_prefundaborts withassert(currentBalance <= amount).
In unit-test helpers, an explicitrequire(or simply minting only whenamount > currentBalance) avoids an unrecoverable stop in case of mis-configuration and keeps other tests running.- assert(currentBalance <= amount); - token.mint(account, amount - currentBalance); + if (currentBalance < amount) { + token.mint(account, amount - currentBalance); + }src/protocol/ProverManagerConfig.sol (1)
44-95: Mark constant getters aspureto save bytecode & gasAll underscored helpers return literals and read no state –
viewis stricter than necessary and prevents the compiler from generatingPUSH-only opcodes.- function _maxBidFraction() internal view virtual returns (uint16) { + function _maxBidFraction() internal pure virtual returns (uint16) { @@ - function _livenessWindow() internal view virtual returns (uint40) { + function _livenessWindow() internal pure virtual returns (uint40) { @@ - function _successionDelay() internal view virtual returns (uint40) { + function _successionDelay() internal pure virtual returns (uint40) { @@ - function _exitDelay() internal view virtual returns (uint40) { + function _exitDelay() internal pure virtual returns (uint40) { @@ - function _provingWindow() internal view virtual returns (uint40) { + function _provingWindow() internal pure virtual returns (uint40) { @@ - function _livenessBond() internal view virtual returns (uint96) { + function _livenessBond() internal pure virtual returns (uint96) { @@ - function _evictorIncentiveFraction() internal view virtual returns (uint16) { + function _evictorIncentiveFraction() internal pure virtual returns (uint16) { @@ - function _rewardFraction() internal view virtual returns (uint16) { + function _rewardFraction() internal pure virtual returns (uint16) { @@ - function _delayedFeePercentage() internal view virtual returns (uint16) { + function _delayedFeePercentage() internal pure virtual returns (uint16) {[predictable_gas_saving]
test/ProverManager/CurrentPeriodIsOpenTest.t.sol (1)
109-113: Callvm.expectEmitbeforevm.prankfor clearer intentAlthough the current order works, registering the expected emission first—and then switching the
msg.sender—makes the test flow easier to follow and avoids accidental reuse of the prank across multiple expectations.- vm.prank(proverA); - vm.expectEmit(); + vm.expectEmit(); + vm.prank(proverA);test/ProverManager/NextPeriodHasBidTest.t.sol (2)
21-27: Depositing the fullcurrentBidis unnecessary noise.The contract only requires the liveness bond to be escrowed for a bid; depositing the (potentially large)
currentBidvalue is irrelevant to the behaviour under test and makes intent less clear.
Consider reducing the deposit toproverManager.livenessBond()to keep the fixtures minimal.
82-84: Hidden assumption on constant liveness bond.The assertion
balanceAfter == balanceBefore + proverManager.livenessBond()implicitly assumes the bond size cannot change between bids. If the bond becomes configurable in the future, this test will give a false negative.
Add a comment or capture the bond value before the call and assert against that snapshot to make the expectation explicit.test/ProverManager/PeriodScenario.t.sol (1)
47-52: Minor: duplicated magic-number computation could be centralised
LibPercentage.scaleByBPS(initialFee, proverManager.maxBidFraction())shows up in several test setups. Extracting a helper (e.g.,uint96 _maxValidBid(uint96 fee)) would DRY the scenario files and avoid future mismatches if the formula changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/libs/LibProvingPeriod.sol(1 hunks)src/protocol/BaseProverManager.sol(4 hunks)src/protocol/ERC20ProverManager.sol(1 hunks)src/protocol/ETHProverManager.sol(1 hunks)src/protocol/ProverManagerConfig.sol(1 hunks)test/ProverManager/CurrencyScenario.t.sol(1 hunks)test/ProverManager/CurrentPeriodIsActiveTest.t.sol(1 hunks)test/ProverManager/CurrentPeriodIsOpenTest.t.sol(1 hunks)test/ProverManager/CurrentPeriodIsOverTest.t.sol(1 hunks)test/ProverManager/InitialState.t.sol(1 hunks)test/ProverManager/InitialStateTest.t.sol(1 hunks)test/ProverManager/InvariantTest.t.sol(1 hunks)test/ProverManager/NextPeriodHasBidTest.t.sol(1 hunks)test/ProverManager/PeriodScenario.t.sol(1 hunks)test/ProverManager/UniversalTest.t.sol(1 hunks)test/mocks/MockCheckpointTracker.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)
🔇 Additional comments (5)
src/protocol/ETHProverManager.sol (1)
33-40: ```shell
#!/bin/bash1. Show the definition and context of _transferOut
rg -n "_transferOut" -C3 src/protocol/ETHProverManager.sol
2. Find all callers of _transferOut within the repo
rg -n "\b_transferOut" -C5 .
3. Check for ReentrancyGuard usage or nonReentrant modifiers
rg -n "nonReentrant" -C2 .
4. Inspect balance-related state updates in ETHProverManager.sol
rg -n "balance" -C3 src/protocol/ETHProverManager.sol
</details> <details> <summary>src/libs/LibProvingPeriod.sol (1)</summary> `65-68`: **Fee scaling change LGTM – verify rounding semantics.** Switching to `scaleByPercentage` improves readability and avoids passing a denominator. Ensure `LibPercentage.scaleByPercentage` still uses 10 000-bps precision like the old call; otherwise fee growth may be off by two decimals. </details> <details> <summary>test/ProverManager/UniversalTest.t.sol (1)</summary> `27-43`: **Ensure `WITHDRAW_AMOUNT` ≤ `DEPOSIT_AMOUNT`** The test implicitly assumes the withdraw amount never exceeds the prior deposit. If these constants diverge in the future this test will revert rather than fail an assertion. A safety guard keeps the test self-contained: ```diff +require(WITHDRAW_AMOUNT <= DEPOSIT_AMOUNT, "Test constant mismatch");test/ProverManager/CurrentPeriodIsOverTest.t.sol (1)
20-32: Guard against uninitialised “next period” struct
getPeriod(currentPeriodId() + 1)will yield zeroed fields if no bid exists.
An explicitassume(period.isInitialized())(or arequirewith a revert-reason) documents the pre-condition and prevents false positives.LibProvingPeriod.Period memory period = proverManager.getPeriod( proverManager.currentPeriodId() + 1 ); +require(period.isInitialized(), "Next period should be initialised for test");src/protocol/BaseProverManager.sol (1)
154-160: Edge-case: publication at exact period endThe check
publicationHeader.timestamp > previousPeriodEndallows eviction if the timestamp equals
previousPeriodEnd + 1but not when it equalspreviousPeriodEnd.
Depending on howendis defined (inclusive vs exclusive) this may unintentionally bar a valid header. Please confirm the intended comparison (>=vs>).
Now all the instantiations are in one place. It also means PeriodScenarios.t.sol is the only file that defines multiple things
This is consistent with the comment describing the parameter
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/mocks/MockInbox.sol (1)
12-22: Consider implementing permissive defaults instead of reverting.While the current implementation provides explicit control, several
BaseProverManagercode paths may depend on these functions succeeding. Consider implementing permissive defaults (e.g., returning0or empty values) to reduce test boilerplate.This aligns with the previous review suggestion about providing "happy path" defaults to avoid forcing each test to deploy custom mocks.
🧹 Nitpick comments (3)
test/ProverManager/CurrentPeriodIsOpenTest.t.sol (3)
173-180: Clarify the test expectation in the negative assertion.The test uses
vm.expectRevert(BaseProverManager.PublicationNotOldEnough.selector, 0)which seems to expect no revert, but the function name suggests it should test that publications before the liveness deadline do NOT revert.Consider renaming the test or clarifying the assertion:
- function test_CurrentPeriodIsOpen_evictProver_shouldNotRevertIfPublicationIsBeforeLivenessDeadline() public { + function test_CurrentPeriodIsOpen_evictProver_shouldRevertIfPublicationIsAfterLivenessDeadline() public {Or update the test logic to match the intended behavior.
191-199: Fix inconsistent test expectation.Similar to the previous test, this uses
vm.expectRevert(..., 0)which expects no revert, but the test name suggests it should verify that publications on period start do revert.The test logic should match the function name:
- function test_CurrentPeriodIsOpen_evictProver_shouldRevertIfPublicationIsOnPeriodStart() public { + function test_CurrentPeriodIsOpen_evictProver_shouldNotRevertIfPublicationIsAfterPeriodStart() public {Or adjust the test logic to match the intended validation.
210-217: Fix test name and expectation mismatch.The test name suggests it should not revert, but it uses
vm.expectRevert(..., 0)and the logic seems backwards.Clarify the test intention:
- function test_CurrentPeriodIsOpen_evictProver_shouldNotRevertIfPublicationIsNotProven() public { + function test_CurrentPeriodIsOpen_evictProver_shouldSucceedIfPublicationIsNotProven() public {And remove the expectRevert if the test should succeed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/protocol/BaseProverManager.sol(5 hunks)test/ProverManager/CurrentPeriodIsOpenTest.t.sol(1 hunks)test/ProverManager/InitialState.t.sol(1 hunks)test/mocks/MockCheckpointTracker.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ProverManager/InitialState.t.sol
- src/protocol/BaseProverManager.sol
🔇 Additional comments (13)
test/mocks/MockCheckpointTracker.sol (4)
7-7: LGTM: Clean error definition for unimplemented functionality.The
NotImplementederror provides clear feedback when stub methods are called.
9-10: LGTM: Well-defined constants for predictable test behavior.The hardcoded constants provide deterministic values that tests can reference and validate against.
14-16: LGTM: Appropriate pure function implementation.The function correctly returns a hardcoded checkpoint for testing scenarios, allowing tests to work with predictable proven checkpoint data.
18-24: LGTM: Correct stub behavior for unimplemented functionality.The function appropriately reverts since transition proving is not implemented in this mock, preventing accidental test dependencies on complex proving logic.
test/mocks/MockInbox.sol (3)
8-8: LGTM: Appropriate default for header validation.The default
truevalue allows tests to proceed with valid headers by default, while still allowing control through the setter.
24-26: LGTM: Controlled header validation for testing.The implementation allows tests to control validation outcomes through the setter, enabling both positive and negative test scenarios.
28-30: LGTM: Appropriate setter for test control.The setter allows tests to configure header validation behavior as needed for different test scenarios.
test/ProverManager/CurrentPeriodIsOpenTest.t.sol (6)
14-16: LGTM: Clear contract purpose and assumptions.The comment clearly explains the contract's scope and assumptions about the fee being non-zero, which is important for understanding the test scenarios.
17-24: LGTM: Proper boundary testing for bid validation.The test correctly verifies that bids matching the current fee are rejected, which prevents unnecessary period changes.
26-33: LGTM: Comprehensive underbid validation testing.The test correctly verifies that insufficient underbids (fee - 1) are rejected, ensuring proper bid validation logic.
68-84: LGTM: Thorough balance validation for liveness bond deduction.The test properly verifies that the liveness bond is correctly deducted from the bidder's balance while maintaining the total escrowed amount.
232-257: LGTM: Comprehensive eviction incentive transfer testing.The test thoroughly validates the eviction incentive mechanism, checking all balance changes and ensuring proper stake deduction.
277-288: LGTM: Proper eviction event testing.The test correctly verifies that the
ProverEvictedevent is emitted with the expected parameters during eviction.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/ProverManager/CurrentPeriodIsOpenTest.t.sol (1)
145-155: Consider more specific revert expectation.The test correctly checks for insufficient balance but uses a generic
vm.expectRevert(). Consider specifying the expected error selector for better test precision.vm.prank(proverA); - vm.expectRevert(); + vm.expectRevert(BaseProverManager.InsufficientBalance.selector); proverManager.bid(offer);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/ProverManager/CurrentPeriodIsOpenTest.t.sol(1 hunks)
🔇 Additional comments (8)
test/ProverManager/CurrentPeriodIsOpenTest.t.sol (8)
1-16: LGTM: Well-structured test contract foundation.The contract structure, imports, and documentation are well-organized. The abstract contract design following the inheritance pattern from
CurrentPeriodIsActiveTestpromotes good test modularity.
17-33: LGTM: Comprehensive bid validation tests.These tests properly verify that bids matching or insufficiently underbidding the current fee are rejected with the correct error. The logic correctly uses
OfferedFeeTooHigherror for both scenarios.
47-66: LGTM: Thorough period closure testing.The test correctly verifies that bidding closes the current period by setting appropriate
endanddeadlinetimestamps while preserving other period attributes. The assertions comprehensively check all period fields.
68-84: LGTM: Proper balance management verification.The test correctly verifies that the liveness bond is deducted from the bidder's balance without changing the total escrowed value, ensuring proper accounting of funds.
86-104: LGTM: Comprehensive next period creation test.The test thoroughly verifies all aspects of the new period creation including prover assignment, stake, fee, delayed fee percentage, and initial state (open period with zero end/deadline).
232-257: LGTM: Thorough evictor incentive transfer verification.The test comprehensively verifies the incentive transfer mechanism, checking that funds are properly transferred from the prover's stake to the evictor's balance while maintaining total escrowed value invariant.
277-288: LGTM: Proper event emission testing.The event testing correctly verifies all parameters of the
ProverEvictedevent, ensuring proper event logging for eviction operations.
290-333: LGTM: Comprehensive exit functionality testing.The exit tests properly verify authorization, period state changes, and event emission. The tests ensure only the current prover can exit and that the period is properly closed with correct timestamps.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/ProverManager/CurrentPeriodIsVacant.t.sol(1 hunks)test/ProverManager/PeriodScenario.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ProverManager/PeriodScenario.t.sol
🧰 Additional context used
🪛 GitHub Actions: CI
test/ProverManager/CurrentPeriodIsVacant.t.sol
[error] 7-9: forge fmt --check failed due to import statement order mismatch. The import of 'IProverManager' should be after 'IInbox'.
🔇 Additional comments (10)
test/ProverManager/CurrentPeriodIsVacant.t.sol (10)
13-20: Excellent precondition validation.This sanity check effectively validates the expected vacant period state and includes a helpful comment explaining the invariant relationship between fee and prover presence.
22-38: Comprehensive publication fee testing for vacant periods.Both regular and delayed fee scenarios are properly tested, confirming that no charges occur when the period is vacant. The test structure is consistent and well-implemented.
47-55: Proper eviction validation for vacant periods.The test correctly verifies that eviction attempts during vacant periods fail with
PeriodNotInitialized, which aligns with the business logic that eviction requires an active prover.
57-65: Validate period closure logic.The test properly verifies that claiming vacancy closes the current period by setting both
endanddeadlineto the current timestamp.
67-83: Thorough validation of unchanged period fields.This test ensures data integrity by verifying that only the intended fields are modified during vacancy claiming. The assertion structure is comprehensive and correct.
85-94: Correct balance deduction verification.The test properly validates that the liveness bond is deducted from the prover's balance when claiming vacancy.
96-109: Comprehensive next period initialization testing.This test thoroughly validates all aspects of the next period setup, including prover assignment, stake amount, fee setting, and initial state values. The assertions are complete and correct.
111-120: Proper event emission verification.The test correctly validates the
ProverVacancyClaimedevent emission with all expected parameters. The use ofvm.expectEmit()is appropriate.
122-130: I’ll inspect the underlying validation logic in BaseProverManager:#!/bin/bash # Show underbid enforcement in BaseProverManager rg "_ensureSufficientUnderbid" -n src/protocol/BaseProverManager.sol -C4 echo "----" # Show claim vacancy helper logic rg "function _claimProvingVacancy" -n src/protocol/BaseProverManager.sol -C4
40-45: Let’s inspect the contract’s constructor (to see if it pre-initializes a period) and the test setup (to understand what_depositdoes):#!/bin/bash # Show BaseProverManager constructor rg -n -A5 -B5 "constructor" src/protocol/BaseProverManager.sol # Dump the test file to see setup and _deposit behavior sed -n '1,200p' test/ProverManager/CurrentPeriodIsVacant.t.sol
…kpoint In the current implementation, CheckpointTracker cannot validate that the start.totalDelayedPublications was set correctly. Instead, the verifier can check numDelayedPublications is set relative to the proven checkpoint. getProvenCheckpoint was only used to get the proven id, so now that is a public getter instead
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/mocks/MockInbox.sol (1)
8-8: Consider adding documentation for the mapping purpose.The
isInvalidmapping serves a specific testing purpose but lacks documentation explaining its role in the mock's behavior.+ /// @notice Mapping to track headers that should be considered invalid for testing mapping(bytes32 headerHash => bool) private isInvalid;test/ProverManager/CurrentPeriodHasPublications.t.sol (2)
12-14: Fix typo in comment.There's a typo in "encaspulate" which should be "encapsulate".
-/// Nevertheless, this contract is used to better encaspulate the proving tests +/// Nevertheless, this contract is used to better encapsulate the proving tests
41-43: Potential issue with checkpoint validation logic.The test sets
firstPub.id = start.publicationIdbut then expectsInvalidStartPublicationerror. This suggests the validation expectsfirstPub.idto bestart.publicationId + 1. Consider adding a comment explaining this relationship.firstPub.id = start.publicationId; + // Setting firstPub.id to start.publicationId should trigger InvalidStartPublication + // because the first publication should be start.publicationId + 1 vm.expectRevert(BaseProverManager.InvalidStartPublication.selector);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/ProverManager/CurrentPeriodHasPublications.t.sol(1 hunks)test/ProverManager/CurrentPeriodIsOpenTest.t.sol(1 hunks)test/ProverManager/PeriodScenario.t.sol(1 hunks)test/mocks/MockCheckpointTracker.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ProverManager/CurrentPeriodIsOpenTest.t.sol
- test/ProverManager/PeriodScenario.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: compare_gas_reports
🔇 Additional comments (13)
test/mocks/MockInbox.sol (3)
12-22: Mock implementation follows appropriate stub pattern.The functions correctly revert with
NotImplemented()since they're not needed for the current testing scenarios. This approach prevents tests from accidentally depending on unimplemented behavior while keeping the mock minimal.
24-26: Header validation logic is well-designed.The implementation defaults to valid headers unless explicitly marked invalid, which provides a good balance between permissive testing and controlled failure scenarios.
28-30: Test utility function provides good control.The
setInvalidHeaderfunction allows tests to precisely control which headers should fail validation, enabling focused testing of error conditions.test/mocks/MockCheckpointTracker.sol (4)
9-11: Good documentation for hardcoded test value.The comment clearly explains the purpose of the hardcoded
provenPublicationIdand how different scenarios can be tested relative to this value.
13-16: Well-structured mock state with appropriate defaults.The public visibility of mock values allows tests to inspect the current state, and the default values (
valid = true,nPublications = 3,nDelayedPublications = 1) provide reasonable starting points for testing.
18-25: Function signature aligns with interface requirements.The
proveTransitionfunction correctly returns two uint256 values and handles the validity check appropriately. The view modifier is correct since this is a mock that doesn't need to modify state during proof verification.
27-34: Setter functions provide good test control.The separation of validity control (
setValid) and publication count control (setNumPublications) allows tests to configure different scenarios independently.test/ProverManager/CurrentPeriodHasPublications.t.sol (6)
16-20: State variables are appropriately scoped.The checkpoint and publication header variables are well-named and provide the necessary test data structure. The empty proof bytes is appropriate for mock testing.
91-92: Deadline logic determines the correct prover.The logic correctly determines whether the deadline has passed and assigns rewards to either the original prover or proverB accordingly. This reflects the expected behavior where anyone can prove after the deadline.
119-119: Timestamp assignment handles bounded and unbounded periods.The conditional assignment
lastPub.timestamp = periodEnd == 0 ? vm.getBlockTimestamp() : periodEndcorrectly handles both cases where the period has a defined end time and where it's open-ended.
116-117: Potential off-by-one error in publication ID assignment.The assignment
firstPub.id = start.publicationId + 1assumes the first publication in the period is the next publication after the checkpoint. Verify this aligns with the protocol's publication ID semantics.#!/bin/bash # Search for publication ID handling patterns ast-grep --pattern $'firstPub.id = $_ + 1' ast-grep --pattern $'start.publicationId + $_'
67-69: Verify the conditional revert logic.The test uses a conditional expectation (
vm.expectRevert(BaseProverManager.LastPublicationIsAfterPeriod.selector, numReverts)) based on whether the period has an end timestamp. This logic should be verified to ensure it correctly handles both bounded and unbounded periods.#!/bin/bash # Verify the usage of conditional vm.expectRevert in the codebase rg -A 5 -B 5 "vm\.expectRevert.*numReverts|expectRevert.*uint64"
85-89: Let’s search across thesrcdirectory for the actual publication counts and inspectLibProvingPeriod.solto find the canonical reward logic:#!/bin/bash # 1. Find all occurrences of the tracker methods rg -n "nPublications\(" -A 5 src rg -n "nDelayedPublications\(" -A 5 src # 2. Check for any reward‐calculation helpers in LibProvingPeriod sed -n '1,200p' src/libs/LibProvingPeriod.sol
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/ProverManager/PreviousPeriodHasPublications.t.sol (1)
56-56: Fix formatting: add space around operator.Missing space in the subtraction operation.
- uint256 periodId = proverManager.currentPeriodId() -1; + uint256 periodId = proverManager.currentPeriodId() - 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/ProverManager/PeriodScenario.t.sol(1 hunks)test/ProverManager/PreviousPeriodHasPublications.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ProverManager/PeriodScenario.t.sol
🧰 Additional context used
🪛 GitHub Actions: CI
test/ProverManager/PreviousPeriodHasPublications.t.sol
[error] 16-56: forge fmt --check failed: code formatting differences detected. Run 'forge fmt' to fix code style issues.
🔇 Additional comments (3)
test/ProverManager/PreviousPeriodHasPublications.t.sol (3)
81-107: Verify the fee calculation logic and prover selection.The test correctly implements complex fee calculation logic including standard and delayed fees, and handles deadline-based prover selection. The escrow balance check ensures the contract's total balance remains unchanged while individual prover balances are updated correctly.
23-79: Test coverage for error scenarios looks comprehensive.The test functions cover all major error scenarios for the prove function when dealing with previous period publications: unknown publications, mismatched checkpoints, timing violations, and proof failures. The test structure follows good practices with clear naming and proper use of vm.expectRevert.
1-109: Address code formatting issues.The pipeline failure indicates formatting issues that need to be resolved. Run
forge fmtto fix code style issues as mentioned in the CI failure.#!/bin/bash # Description: Check and fix Solidity code formatting # Expected: Code should be properly formatted according to forge standards # Run forge fmt to fix formatting issues forge fmt # Verify no formatting issues remain forge fmt --check
nikeshnazareth
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/CheckpointTracker.t.sol(1 hunks)test/ProverManager/NextPeriodHasBidTest.t.sol(1 hunks)test/ProverManager/PreviousPeriodHasPublications.t.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)test/mocks/MockVerifier.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ProverManager/NextPeriodHasBidTest.t.sol
- test/ProverManager/PreviousPeriodHasPublications.t.sol
🔇 Additional comments (3)
test/mocks/MockVerifier.sol (1)
6-26: LGTM! Well-structured mock implementation.The
MockVerifierprovides good test control with the ability to toggle proof validity. The implementation correctly aligns with the updatedIVerifierinterface including the newintermediateCheckPointparameter.test/mocks/MockInbox.sol (1)
8-40: Clean and effective mock implementation.The simplified
MockInboxprovides all necessary functionality for testing:
- Starting
numPublicationsat 1 ensures there's always a previous hash- Deterministic hash generation in
getPublicationHashmakes tests predictable- The
isInvalidmapping withsetInvalidHeaderallows testing validation failurespublishMultipleenables efficient test setuptest/CheckpointTracker.t.sol (1)
1-188: Comprehensive and well-structured test suite.The test file provides excellent coverage of the
CheckpointTrackerfunctionality:
- Thorough constructor validation including zero genesis check
- Complete edge case coverage for
proveTransition- Proper event emission testing
- Good use of helper functions for test setup
- Clear test naming (except for the one issue noted above)
This mimics the natural deployment mechanism
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ProverManager/CurrencyScenario.t.sol (1)
40-43: Clarify the setUp() design decision.The comment about not calling
super.setUp()is intentional but could be confusing for future maintainers. Consider adding more context about why this design choice was made.function setUp() public virtual override { token = new MockERC20(); - // do not call super.setUp() because it will be called by the Test contract + // Note: super.setUp() is intentionally not called here as it will be called + // by the concrete Test contract that inherits from this currency contract }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/ProverManager/CurrencyScenario.t.sol(1 hunks)
🔇 Additional comments (4)
test/ProverManager/CurrencyScenario.t.sol (4)
1-9: LGTM: Clean imports and header setup.The SPDX license, pragma version, and imports are properly structured for the test file.
10-35: LGTM: ETHCurrency implementation is well-structured.The ETH-specific implementation correctly:
- Uses
vm.dealto fund accounts with ETH- Deploys
ETHProverManagerwith the initial deposit value- Handles ETH deposits and balance queries appropriately
- Skips preparation for deposits since ETH doesn't require approval
45-72: Excellent improvement: CREATE2 deployment addresses previous concerns.The implementation now uses CREATE2 deployment with a salt, which addresses the fragile fixed-address deployment concern from previous reviews. This approach:
- Provides deterministic, collision-resistant addresses
- Preserves normal constructor semantics
- Eliminates the risk of address collisions
The token minting and approval flow is correctly implemented before deployment.
82-95: LGTM: ERC20-specific operations are correctly implemented.The ERC20 implementation properly handles:
- Token approval before deposits
- ERC20 deposit calls to the ProverManager
- Token balance queries
The logic is sound and follows expected ERC20 patterns.
This creates a state structure for tests, using the same intuition as #85, except it uses the latest version of the prover manager (which involves running tests against the
ETHProverManagerandERC20ProverManager).I'm mostly just copying and tweaking the tests from #85, and from the existing tests on main (in the *.outdated and *.suppressed files, which I will remove as they are consumed). It's incomplete but I thought it makes sense to push the current version to deprecate #85, and also so you can see the structure if desired.
In the process I've also made some improvements/bug fixes to the actual code:
claimProvingVacancyinstead)scaleByPercentagefunction (instead ofscaleBy)bidfunction continues working on a period that has theoretically started but has not been triggered by a publication (so it's still the "next period")ProverManagerConfigand actually made those default values so I could make the two prover managers concrete classes. Otherwise, there is no prover manager in our code base that can be deployed. I also added getters for the config parameters.ProverVacancyClaimedevent (note that it is called from the constructor as well).I should finish copying and expanding the test suites tomorrow.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Documentation
Tests