feat: challenge protocol update #440
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request restructures core challenge and assertion management within the Colosseum ecosystem. The Colosseum contract now relies on the IL2OutputOracle interface and features new functions (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant A as Asserter
participant C as Colosseum
participant Ch as Challenger
A->>C: createAssertion(outputIndex, asserter)
Note right of C: Check for existing assertion\nStore new assertion data\nEmit AssertionCreated event
C-->>A: Acknowledgement
Ch->>C: createChallenge(outputIndex, l1BlockHash, l1BlockNumber)
Note right of C: Verify assertion status\nUpdate challenge status\nEmit ChallengeCreated event
C-->>Ch: Acknowledgement
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (37)
packages/contracts/contracts/test/L2OutputOracle.t.sol (1)
41-43: Constructor parameters updated to use explicit address type casting.The L2OutputOracle constructor now accepts addresses instead of contract instances for
_validatorPooland_validatorManager. This change promotes a more interface-based approach, reducing tight coupling between contracts.This shift to accepting addresses rather than contract instances is a good architectural practice that facilitates:
- Easier upgradability
- Better separation of concerns
- More flexibility in contract interactions
Also applies to: 55-57, 69-71
packages/contracts/contracts/libraries/Types.sol (1)
194-209: Added AssertionStatus enum to track assertion state.The AssertionStatus enum provides a formal way to track the lifecycle state of assertions through well-defined states (IN_PROGRESS, ACCEPTED, REJECTED, RESTORED).
Note: There's a TODO comment at line 194-195 about fixing the URL. This should be addressed before finalizing.
Remember to update the TODO comment with the correct URL reference before finalizing this PR.
packages/contracts/contracts/L1/KromaPortal.sol (1)
388-389: TODO comment should be addressed.There's a TODO comment questioning why a specific check is necessary. This should be addressed or clarified before finalizing the PR.
Consider addressing the TODO comment with an explanation of why the finalization period check is necessary, or remove it if it's been determined to be required.
packages/contracts/contracts/test/ValidatorManager.t.sol (7)
4-12: Ensure imports are consistent with project structure.
All newly introduced imports appear related and essential. Confirm no unused imports remain, especially after interface-based refactoring.
16-17: Constructor parameter type shift toaddress.
Passing addresses instead of direct contract references can improve flexibility but reduces compile-time type safety. Ensure consistent usage throughout the codebase and consider interface-based calls where appropriate for extra clarity.
173-180: Use ofvm.mockCallfor finalization check.
MockingisFinalizedhelps isolate logic in tests, but be cautious: if test coverage needs to reflect real finalization constraints, ensure that separate integration tests or end-to-end checks exist.
443-448: Repeated mock calls for “isFinalized”.
Consolidate repeated stubs into helper functions if possible, to reduce duplication. This keeps tests cleaner and maintains consistency if the finalization logic changes.
494-498: Large inline mock calls in loop.
Repeated calls tovm.mockCallwithin loops can be more maintainable if moved to a utility method to avoid logic duplication and reduce the chance of missing updates if signatures change.
1079-1080: Addresses passed toMockL2OutputOracleconstructor.
Check thataddress(pool)andaddress(valMgr)are non-zero. Revisit boundary conditions if either contract is not yet deployed or incorrectly set.Do you want extra checks on constructor calls to revert on zero addresses?
1118-1123: Mocking finalization before MPT transition.
For large transitions (like MPT index updates), confirm that re-mockingisFinalizedwon't mask actual finalization logic. Consider adding negative tests to ensure unstoppable finalization is not prematurely bypassed.packages/contracts/contracts/test/Colosseum.t.sol.bak (2)
356-500: Challenger logic for_proveFaultwith ZK proofs.
The code referencesZkEvmTestDatastructures. Confirm they are always valid, and that the underlying proof method aligns with your production ZK library. Unexpected or malicious data states could skip normal checks.
1159-1685:Colosseum_ValidatorSystemUpgrade_TestandColosseum_MptTransition_Testsections.
Upgrading the validator system mid-challenge can be risky. Carefully test partial migrations or uninitialized state. Confirm the newmptFirstOutputIndexrestrictions are thoroughly validated.Do you want a separate suite of upgrade tests specifically checking partial-challenge scenarios?
packages/contracts/contracts/test/ValidatorPool.t.sol (8)
4-12: Imports refactored for addresses and interface usage.
Switching to address-based arguments can streamline constructor calls, but it removes compile-time type checks. Confirm all references are correct and can’t inadvertently be zero.
113-114: Upgrade logic forMockL2OutputOracle.
This approach aligns with the new constructor signature. Verify that the addresses passed in are not in a partially constructed state, especially if this upgrade is repeated multiple times.
140-140: Repeated deposit pattern:pool.deposit{value: requiredBondAmount}().
Multiple calls spread across tests. Consider centralizing deposit logic in a helper function for easier updates if deposit conditions change.
164-164: Double deposit scenario.
Line 164 shows another deposit with a potentially large amount. Ensure test coverage for integer overflow if deposit sums are huge.
175-175: Deposit from user with zero balance check.
Ifasserterortrustedhas zero balance, test would revert. Confirm that test scenarios intentionally cover an out-of-funds address for negative testing.
407-407: Looped deposit for multiple outputs.
Large loops might affect gas usage in a real scenario. Check that the test accurately represents a realistic chain environment.
659-661: Comparative deposit calls fortrustedandasserter.
Demonstrates difference in who can become a validator. Validate that partial deposit forassertercannot pass the threshold.
810-810: Depositing entiretrusted.balanceagain.
Double-check final state if the user invests all funds. The line ensures no validator role changes remain pending.packages/contracts/contracts/L1/L2OutputOracle.sol (2)
464-474: EnhancedisFinalizedfunction
The function now checks_outputIndex == 0as always finalized, then consults the Colosseum for assertion details. The time-based finalization logic appears correct, but consider caching or referencing constants from Colosseum to reduce repeated calls.
477-482: New setter for latest finalized output index
Restricts the caller to the Colosseum contract. The revert message "replace an output" might be slightly unclear given the function name "setLatestFinalizedOutputIndex."- "L2OutputOracle: only the colosseum contract can replace an output" + "L2OutputOracle: only the Colosseum contract can set the latest finalized output index"packages/contracts/contracts/test/CommonTest.t.sol (1)
328-329: Zero addresses in constructor parameters
Accepting_colosseumand_zkMerkleTrieas address(0) might be placeholders for future assignment. Confirm they are not mistakenly used before initialization.packages/contracts/contracts/test/Colosseum.t.sol (3)
514-514:Cancel challenge by forced bisectcall
Callingbisect(..., 0, 0)to cancel is somewhat non-intuitive. Consider a more descriptive function or parameter usage.
602-602: Re-invokingtest_proveFaultWithZkVm_succeeds()
Multiple calls in test logic can be helpful but watch for potential state pollution or repeated side effects.
645-645: Another call totest_proveFaultWithZkVm_succeeds()
Using repeated calls for testing can confirm re-entrancy safety or detect unexpected state carryover, but it may also complicate the test flow if not carefully managed.packages/contracts/contracts/L1/Colosseum.sol.bak (3)
16-21: Consider constraining large constant definitions and verifying time units.The contract defines multiple constants (e.g.,
TURN_INIT,DELETED_OUTPUT_ROOT) and includes numerically large timeout periods such asCREATION_PERIOD_SECONDS,BISECTION_TIMEOUT,PROVING_TIMEOUT. Consider documenting what these values mean in more detail, especially if they can be overridden, and confirm they align with time units or block intervals. Large constants should be double-checked for potential overflow in other contexts.
53-76: Scrutinize external calls for potential reentrancy or error handling.The constructor uses immutable references to
L2OutputOracle,ZKProofVerifier, and sets_segmentsLengthsviainitialize(_segmentsLengths). While this appears safe, always ensure that external calls (e.g., ininitialize) cannot introduce unexpected state changes or reentrancy. If needed, consider adding appropriate checks or the use of a reentrancy guard to be consistent with best practices when calling external components.
700-730: Assess fault proof logic for completeness and follow best practices for verifying proofs.The
_proveFaultfunction delegates zero-knowledge verification toZK_PROOF_VERIFIER. Ensure that:
- The derived
publicInputHashcaptures all data needed to validate the dispute.- The code reverts for invalid or tampered proofs as intended.
- Interval checks (e.g.,
timeoutAt) are thoroughly tested, especially around boundaries.Consider adding explicit checks or events reflecting the reason for proof acceptance or rejection to enhance transparency and traceability.
packages/contracts/contracts/L1/Colosseum.sol (2)
29-44: Document and confirm guardian-related timeouts.Lines 25-27 introduce
MAX_CLOCK_DURATION = 3.5 daysandGUARDIAN_PERIOD = 3.5 days, presumably to control how long the asserter or challenger has to act. Ensure the logic combining these windows (guardian period vs. max duration) is consistent with the intended challenge flow. Document the meaning behind using 3.5 days and confirm no negative or zero-edge scenarios occur if block timestamps differ unexpectedly.
723-730: Better error messaging for proof validity failures.In
_proveFault, if an invalidsrcOutputis found, the code reverts withInvalidPublicInput(). Consider providing more context or reverts that differentiate between a mismatch in segments vs. other proof validation errors. Clearer error messages can expedite debugging, especially in complex challenge flows.packages/contracts/contracts/L1/interfaces/IColosseum.sol (1)
8-13: Validate proof structure in interface methods.The
ZkVmProofstruct includeszkVmProgramVKey,publicValues, andproofByteswith no explicit size constraints. If these fields can be arbitrarily large, you may want to clarify or enforce maximum sizes within the implementation to avoid excessive gas usage or malicious inputs when used on-chain.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (2)
2-2: Consider using a caret version for the Solidity pragma.
Fixing the version strictly to0.8.15can limit forward compatibility and exclude any future minor or patch releases. Using^0.8.15(or a range that meets your needs) can help ensure you benefit from ongoing compiler optimizations and bug fixes.
10-14: Enhance event metadata for easier debugging.
TheOutputReplacedevent captures the new output root and submitter but omits the old output root. Adding a parameter for the replaced output root (e.g.,_oldOutputRoot) can aid in debugging and historical audits.packages/contracts/contracts/L1/interfaces/IValidatorPool.sol (3)
2-2: Adopt a caret version for the Solidity pragma.
Similarly to the other interface, using^0.8.15(or a suitable range) provides more flexibility than pinning the compiler version to0.8.15.
8-15: Review event parameter completeness.
The events cover important fields, but consider whether capturing additional data (such as timestamps or additional details about bond changes) would help off-chain services or explorers more effectively track and debug validator pool behaviors.
45-45: Ensure implementing contracts handle reentrancy protections for deposits.
Sincedeposit()ispayable, the implementing contract should guard against potential reentrancy and properly account for deposited funds. Consider using modifiers likenonReentrantor a checks-effects-interactions pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/contracts/contracts/L1/AssetManager.sol(9 hunks)packages/contracts/contracts/L1/Colosseum.sol(18 hunks)packages/contracts/contracts/L1/Colosseum.sol.bak(1 hunks)packages/contracts/contracts/L1/KromaPortal.sol(9 hunks)packages/contracts/contracts/L1/L1CrossDomainMessenger.sol(2 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(7 hunks)packages/contracts/contracts/L1/ValidatorManager.sol(3 hunks)packages/contracts/contracts/L1/ZKProofVerifier.sol(3 hunks)packages/contracts/contracts/L1/interfaces/IColosseum.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IValidatorPool.sol(1 hunks)packages/contracts/contracts/echidna/FuzzKromaPortal.sol(3 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/AssetManager.t.sol(4 hunks)packages/contracts/contracts/test/Colosseum.t.sol(27 hunks)packages/contracts/contracts/test/Colosseum.t.sol.bak(1 hunks)packages/contracts/contracts/test/CommonTest.t.sol(15 hunks)packages/contracts/contracts/test/KromaPortal.t.sol(8 hunks)packages/contracts/contracts/test/L1ERC721Bridge.t.sol(3 hunks)packages/contracts/contracts/test/L2OutputOracle.t.sol(6 hunks)packages/contracts/contracts/test/ValidatorManager.t.sol(11 hunks)packages/contracts/contracts/test/ValidatorPool.t.sol(18 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol(2 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol.bak(1 hunks)
🔇 Additional comments (103)
packages/contracts/contracts/L1/ValidatorManager.sol (2)
4-14: Consistent formatting applied to import statements.The removal of spaces between the imported entity names and the braces (
import {Math}instead ofimport { Math }) improves consistency across the codebase.
683-683: Comment indentation adjusted.The comment alignment has been adjusted but maintains the same meaning and purpose.
packages/contracts/contracts/L1/L1CrossDomainMessenger.sol (2)
4-7: Consistent formatting applied to import statements.The import statements have been reformatted to remove spaces between the imported entity names and their braces, maintaining consistency with other files in the codebase.
54-54: Space removed in value parameter.The space in the call to
depositTransaction{value: _value}has been removed, which is a cosmetic change that doesn't affect functionality.packages/contracts/contracts/L1/ZKProofVerifier.sol (4)
5-15: Consistent formatting applied to import statements.Import statements have been reformatted to remove spaces between the imported entity names and their braces, which improves consistency across the codebase.
203-204: Array slice notation formatting.The spacing in array slice notation has been adjusted to include spaces around the colon, changing from
publicValues[8:40]topublicValues[8 : 40], which improves readability.
209-209: Array slice notation formatting.Similar to the previous changes, the spacing in array slice notation has been adjusted to include spaces around the colon, maintaining consistency.
281-286: Parameter indentation adjusted.The parameters in the function call to
verifyInclusionProofhave been adjusted for improved readability.packages/contracts/contracts/L1/AssetManager.sol (6)
4-12: Consistent formatting applied to import statements.Import statements have been reformatted to remove spaces between the imported entity names and their braces, which improves consistency with other files in the codebase.
382-382: For-loop syntax spacing adjusted.The space before the loop increment expression has been removed, which is a cosmetic change that doesn't affect functionality.
452-452: For-loop syntax spacing adjusted.Similar to the previous for-loop, spaces before the increment expression have been removed for consistency.
Also applies to: 471-471
682-684: Mathematical expression formatting.The mathematical expression has been reformatted to include spaces around the operators for improved readability.
699-701: Mathematical expression formatting.The mathematical expression has been reformatted to include spaces around the operators for improved readability, maintaining consistency with previous changes.
794-798: Comment indentation adjusted.Comments have been reformatted with consistent indentation to improve readability while maintaining the same meaning.
Also applies to: 837-843, 857-862
packages/contracts/contracts/echidna/FuzzKromaPortal.sol (3)
3-8: Consistent formatting applied to import statements.Import statements have been reformatted to remove spaces between the imported entity names and their braces, maintaining consistency with other files in the codebase.
49-49: Space removed in value parameter.The space in the call to
depositTransaction{value: _mint}has been removed, which is a cosmetic change that doesn't affect functionality.
33-34:✅ Verification successful
Added Colosseum parameter to KromaPortal initialization.
A new parameter
_colosseum: address(0)has been added to the KromaPortal constructor, which indicates integration with a new Colosseum contract. This aligns with the PR objective to modify the bisection process and update criteria for output finalization.Since this is a test file, the default value of
address(0)is appropriate for the test environment. In production deployments, a proper Colosseum contract address should be provided.
🏁 Script executed:
#!/bin/bash # Check if KromaPortal contract now requires a Colosseum parameter grep -A 20 "constructor(" packages/contracts/contracts/L1/KromaPortal.sol | grep -i colosseumLength of output: 166
Action: Confirmed integration of the Colosseum parameter.
The new
_colosseumparameter is now correctly required by the KromaPortal constructor and is properly assigned to theCOLOSSEUMstate variable, as verified from the output inpackages/contracts/contracts/L1/KromaPortal.sol. The test file (FuzzKromaPortal.sol) appropriately usesaddress(0)for this parameter in the test environment, while production deployments should provide a valid Colosseum contract address.
- Confirmed that the constructor in KromaPortal accepts
address _colosseumand assignsCOLOSSEUM = IColosseum(_colosseum).- Verified that the test file uses a safe default (
address(0)).packages/contracts/contracts/test/L1ERC721Bridge.t.sol (4)
4-4: Style update for import statementsThe import format has been updated to use a more compact style without spaces inside the curly braces, which is consistent with other files in the project.
6-8: Style update for import statementsThe import format has been updated to use a more compact style without spaces inside the curly braces, which is consistent with other files in the project.
74-83: Formatting improvement for improved code readabilityThe function parameters for
abi.encodeCall(L2ERC721Bridge.finalizeBridgeERC721, ...)have been reformatted for better readability. This change is purely cosmetic and doesn't affect functionality.
163-165: Simplified parameter formattingThe parameters for
abi.encodeCall(L2ERC721Bridge.finalizeBridgeERC721, ...)have been consolidated into a more compact format. This is a cosmetic change that maintains the same functionality while improving code readability.packages/contracts/contracts/test/AssetManager.t.sol (4)
4-5: Style update for import statementsThe import format has been updated to use a more compact style without spaces inside the curly braces, which is consistent with other files in the project.
7-13: Style update for import statementsThe import format has been updated to use a more compact style without spaces inside the curly braces, which is consistent with other files in the project.
25-33: Parameter alignment for improved readabilityConstructor parameters for
AssetManagerhave been vertically aligned for better readability, following a consistent code style pattern. This change is cosmetic and maintains the same functionality.
103-103:❓ Verification inconclusive
Commented out time warping code
The line that warps the test to the time when the output is finalized has been commented out. This could affect test behavior, as it prevents the test from advancing time to when the output becomes finalized.
This might lead to timing-related test failures. Consider whether this was intentional or if it should be uncommented. Let's verify if other tests that depend on this timing behavior are still passing:
🏁 Script executed:
#!/bin/bash # Check if any tests related to AssetManager or output finalization are failing # Run tests specifically targeting AssetManager to see if they pass cd packages/contracts forge test --match-path "**/AssetManager.t.sol" -vLength of output: 172
Action Required: Verify Timing Behavior in AssetManager Tests
The commented-out time warping code (
// vm.warp(oracle.finalizedAt(terminateOutputIndex));) could lead to timing-related failures in tests that depend on advancing to the output finalization time. However, our initial test check did not produce meaningful results because the testing environment lacked theforgecommand.
- Issue: The absence of the time warp may impact tests that assume the state advances to a finalized output.
- Next Steps:
- Manual Verification: Please run the tests locally or in an environment where
forgeis available to confirm if tests relying on time advancement are passing.- Decision Point: If tests fail due to timing issues, consider uncommenting the time warp line or adjusting the test logic to account for the current state.
packages/contracts/contracts/test/mock/MockColosseum.sol (4)
4-6: Style update for import statementsThe import format has been updated to use a more compact style without spaces inside the curly braces, which is consistent with other files in the project.
10-10: Parameter type change from L2OutputOracle to addressThe parameter type for
_l2Oraclehas been changed fromL2OutputOracletoaddress. This change aligns with the interface-based approach mentioned in the PR objectives, making the code more flexible by accepting any address that implements the required interface.
33-33: Added whitespace at end of functionA blank line has been added after the
setL1Headfunction, which is a minor styling improvement.
15-20:❓ Verification inconclusive
Simplified constructor with fewer parameters
The constructor parameters passed to the parent
Colosseumcontract have been significantly reduced. The following parameters have been removed:
_creationPeriodSeconds_bisectionTimeout_provingTimeout_segmentsLengthsThis change reflects a major simplification of the Colosseum contract's initialization, which aligns with the PR's objective to modify the bisection process.
Let's verify that these parameters are now properly handled in the updated Colosseum contract:
🏁 Script executed:
#!/bin/bash # Check how the Colosseum contract handles these parameters now # Look at the updated Colosseum contract constructor cat contracts/L1/Colosseum.sol | grep -A 20 "constructor" # Check if these parameters are now handled differently, e.g., as constants cat contracts/L1/Colosseum.sol | grep -E "creationPeriod|bisectionTimeout|provingTimeout|segmentsLengths"Length of output: 283
Attention: Manual Verification of Parent Contract Location Required
The changes in the
MockColosseumconstructor correctly simplify the call to the parentColosseumcontract by removing the parameters_creationPeriodSeconds,_bisectionTimeout,_provingTimeout, and_segmentsLengths. However, our first verification attempt failed to locate the updatedColosseum.solfile at the expected path (contracts/L1/Colosseum.sol).
- Confirm that the updated
Colosseum.solfile exists and verify its new location if it has been moved or renamed.- Ensure that the constructor in the parent contract now handles the reduced parameter set as intended.
- Adjust the verification commands or file path as necessary once the correct location is determined.
packages/contracts/contracts/test/mock/MockColosseum.sol.bak (1)
1-42: This is the backup file of the previous MockColosseum.sol implementationThis file appears to be the original version of MockColosseum.sol before the changes were made. It shows the previous implementation with all the parameters that have now been removed in the updated version.
The main differences are:
_l2Oraclewas typed asL2OutputOracleinstead ofaddress- Additional parameters were included:
_creationPeriodSeconds_bisectionTimeout_provingTimeout_segmentsLengthsThis file provides a good reference for understanding the changes made to the updated MockColosseum contract.
packages/contracts/contracts/test/L2OutputOracle.t.sol (3)
4-15: Import statements are now more consistent with space removal and organization.The import statements have been reformatted for consistency, removing spaces between curly braces, and adding the ZKProofVerifier import. This improves code style uniformity across the codebase.
23-23: Spacing standardized in function call.The spacing in the
pool.depositfunction call has been standardized by removing spaces inside the curly braces.
430-430: Spacing standardized in function call.Consistent with earlier changes, the spacing in the
pool.depositfunction call has been standardized.packages/contracts/contracts/test/KromaPortal.t.sol (6)
8-8: Added import for IColosseum interface.The IColosseum interface import has been added, which will be used for interacting with the Colosseum contract via its interface rather than through direct dependency.
315-336: Test refactored to use Assertion structure instead of CheckpointOutput.The test has been refactored to:
- Use the new Types.Assertion structure instead of CheckpointOutput
- Mock COLOSSEUM.getAssertion instead of L2_ORACLE.getL2Output
- Add documentation about index 0 being always finalized
- Use a consistent finalizationPeriod variable
This change aligns the test with the architectural shift to use the Colosseum contract for managing assertions and their finalization status.
340-357: Test improved to handle repeated output submission.The test has been updated to accommodate the fact that index 0 outputs are always finalized by adding code to submit multiple outputs and validate against different output indices.
418-427: Added process to repeat output submission in setUp.Similar to the previous change, this ensures proper test setup by accounting for the fact that output at index 0 is always finalized.
805-839: Test renamed and updated to focus on output finalization rather than timestamp.The test has been renamed from checking timestamp-based finalization to checking overall output finalization status. The implementation now uses an Assertion structure and mocks COLOSSEUM.getAssertion instead of L2_ORACLE.getL2Output.
1002-1006: Added mock calls to oracle.isFinalized method.Mock calls for oracle.isFinalized have been added to properly test the updated finalization mechanism which now relies on a dedicated method rather than timestamp calculation.
Also applies to: 1082-1086
packages/contracts/contracts/libraries/Types.sol (3)
149-158: Added Challenge struct to represent the challenge state.The Challenge struct has been introduced to track the details of a challenge, including:
- Tracking turns and time constraints for asserters and challengers
- Recording segment information for bisection
- Maintaining participant addresses and timestamps
This structure provides a comprehensive way to manage the challenge lifecycle and state.
160-167: Added Segment struct for managing bisection details.The Segment struct has been introduced to track segmentation information critical for the bisection protocol during challenges:
- Output values
- Position indicators
- Start and end points
- Boundary output values
This addition provides a clear structure for organizing bisection data.
180-189: Changed from Challenge to Assertion struct with enhanced fields.The former Challenge struct has been renamed to Assertion and expanded with additional fields to track:
- Starting block number and output root
- Timestamps for assertion, acceptance, and rejection
- Status tracking via enum
- Challenge count
This change represents a shift in the protocol design toward a more comprehensive assertion lifecycle management approach.
packages/contracts/contracts/L1/KromaPortal.sol (5)
4-18: Imports reorganized and updated to include IColosseum.The imports have been reorganized for better readability and the IColosseum interface has been added to support the new assertion-based architecture.
72-76: Added COLOSSEUM immutable reference.A new immutable reference to the IColosseum contract has been added, allowing the KromaPortal to interact with the Colosseum contract through its interface.
157-161: Added DeletedOutput error type.A new error type
DeletedOutputhas been introduced to handle scenarios where a withdrawal transaction references a deleted output. This improves error reporting specificity.
192-194: Updated constructor to accept Colosseum address.The constructor has been updated to accept an additional parameter for the Colosseum contract address, and initializes the COLOSSEUM immutable variable.
Also applies to: 200-200
543-549: Refactored isOutputFinalized to use internal helper method.The isOutputFinalized function has been refactored to use an internal helper method _isOutputFinalized that delegates to L2_ORACLE.isFinalized. This design:
- Improves code organization
- Reduces duplication
- Makes the code more maintainable
- Enables consistent finalization checks throughout the contract
This change represents a shift from timestamp-based finalization checks to delegating this responsibility to the L2 Oracle contract.
packages/contracts/contracts/test/ValidatorManager.t.sol (4)
148-149: Constructor calls with addresses.
The usage ofaddress(pool)andaddress(valMgr)matches the constructor’s updated signature. No issues spotted, but confirm that each address is valid and not zero at runtime.
169-169: Check deposit invocation for potential re-entrancy or partial refunds.
When callingpool.deposit{value: trusted.balance}(), ensure that user balances won't exceedMAX_UNBONDin subsequent logic if immediate unbonds are triggered.
475-480: Finalization mocking for next output index.
Ensure that this mapping of(terminateOutputIndex + 1) -> trueis undone or tested with “false” paths as well. Edge scenarios might be missed if only the success path is stubbed.
1073-1073: Upgrading oracle address insetUp.
Be sure to verify the correctness of the newly assignedMockL2OutputOracle. If the contract is replaced after partial test runs, tests might carry unexpected state.packages/contracts/contracts/test/Colosseum.t.sol.bak (5)
1-49: Overall file introduction and imports.
This entire file is newly added. The imports and contract definitions appear consistent with the challenge/fault-proving logic. Verify that no cyclical references exist across newly introduced dependencies.
121-240: Bisection and segments logic.
The_bisectand_newSegmentslogic is central to fault proving. Validate for integer division edge cases (e.g., off-by-one in segment size, or zero segment length). Ensure that large block ranges do not cause integer overflow.
241-355: Ready-to-prove states and event emission.
The code callsemit ReadyToProveon final bisection. Double-check that the event is fired only once per challenge, and that subsequent calls fail or revert.
501-700: Output replacement and slash flows.
Slash and revert slash flows are heavily reliant on external addresses. Confirm newdeletedOutputsmapping usage and event consistency. Watch out for partial reverts that could lead to half-updated states.
901-1158: Security council overrides.
dismissChallengeandforceDeleteOutputinvolve the security council. Confirm robust multi-sig or no single-guardian bypass is possible. Test reentrancy or repeated calls in the same block.packages/contracts/contracts/test/ValidatorPool.t.sol (9)
17-18: Constructor parameters nowaddress _validatorPool, address _validatorManager.
Mirrors the shift away from typed contract references. Validate that these addresses are properly assigned to internal storage or used solely in the base constructor.
26-35: Forwarding parameters toL2OutputOracle.
No immediate issues detected. Confirm naming consistency across the codebase so that_colosseum,_l2BlockTime, etc., are always used or stored as intended.
147-147: Asserter deposit.
This deposit tests whether asserter can also become a validator. Watch out for collisions if asserter and trusted are the same address.
305-305: Mixed interaction logic:.addOutputthen.deposit.
Be sure deposit reverts or passes are tested thoroughly when called in the middle of an output submission.
545-545: Pending bond deposit.
Line 545 indicates a deposit specifically to cover bond amounts. Confirm separate tests exist for partial bond coverage or invalid bond size.
650-650: Deposit of 1 Wei.
Tests minimal deposit usage. IfrequiredBondAmountis larger, this deposit shouldn’t make the user a validator. The test logic is correct if it simply verifies the final balance.
670-678: Multiple deposit calls with variable amounts.
Line 670, 674, and 678 each deposit different amounts. Good coverage for boundary conditions. Confirm tests for deposit > requiredBondAmount are thorough.
769-770: Upgrading toMockL2OutputOraclewith address references.
These lines match the pattern in other files for passing addresses. Ensure that the upgrade step is tested for re-initialization or unexpected collisions.
805-805: Depositing after system upgrade disallowed.
The revert message ensures no new deposits post-termination. Confirm that partial deposits or leftover balances are handled gracefully.packages/contracts/contracts/L1/L2OutputOracle.sol (5)
4-4: Add relevant imports
The new imports (Initializable, Constants, Types, ISemver, IValidatorManager, IValidatorPool, IColosseum) appear valid and consistent with the contract's usage.Also applies to: 6-11
22-24: Marking the validator pool as immutable
DefiningVALIDATOR_POOLas an immutable reference toIValidatorPoolis a solid approach that prevents accidental reassignments while retaining upgrade flexibilities.
27-27: Added doc comment
The new comment clarifies the role of the validator manager in the contract, improving documentation.
73-77:✅ Verification successful
Introduced new state variable
latestFinalizedOutputIndex
This variable tracks the latest finalized output index. Ensure that any assumptions about zero-based or large indices are handled carefully.Please verify how
latestFinalizedOutputIndexis used in related contracts to avoid off-by-one or boundary logic issues. You can run:
🏁 Script executed:
#!/bin/bash rg 'latestFinalizedOutputIndex' -A 5Length of output: 2013
Verification Complete: Usage of
latestFinalizedOutputIndexis consistent across the contracts.After running the verification, we observed that:
- The variable is declared in
packages/contracts/contracts/L1/L2OutputOracle.soland exposed via theIL2OutputOracleinterface.- In
Colosseum.sol, it is retrieved and used to computenextFinalizedOutputIndex(via adding 1), which aligns with the assumption that the genesis output (index 0) is already finalized.The current implementation appears to correctly adopt a zero-based index model. However, please continue to monitor boundary conditions—especially as the index value grows—to ensure that the off-by-one handling in loops (e.g., the iteration over finalized outputs) remains robust.
279-281:❓ Verification inconclusive
Trigger assertion creation in Colosseum
InvokingIColosseum(COLOSSEUM).createAssertionduring L2 output submission tightly couples this Oracle with the Colosseum. Check that no external-call re-entrancy or error-handling issues arise from this approach.Confirm that
createAssertioncalls handle possible reverts or error conditions gracefully:
🏁 Script executed:
#!/bin/bash rg 'createAssertion(' -A 5Length of output: 111
I'll generate a corrected shell script to re-run the search with proper escaping for the parentheses. This should provide a clear output so that we can assess how
createAssertionis used and whether it has proper error-handling.
🏁 Script executed:
#!/bin/bash rg 'IColosseum\(COLOSSEUM\)\.createAssertion\(' -A 5Length of output: 578
🏁 Script executed:
#!/bin/bash rg 'try\s+IColosseum' -A 5Length of output: 28
Action Required: Review External Call Error Handling & Re-entrancy Controls
The call to
IColosseum(COLOSSEUM).createAssertion(outputIndex, msg.sender);is made directly without a surrounding try-catch or any explicit error-handling mechanism. As a result:
- If
createAssertionreverts due to an error, the entire transaction will revert.- There is no indication that re-entrancy guard measures (e.g., using
nonReentrant) are applied around this area.Please verify manually that:
- This direct call is intended to propagate any revert (i.e., failing fast is desired).
- The Colosseum contract’s
createAssertionmethod is secure against re-entrancy.- Additional error-handling (or try-catch wrappers) is not required given the design choices for L2 output submission.
packages/contracts/contracts/test/CommonTest.t.sol (2)
5-46: New import statements
These large blocks of new imports provide test utilities, governance, bridging, and universal libraries. The imports match the usage in subsequent tests.
213-213: InstantiatingL2ToL1MessagePasser
This strongly typed usage for the predeployed address clarifies contract references and ensures type safety.packages/contracts/contracts/test/Colosseum.t.sol (27)
4-18: Extensive new import statements
These imports fromTypes,ISP1Verifier,IValidatorManager, etc. align with newly introduced test logic. References appear consistent.
53-57: Added constants for challenge phases
CHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEimprove readability and reduce magic numbers in the challenge state machine.
64-68: New challenge-related events
AssertionCreated,ChallengeCreated,ChallengerHasTimedOut, andChallengeCanceledare valuable for debugging and state tracing.
102-103: Deposits fortrustedandasserter
Explicitpool.depositcalls ensure these actors have the necessary stake. Helps keep the test scenario accurate.
119-120: Challenger deposits and_createAssertion()
Allowing new challengers to deposit and then initiate assertion logic covers essential challenge lifecycle steps.Also applies to: 122-122
130-130: UsingZkVmTestData.INVALID_BLOCK_NUMBER
This line references an invalid L2 block number from test data. Verify correctness against the rest of the test constants.
133-133: ReturningZkVmTestData.PREV_OUTPUT_ROOT
Indicates a specific scenario for previous root validation in tests. Looks consistent with the invalid block approach.
138-138: ReturningZkVmTestData.TARGET_OUTPUT_ROOT
Helps simulate the target root scenario in negative test cases.
152-171: Refined_detectFaultlogic
This segment-based approach checks whether the mismatch is atposor not and bisects accordingly. Double-check off-by-one logic in(start + 1 == end).
179-180: Challenger deposit
Collecting a bond from new challengers is consistent. Ensure the deposit does not exceed the test environment's constraints.
186-201: Private_createAssertionfunction
Good usage ofvm.expectEmitto verify events. Also, note the reliance onoracle.submitL2Outputsuccess for test completion.
279-279: Reference_doProveFault
CapturingpublicInputHashclarifies how the test tracks the final proof.
370-370:test_challengerTimeout_succeeds
Ensures that the challenger times out correctly and triggers theChallengerHasTimedOutevent.Also applies to: 383-385
388-388:test_challengerTimeout_revertsintroduction
Verifies that the challenge fails if tried too early, improving coverage of the time-based logic.
414-414:vm.prank(challenger)usage
Testing scenario for a wrong fork revert. The directprankis consistent with forging ephemeral calls.
421-421:block.number - 1for reorg
Ensures a mismatch in block references. This negative test approach is appropriate.
436-436: Newtest_bisect_withBadPos_reverts
Adds coverage for invalid segment positions in the challenge bisection logic.
446-451: Invalid position test logic
Declaresinvalid_positionand expectsColosseum.InvalidPosrevert. Solid negative test coverage.
458-459: Calculating position and output
Ensures we gather the next step of the bisection from_detectFault. Straightforward logic.
479-479: Callingtest_proveFaultWithZkVm_succeeds
Nests the fault-proof test within another scenario. Confirm repeated calls don’t conflict with shared state.
526-526: Setting L1 head toZkVmTestData.L1_HEAD
Explicitly simulating a fresh L1 head inMockColosseum. Helps ensure correct chain context for ZK proofs.
606-606: Creating emptyZkVmProof
Tests how the system handles a zero-data proof. This can be valuable for negative or boundary coverage.
608-608: CallingproveFaultWithZkVmwith empty proof
Passing an empty proof often indicates an edge case. Verify that this won't cause silent acceptance in the real contract.
626-626: CapturingpublicInputHash
Uses the returned value from the fault proof. Confirm that it doesn’t require additional checks for potential re-entry.
637-637: DestructuringdeletedOutputs
Checks partial returned fields from the mapping. Watch for any uninitialized or stale data usage.
686-686:publicInputHash = test_proveFaultWithZkVm_succeeds()
Capturing the result again for further checks is consistent, though repeated usage of the same approach might be consolidated in the future.
758-758: Final invocation oftest_proveFaultWithZkVm_succeeds()
Extends coverage so that a final scenario also verifies the proof. Ensures stability for repeated calls.packages/contracts/contracts/L1/Colosseum.sol.bak (1)
374-380: Ensure creation period logic is enforced correctly.The code checks if
targetOutput.timestamp + CREATION_PERIOD_SECONDS < block.timestampand reverts withCreationPeriodPassed(). Confirm that external calls do not bypass or incorrectly settargetOutput.timestamp. Consider adding unit tests around the boundary conditions of the creation period to ensure correct revert behavior for this time-based logic.packages/contracts/contracts/L1/Colosseum.sol (1)
423-426: Clarify assertion status checks.In
createChallenge, the code reverts ifassertion.status != Types.AssertionStatus.IN_PROGRESS. While this ensures that no challenge arises for a finalized or canceled assertion, ensure there's coverage for partial states or transitions. For instance, if an assertion times out but is not formally updated, consider whether its status remains stuck inIN_PROGRESS. Thoroughly test edge cases where multiple challenges may be created against the same assertion in short order.packages/contracts/contracts/L1/interfaces/IColosseum.sol (1)
14-21: Check alignment of ChallengeStatus with contract code.The enum
ChallengeStatushas six states. Confirm that all references in the coreColosseumcontract perfectly match the same index ordering here (e.g.,NONE=0, CHALLENGER_TURN=1, ASSERTER_TURN=2, ...). If a mismatch occurs, it can lead to subtle logic bugs. Confirm test coverage ensuring consistent usage.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (1)
58-60:❓ Verification inconclusive
Verify that access controls protect state-modifying functions.
Functions likereplaceL2OutputandsetLatestFinalizedOutputIndexcan alter critical contract states. Ensure the implementing contract enforces proper authorization to prevent unauthorized calls.
🏁 Script executed:
#!/bin/bash # Description: Verifies that state-modifying functions in IL2OutputOracle are protected by access controls. # We assume the actual implementing contract references IL2OutputOracle and defines these functions. # Step 1: Find implementing contracts that reference IL2OutputOracle fd -e sol --exec rg "IL2OutputOracle" # Step 2: Examine each implementation to confirm proper access checks using require(), onlyOwner modifiers, etc. # (You can manually inspect the matching lines to confirm that each function is guarded.)Length of output: 227
Action Required: Confirm Access Control Enforcement in Implementations
The interface file only defines the function signatures, so it’s critical to verify in the actual implementation contracts that state-changing functions—
replaceL2OutputandsetLatestFinalizedOutputIndex—are properly restricted (e.g., through authorized modifiers likeonlyOwneror equivalent require checks). The current automated check (searching for references toIL2OutputOracle) did not yield definitive evidence of access controls, so please manually confirm that the implementing contracts enforce these permissions.
- Verify that the implementations properly restrict these functions via access control modifiers or require statements.
- Review any contracts instantiating
IL2OutputOracleto ensure modifications to critical state are guarded.
|
View your CI Pipeline Execution ↗ for commit a1416c5.
☁️ Nx Cloud last updated this comment at |
7a58b49 to
c3f4194
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/contracts/contracts/libraries/Types.sol (1)
194-195: Fix the TODO comment about missing documentation URL.There's a TODO note about a missing URL for the state diagram documentation.
Update the URL in the TODO comment to point to the correct documentation for assertion states:
- // TODO fix the url - * See the https://specs.kroma.network/fault-proof/challenge.html#state-diagram + * See the https://specs.kroma.network/protocol/fault-proof/challenge.html#state-diagrampackages/contracts/contracts/test/Colosseum.t.sol (4)
76-82: Mock deployment with updated constructor parameters
The changes in constructor parameter formatting and upgraded proxy usage look correct. Ensure that all parameters align with the new constructor signature of the underlying contracts.Consider adding inline comments documenting each constructor parameter for clarity in large parameter lists.
Also applies to: 87-88
188-201:_createAssertionupdates
The function now includes event expectation (AssertionCreated) and a more structured approach to retrieve and submit the L2 output. This effectively tests the new assertion-creation flow inColosseum.You may wish to add negative test scenarios to confirm that creating an assertion with the same index reverts as expected.
308-315: Commented-out_dismissChallengehelper
The lines remain commented. If no longer necessary, consider removing them to reduce clutter, or add a TODO explaining why they remain.-// function _dismissChallenge(uint256 txId) private { -// ... -// } +// TODO: Revisit or remove if obsolete
510-510: Repetitive usage oftest_proveFaultWithZkVm_succeeds
This test is invoked to simulate an output replacement scenario. Ensure repeated usage doesn't overshadow more granular test coverage.Add dedicated tests for multiple replacements or repeated proof calls if not already present.
packages/contracts/contracts/L1/Colosseum.sol (2)
92-98: Legacy spacer variables
Keeping placeholder spacers for backward compatibility is a valid approach if they replace removed mappings or structs. Ensure these remain well-documented to avoid confusion.Add inline comments clarifying that these spacers preserve storage layout.
395-395: Emptyinitialize()
A placeholder for future upgrade patterns or initialization logic. If no additional logic is planned, it’s harmless but keep it documented that it’s intentionally empty.Add a comment clarifying its purpose:
function initialize() public initializer { + // Intentionally left empty for upgradeability / future expansions }packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (1)
4-7: Natspec documentation for public interfaces.
It might be helpful to add additional NatSpec comments above imports or the interface declaration to clarify usage expectations, especially if these are commonly used throughout the codebase.packages/contracts/contracts/L1/interfaces/IValidatorPool.sol (1)
1-3: Reuse or alignment of code comments.
Similar to other interfaces, consider explicitly mentioning the purpose of this interface to unify code readability across the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/contracts/contracts/L1/AssetManager.sol(9 hunks)packages/contracts/contracts/L1/Colosseum.sol(20 hunks)packages/contracts/contracts/L1/KromaPortal.sol(9 hunks)packages/contracts/contracts/L1/L1CrossDomainMessenger.sol(2 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(7 hunks)packages/contracts/contracts/L1/ValidatorManager.sol(3 hunks)packages/contracts/contracts/L1/ZKProofVerifier.sol(3 hunks)packages/contracts/contracts/L1/interfaces/IColosseum.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IValidatorPool.sol(1 hunks)packages/contracts/contracts/echidna/FuzzKromaPortal.sol(3 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/AssetManager.t.sol(4 hunks)packages/contracts/contracts/test/Colosseum.t.sol(27 hunks)packages/contracts/contracts/test/CommonTest.t.sol(16 hunks)packages/contracts/contracts/test/KromaPortal.t.sol(8 hunks)packages/contracts/contracts/test/L1ERC721Bridge.t.sol(3 hunks)packages/contracts/contracts/test/L2OutputOracle.t.sol(6 hunks)packages/contracts/contracts/test/ValidatorManager.t.sol(11 hunks)packages/contracts/contracts/test/ValidatorPool.t.sol(18 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/contracts/contracts/L1/L1CrossDomainMessenger.sol
- packages/contracts/contracts/echidna/FuzzKromaPortal.sol
- packages/contracts/contracts/L1/ZKProofVerifier.sol
- packages/contracts/contracts/L1/AssetManager.sol
- packages/contracts/contracts/L1/ValidatorManager.sol
- packages/contracts/contracts/L1/L2OutputOracle.sol
- packages/contracts/contracts/test/L1ERC721Bridge.t.sol
- packages/contracts/contracts/test/ValidatorPool.t.sol
- packages/contracts/contracts/test/L2OutputOracle.t.sol
- packages/contracts/contracts/L1/KromaPortal.sol
- packages/contracts/contracts/test/AssetManager.t.sol
- packages/contracts/contracts/test/CommonTest.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (78)
packages/contracts/contracts/test/mock/MockColosseum.sol (3)
4-6: Clean import formatting with braces.The import statements have been properly formatted to use braces, providing better consistency across the codebase.
10-15: Constructor parameter type refinement and new fields added.The constructor's parameter type for
_l2Oraclehas been changed fromL2OutputOracletoaddress, and two new parameters (_guardianPeriodand_maxClockDuration) have been added. This change enables better flexibility and integrates with the new protocol challenge design.
17-23: Updated parent constructor initialization.The parent constructor initialization correctly passes the parameters in the proper order, reflecting the updated parameter list in the Colosseum contract.
packages/contracts/contracts/test/KromaPortal.t.sol (8)
8-8: Added import 8000 for IColosseum interface.Added import for the new IColosseum interface, which is needed for the mock calls in the test file.
20-24: Added proper superclass setup call.The setUp function now correctly calls
super.setUp(), ensuring proper initialization from the parent class. This is a critical fix to prevent potential initialization issues.
316-324: Updated assertion verification to use new Types.Assertion structure.The test now uses the new Types.Assertion structure instead of directly interacting with output roots. This change aligns with the architectural shift from output-based to assertion-based verification.
327-336: Updated finalization checking logic.The output finalization logic has been updated to use a consistent finalizationPeriod variable and to properly check different output indices (index 0 is always finalized, testing index 1). This provides clearer testing semantics.
348-355: Added documentation for index 0 finalization and output testing logic.A comment has been added to explain that the output at index 0 is always finalized, requiring the test to submit a second output to properly test finalization behavior. This improves code clarity and maintainability.
805-830: Updated the isOutputFinalized test function to use assertion structure.The function has been renamed from
ifOutputTimestampIsNotFinalized_revertstoifOutputIsNotFinalized_revertsand now uses the Types.Assertion structure, improving code clarity and alignment with the new architecture.
1002-1006: Added mock call for oracle finalization.Added a mock call to simulate oracle finalization, replacing direct access to the oracle's finalization state. This is consistent with the interface-based approach being adopted.
1082-1086: Added mock call for oracle finalization in diff test.The same mock call pattern for oracle finalization is used in the differential test, ensuring consistent testing methodology throughout the file.
packages/contracts/contracts/libraries/Types.sol (4)
149-158: Added Challenge struct to support new assertion/challenge framework.The new Challenge struct includes fields for tracking turn-based challenge state, time management, and segment identification, providing a comprehensive structure for implementing the protocol's challenge mechanism.
160-167: Added Segment struct for bisection challenges.The new Segment struct defines the data structure needed to represent a segment in the bisection protocol, including the output, position, start/end bounds, and related outputs.
170-189: Renamed and enhanced Challenge struct to Assertion.The former Challenge struct has been renamed to Assertion and enhanced with additional fields for tracking assertion lifecycle and status, including acceptance, rejection, and challenge counts. This better represents the protocol's assertion-based model.
205-209: Added AssertionStatus enum for tracking assertion lifecycle.Added a well-defined enum (IN_PROGRESS, ACCEPTED, REJECTED, RESTORED) to represent the possible states of an assertion throughout its lifecycle. This provides type safety and clarity when tracking assertion status.
packages/contracts/contracts/test/ValidatorManager.t.sol (10)
4-12: Improved import formatting with consistent brace usage.The import statements have been properly formatted to use braces consistently, following project code style guidelines.
16-34: Updated MockL2OutputOracle constructor to use address types.The constructor parameters have been changed from contract types to address types, making the interface more flexible and consistent with the architectural changes in the parent contracts.
169-170: Formatting fix for deposit function call.Added curly braces for the value parameter in the deposit function call, improving code readability and consistency.
174-180: Replaced direct warp with mock call for finalization.Replaced the direct warping based on oracle.finalizedAt with a mock call to simulate the finalization state. This approach provides better test isolation and aligns with the interface-based testing approach.
401-406: Replaced warp with mock call for finalization in distribution test.Consistent with the previous change, this section replaces direct warping with a mock call to simulate output finalization in the distribution reward test.
474-480: Used mock call for finalization in the priority validator test.Similarly, this section uses a mock call to simulate output finalization in the priority validator update test.
815-821: Used mock call for finalization in the tryJail test.This section uses a mock call to simulate output finalization in the tryJail test, maintaining consistency with the testing approach throughout the file.
1079-1081: Updated MockL2OutputOracle constructor in MptTransition_Test.Updated the constructor parameters to use address types in the MptTransition_Test, consistent with the changes in the main test class.
1103-1104: Formatting fix for deposit function call in MptTransition_Test.Added curly braces for the value parameter in the deposit function call, improving code readability and consistency.
1118-1124: Replaced direct warp with mock call for finalization in MptTransition_Test.Similar to the main test class, this section replaces direct warping with a mock call to simulate the finalization state in the MptTransition_Test setup.
packages/contracts/contracts/test/Colosseum.t.sol (17)
4-18: Consistent import structure and references
All import statements have been updated or added consistently, referencing the correct paths for libraries, interfaces, mocks, and test data.
53-55: Clarity in constant definitions
DefiningCHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEclarifies state transitions in the test suit. These constants help improve readability.
62-66: New events align with underlying contract
The introduction ofReadyToProve,AssertionCreated,ChallengeCreated,ChallengerHasTimedOut,ChallengeCanceledensures the test contract is in sync with the updated events inColosseum.sol.
102-102: Deposits made by multiple roles
These lines deposit balances for trusted, asserter, and challenger. The logic is straightforward and appears correct.Also applies to: 104-104, 120-120
123-123: Extracted_createAssertion()for reusability
Replacing inlined logic with a private helper function_createAssertionis a good move for code reuse and clarity.
131-131: Usage ofZkVmTestDataconstants
References toINVALID_BLOCK_NUMBER,PREV_OUTPUT_ROOT, andTARGET_OUTPUT_ROOTprovide clarity in simulating invalid outputs.Also applies to: 134-134, 139-139
153-173: Refined_detectFaultfunction
This function splits logic by comparing_challenge.segment.outputto_getOutputRoot(...)and then setsstart/endaccordingly. Checking the boundary condition(start + 1 == end)is correct for concluding a final position. Division for midpoints also appears correct.
204-224:_createChallengeadjustments
The logic to emitChallengeCreatedupon callingcolosseum.createChallenge(...), then verifying the resulting challenge struct, is sound. It thoroughly checks new fields liketurn,updatedAt, andsegment.*.
238-267: Bisection testing
The_bisecthelper simulates multiple bisection steps, verifying time-left calculations. Integration with_detectFault(...)is thorough, and the final asserts confirm correctness of turn increments and updated segment outputs.
279-280: Additional fault detection calls
Checking_detectFaultusage in synergy with_doProveFaultensures correct detection of conflict points.
370-385: Test for successful challenger timeout
The test checks that afterchallenges[_outputIndex][challenger].challengerTimeLefthas elapsed, the asserter can callcolosseum.challengerTimeout(...). Event emission and final state are verified.
388-403: Timeout revert scenario
Verifies that we revert when the challenger attempts to time out a bit early (- 1). The usage ofvm.expectRevertwithColosseum.ImproperChallengeStatus.selectoris correct.
436-451:test_bisect_withBadPos_reverts
This test ensures that passing a position beyond the correct midpoint reverts withInvalidPos. It's consistent with the new bisection logic.
453-466:test_bisect_ifNotYourTurn_reverts
Verifies that only the correct participant can callbisectbased on whose turn it is. Properly reverts withNotAllowedCaller.
478-487: Reconciling challenge cancellation
After a different challenge replaces the output root, callingcolosseum.bisect(..., 0, 0)by the same challenger emitsChallengeCanceled. The steps are logically consistent with the new cancellation flow.
517-542:test_proveFaultWithZkVm_succeeds
Covers final output replacement, verifying that a correct ZK proof sets the challenge toNONEand overwrites the output.
548-566:test_proveFault_finalizedOutputnegative scenario
As expected, time progression beyond finalization triggersImproperChallengeStatus. The test ensures we can’t prove fault post-finalization.packages/contracts/contracts/L1/Colosseum.sol (17)
4-10: New imports for Initializable and interface usage
The shift to an interface-based approach (e.g., usingIL2OutputOracleinstead ofL2OutputOracle) and leveraging OpenZeppelin’sInitializableindicates a more modular design.
24-29: Use ofFINALIZATION_LOOP_LIMIT
Limiting iterations in finalization loops helps prevent excessive gas usage or potential DoS vectors. This is a good protective measure.
61-63: Immutable public variables for contract configuration
Storing constants likeMAX_CLOCK_DURATION,L2_ORACLE,L2_ORACLE_SUBMISSION_INTERVAL,SECURITY_COUNCIL, andGUARDIAN_PERIODas immutable helps avoid storage overhead and clarifies invariants.Also applies to: 66-67, 77-78, 82-83, 87-88
102-103: New mappings
•verifiedPublicInputstracks whether a given public input is verified.
•deletedOutputsrecords outputs that have been marked as deleted.
•assertionsmanages active assertions for each output index.These new mappings align well with the new challenge-assertion flow.
Also applies to: 107-108, 109-109
120-125:onlyL2OutputOraclemodifier
Restricts certain critical functions (e.g.,createAssertion) to be called by the L2 oracle contract. This ensures consistent flow from L2 outputs.
128-139:AssertionCreatedevent
The new event properly indicates the creation of an assertion, linking output index, asserter, and timestamp. Ensures transparency and debug traceability.
226-238:ChallengerHasTimedOutevent
Warns watchers that a challenger exceeded their bisection or proving window. This new event complements asserter and challenge-lifecycle events.
464-467:createChallengeintegration withassertions
Checks thatassertions[_outputIndex].status == IN_PROGRESSand the assertion actually exists, then incrementsnumberOfChallenges. Properly reverts if the output is already marked deleted or a reorg is detected.Also applies to: 469-497
483-486: Timeout calculations
• SubtractingelapsedfromMAX_CLOCK_DURATIONfor the challenger’s time left is accurate.
• Ifelapsed >= MAX_CLOCK_DURATION, the challenger is immediately timed out.
• Stored segment fields are consistent with earlier logic.Also applies to: 488-497
510-566:bisectfunction
Central to the new challenge protocol. Checks assertion existence, challenge’s current status, time-lapse, and the correctness of_posrelative to the midpoint.
• Reverts if_posis invalid or if bisection is no longer needed.
• EmitsReadyToProvewhen no further splits are possible.
• This logic is more robust than previous segment-based approach.
607-609: Relocating ZkVM fault proof
_proveFaultis now called byproveFaultWithZkVm. Clean separation of public & internal logic.
627-637:acceptAssertion
Checks if no challenges remain and_assertion.assertedAtis sufficiently old. If so, marks the assertion as accepted. This helps finalize valid outputs that go unchallenged.
645-653: Helper_acceptAssertionIfNoChallengeAndTimedOut
This function is both clean and easily testable. Ensures an assertion can only be accepted if the assertion has zero challenges and the asserter’s time window has fully elapsed.
768-844: ZkVM-based proof fault
• Validates the challenge status isREADY_TO_PROVEorASSERTER_TIMEOUT.
• ComparessrcOutputwithchallenge.segment.outputorstartOutput.
• Marks the output as deleted, calls security council for final validation.
• Overall logic is consistent with the new approach.
858-884: Challenge cancellation
If the output got deleted externally, the challenger can cancel. The logic properly refunds bonds or unbonds validators. This is an important grace condition.
893-920: Challenger timeout handling
Decrementsassertion.numberOfChallengesand slashes or reassigns bonds. The code is cohesive with the new approach of having an asserter and multiple potential challengers.
930-932: Challenge flow expansions
•_isAbleToBisectensures we have more than 2 blocks of difference.
•_challengeStatusincludes new states for either participant’s timeout.
•_isTimeoutclarifies time checks for the asserter vs. challenger, reverting if the address is invalid.Also applies to: 953-960, 1013-1023
packages/contracts/contracts/L1/interfaces/IColosseum.sol (9)
1-3: License and pragma
Matches other contracts' settings for SPDX and pragma solidity 0.8.15, ensuring version alignment.
4-5: ImportingTypes
This file depends onTypesfor theChallengeandAssertionstructs. The path reference looks accurate.
6-21: Interface outlines critical challenge states and data structures
DefinesZkVmProofandChallengeStatus, central to the challenge protocol. This clarifies the shared specification for implementing contracts.
23-39: Error definitions
Comprehensive error coverage matches the contract. Interface-level errors help unify revert reasons across multiple contract implementations.
40-54: Event definitions
The interface lists key events:AssertionCreated,Bisected,ChallengeCanceled, etc. This fosters consistent event usage in any implementing contract.
55-66: Read-only getters
Accessors for important config variables (e.g.,L2_ORACLE(),SECURITY_COUNCIL()) are consistent with best practices, enabling external consumption of contract metadata.
67-74: Core protocol functions
bisect(),cancelChallenge(),createAssertion(),createChallenge()define the main challenge and assertion flows. The parameters align with new contract logic.
75-86:dismissChallenge()andforceDeleteOutput()
Provides specific challenge resolution and output-deletion flows. Exposes them for external calls by relevant authorities (security council).
88-93: Helpers for retrieving stored data
getChallenge(),getAssertion(), anddeletedOutputs(...)are consistent with the storage layout.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (5)
1-3: Consider aligning Solidity version with the project's recommended compiler version.
Although pinning topragma solidity 0.8.15;is valid, you may want to confirm this matches the rest of the project's recommendations or security posture.Please verify if there’s a broader directive on standardized Solidity versions for your codebase.
8-15: Event naming and parameter order appear consistent.
The event names and indexed parameters are intuitive. No immediate issues are found.
16-39: Functional coverage check.
All getter functions for reading state (likeCOLOSSEUM(),FINALIZATION_PERIOD_SECONDS(), etc.) and ID-based lookup methods (e.g.,getL2Output) are declared with sensible names. No logic concerns in an interface, but ensure implementing contracts handle thorough validations.
40-60: Initialization and mutation functions.
Functions likeinitialize(...),replaceL2Output(...), andsetLatestFinalizedOutputIndex(...)appropriately capture essential parameters for Oracle updates. No concerns here, but confirm implementing contracts will manage access control (e.g. only callable by authorized addresses).Ensure you have or plan robust access-control checks in any contract implementing these methods.
61-74: Completion.
The interface layout is coherent and thorough. ThesubmitL2Outputfunction is well-defined with relevant parameters. Theversion()function is a helpful clarity measure.packages/contracts/contracts/L1/interfaces/IValidatorPool.sol (5)
4-5: Check for internal library usage.
You importTypesfrom../../libraries/Types.sol. Verify that all structures (e.g.,Types.Bond) are accurately defined for external usage, as an interface can be imported by third-party developers.
6-16: Event definitions are clear.
All event parameters are properly indexed and descriptive. Good job ensuring consistent naming.
17-38: Multiple view functions for cursory checks.
The definitions for retrieving constants likeMAX_UNBOND(),PORTAL(), etc. align well with typical bonding/validator logic. They appear logically consistent.
39-63: Interface for bond lifecycle management.
Functions likeaddPendingBond,createBond,releasePendingBond, andunbondprovide a clear workflow. Remember to ensure correct permission checks and state transitions in implementations.Confirm that caller authorizations and bond-lifecycle invariants are enforced properly in the implementing contracts.
64-73: Final checks.
Includingversion()for clarity is consistent with other interfaces. The overall structure of the interface is coherent.
c3f4194 to
e986d45
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/contracts/contracts/L1/Colosseum.sol (1)
378-384: 🛠️ Refactor suggestionValidating
_l2Oracleand auto-initializing.Consider adding a code-length check on
_l2Oracleto avoid misconfiguration (e.g., zero code).+require(_l2Oracle.code.length > 0, "Invalid L2 Oracle address");
🧹 Nitpick comments (2)
packages/contracts/contracts/test/Colosseum.t.sol (1)
307-315: Commented-out code.These lines appear to be legacy or deprecated. Consider removing them if they are no longer needed.
packages/contracts/contracts/test/CommonTest.t.sol (1)
222-223: Consider making these durations compile-time constants.Defining
guardianPeriodandmaxClockDurationas compile-time constants clarifies intent and prevents unintended changes. For instance:-uint256 guardianPeriod = 3.5 days; -uint256 maxClockDuration = 3.5 days; +uint256 constant GUARDIAN_PERIOD = 3.5 days; +uint256 constant MAX_CLOCK_DURATION = 3.5 days;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/contracts/contracts/L1/Colosseum.sol(19 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(6 hunks)packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol(1 hunks)packages/contracts/contracts/test/Colosseum.t.sol(27 hunks)packages/contracts/contracts/test/CommonTest.t.sol(16 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/contracts/test/mock/MockColosseum.sol
- packages/contracts/contracts/L1/L2OutputOracle.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (66)
packages/contracts/contracts/test/Colosseum.t.sol (26)
4-18: Imports look consistent.No immediate issues found with these newly added/modified imports.
29-36: Constructor call format update.The parameter alignment improves readability, and no functional issues are apparent.
53-55: New constants for turns and ready state.Clear naming of constants enhances readability.
63-66: New event declarations look good.These events allow better state change tracking.
77-82: MockColosseum constructor argument changes.Parameter injection for securityCouncil, guardianPeriod, and maxClockDuration remains consistent with the contract’s updated logic.
87-88: Re-initializing colosseum references.Casting the contract to its new implementation class is standard.
102-105: Depositing from trusted and asserter.These prank calls with deposits appear correct for test setup.
120-120: Challenger deposit.No issues spotted. Just ensures that the challenger also deposits.
123-124: Assertion creation trigger.Invoking the
_createAssertion()helper is aligned with the test setup steps.
131-131: Setting targetBlockNumber.Referenced test data constant is correct for negative test scenarios.
134-134: Use of ZkVmTestData.PREV_OUTPUT_ROOT.Straightforward reference, no issues found.
139-139: Use of ZkVmTestData.TARGET_OUTPUT_ROOT.No issues with referencing this constant for the test.
153-172: Binary search-based fault detection logic.Implementation looks correct for segment narrowing; off-by-one edge cases are handled by checking
start + 1 == end.
278-280: Initiating_detectFaultand_doProveFault.Usage aligns with the test’s flow of detecting and proving faults.
336-349: Testing bisect logic post-challenger timeout.Properly checks time warp and expects revert with
ChallengerTimedOut. Good negative test coverage.
352-367: Testing bisect logic post-asserter timeout.Similarly checks time warp for asserter’s timeout. This ensures thorough coverage.
372-385:test_challengerTimeout_succeeds.Correctly tests that the asserter can finalize a timeout scenario. Emission of event and call to
challengerTimeoutconfirm logic. 6D40
388-403:test_challengerTimeout_reverts.Ensures revert if challenger’s timeout has not fully elapsed.
417-421: Representing a reorg scenario.
block.number - 1for mismatch block hash. Helps simulate L1 reorg condition.
453-465: Not your turn check.Confirms revert when the non-turn party attempts
bisect.
478-478: Callingtest_proveFaultWithZkVm_succeeds()within other tests.Reuse of this helper method is efficient for setting up fault scenarios.
Also applies to: 511-511
517-539:test_proveFaultWithZkVm_succeedslogic.Covers the entire fault-proof path, verifying final output states. Good coverage.
605-609: Empty proof usage.Testing for an empty
ZkVmProofensures edge-case coverage.
626-638: Retrieving deleted output.Validates the
colosseum.deletedOutputs(outputIndex)and compares states.
644-644: Time-warping beyond finalization guard.Testing over 7 days ensures correct revert or acceptance.
Also applies to: 676-676
733-733: Subsequent call to prove fault again.Demonstrates repeated usage in a separate scenario, no new issues.
packages/contracts/contracts/L1/Colosseum.sol (29)
4-10: New imports forInitializableand others.Enables upgradability features and usage of universal interfaces.
13-13: Inheritance fromInitializableandISemver.Ensures the contract can be used in proxy-based deployments and versioning is tracked.
53-57: IntroducingMAX_CLOCK_DURATIONfield.This immutable convenience aligns with per-asserter/per-challenger timeout logic.
61-61:IL2OutputOraclereference.Using an interface fosters modularity.
69-72: Storing submission interval.This config ensures alignment with mandatory block checkpoint intervals.
75-99: Security Council, Guardian period, legacy mappings, and new data structures.The newly introduced immutables & placeholders appear consistent with the updated architecture.
117-120:onlyL2OutputOraclemodifier.Restricts external calls to the recognized L2 oracle instance.
129-133:AssertionCreatedevent.Clear event signature with indexed parameters for efficient off-chain filtering.
199-205:OutputForceDeletedevent: newasserterparameter.Enhances visibility into who originally submitted.
228-232:ChallengerHasTimedOutevent.Matches new logic for timing out challengers.
290-293:L1Reorgederror.Appropriately captures L1 blockhash mismatch scenarios.
295-353: New error definitions.Better granularity for challenge and assertion error states.
369-377: Revised constructor signature.Enables passing
_l2Oracleas an address and sets up new config variables.
392-423:createAssertionfunction and event emission.Allows the L2 oracle to record a new assertion. Looks correct.
433-501:createChallengefunction.Refactored to use new assertion structure. Logic for timeouts and partial reorg checks is consistent with the new challenge flow.
513-586:bisectfunction.Includes checks for correct turn, new
BisectUnnecessaryerror if already ready to prove, and properly updates segment boundaries.
599-599:proveFaultWithZkVmwrapper.Delegates to the private
_proveFaultfor clarity.
618-629:acceptAssertionflow.Confirms no active challenges and evaluates if enough time has passed.
631-645:_acceptAssertionIfNoChallengeAndTimedOut.Simple check merges no-challenge + timed-out to finalize assertion.
654-659:cancelChallengelogic.Relies on
_cancelIfOutputDeleted; reverts if not viable.
671-705:dismissChallenge.Ensures correct revert if output not actually deleted or public input not verified. Rolls back output root upon security council request.
713-731:forceDeleteOutput.Allows the security council to forcibly delete outputs if irrecoverably broken.
756-836:_proveFaultfunction.Verifies proof correctness, manipulates validator pool, and marks the assertion as rejected. Appears logically sound.
851-876:_cancelIfOutputDeleted.Securely checks output root, ensures only challenger can cancel, then refunds or unbonds accordingly.
885-912:_challengerTimeout.Deletes challenge, emits
ChallengerHasTimedOut, and slashes or repurposes bond.
921-923:_isAbleToBisect.Confirms enough distance between start and end to continue bisection.
991-996:getStatusfunction.Relays challenge status from
_challengeStatus. Straightforward usage.
977-982:getAssertionaccessor.Exposes stored assertion data. No issues found.
998-1016:_isTimeouthelper.Compares elapsed time to the stored time left for asserter or challenger.
packages/contracts/contracts/test/CommonTest.t.sol (10)
5-45: All import statements look consistent.No duplicates or conflicting imports are detected. These appear correctly organized and aligned with the project’s structure and dependencies.
213-214: Verify that the predeploy address is correct for L2ToL1MessagePasser.Please confirm that
Predeploys.L2_TO_L1_MESSAGE_PASSERholds the correct contract address for the environment. Incorrect addresses here will cause transaction failures.
331-332: Zero address usage for_zkMerkleTrieand_colosseum.Using
address(0)as a placeholder is acceptable for testing. Ensure production environments finalize valid addresses to avoid runtime errors.
374-374: ValidatorManager instantiation looks correct.The constructor parameters are clearly kept in
_constructorParams; passing them here appears consistent with the rest of the code.
381-393: Initialize and upgrade Colosseum carefully.The deployment pattern with a
Proxyplus anupgradeTocall is standard, but confirm that each parameter match (such asguardianPeriod/maxClockDuration) is correct. Missing or mismatched values can break the challenge flow.
511-546: Portal_Initializer adjustments to finalization logic and deposits.
- Line 511 introduces
finalizationPeriod.- Line 525 deposits the entire balance of
trusted. Verify no reentrancy or unbounded deposit concerns.- Lines 534-535 link
_zkMerkleTrieand_colosseumin the portal—looks correct for hooking up the new challenge logic.- Line 545 calculates
finalizationPeriod = portal.COLOSSEUM().GUARDIAN_PERIOD() + portal.COLOSSEUM().MAX_CLOCK_DURATION(). Great for flexible configuration, but confirm no unexpected overflow or extreme value scenario.
552-552: L2CrossDomainMessenger usage.Here, referencing
Predeploys.L2_CROSS_DOMAIN_MESSENGERis standard for bridging. Looks consistent with the rest of the bridging environment.
765-765: DUMMY_HASH definition.This is presumably a placeholder for verifying zero-knowledge proofs. As long as it matches the intended test data, there’s no functional issue.
889-889: CallerCaller fallback returns raw data.Returning the raw call result from
returndatais a valid approach to bubble up call results. Ensuring no reentrancy concerns exist is crucial. This method is a typical debug pattern.
917-917: ConfigurableCaller fallback also returns raw data.Same approach: returning the raw
returndatais correct for re-bubbling. Make sure any revert reasoning isn’t lost in the translation.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (1)
1-72: Interface design is comprehensive and consistent.All key getters, events, and management functions for L2 outputs are well-defined. Naming follows established patterns, making it straightforward to integrate with the rest of the system.
|
I wish you could update the storageLayout as well~! |
| @@ -116,8 +117,8 @@ contract L2OutputOracle is Initializable, ISemver { | |||
| * @param _finalizationPeriodSeconds Output finalization time in seconds. | |||
| */ | |||
| constructor( | |||
| ValidatorPool _validatorPool, | |||
| IValidatorManager _validatorManager, | |||
| address _validatorPool, | |||
There was a problem hiding this comment.
We don't need validator pool contract anymore.
| address _validatorPool, |
|
Please reformat all changes after properly configuring your formatter. |
23f03ca to
82f0175
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/contracts/contracts/L1/L2OutputOracle.sol (1)
459-477: 🛠️ Refactor suggestionCompletely rewrote finalization logic.
The
isFinalizedfunction has been reimplemented to:
- Consider output at index 0 as always finalized (special case)
- Query the Colosseum contract for assertion status
- Determine finalization based on assertion status, timestamps, and challenge counts
This complex conditional logic has multiple finalization paths depending on assertion state and timing.
While functionally correct, this complex conditional logic could be simplified for better readability and maintainability. Consider breaking down the conditions into more readable blocks:
function isFinalized(uint256 _outputIndex) external view returns (bool) { if (_outputIndex == 0) { return true; } Types.Assertion memory assertion = IColosseum(COLOSSEUM).getAssertion(_outputIndex); - if ( - (assertion.assertedAt != 0 && - assertion.status == Types.AssertionStatus.IN_PROGRESS && - assertion.numberOfChallenges == 0 && - block.timestamp - assertion.assertedAt > - IColosseum(COLOSSEUM).GUARDIAN_PERIOD() + - IColosseum(COLOSSEUM).MAX_CLOCK_DURATION()) || - (assertion.acceptedAt != 0 && - block.timestamp - assertion.acceptedAt > IColosseum(COLOSSEUM).GUARDIAN_PERIOD()) - ) { - return true; - } - return false; + // No assertion found + if (assertion.assertedAt == 0) { + return false; + } + + // Check IN_PROGRESS assertions + if (assertion.status == Types.AssertionStatus.IN_PROGRESS) { + // Must have no active challenges + if (assertion.numberOfChallenges > 0) { + return false; + } + // Check if sufficient time has passed + uint256 requiredTime = IColosseum(COLOSSEUM).GUARDIAN_PERIOD() + + IColosseum(COLOSSEUM).MAX_CLOCK_DURATION(); + return (block.timestamp - assertion.assertedAt > requiredTime); + } + + // Check ACCEPTED assertions + if (assertion.status == Types.AssertionStatus.ACCEPTED) { + return (block.timestamp - assertion.acceptedAt > IColosseum(COLOSSEUM).GUARDIAN_PERIOD()); + } + + // REJECTED or RESTORED assertions are not finalized + return false; }
🧹 Nitpick comments (11)
packages/contracts/contracts/libraries/Types.sol (1)
191-210: Added AssertionStatus enum.A new enum has been added to track the lifecycle states of an assertion:
IN_PROGRESS- Initial stateACCEPTED- Assertion passed challenge periodREJECTED- Assertion proved to be invalid or timed outRESTORED- Special state for restored outputsThis provides a clear state model for assertion lifecycle management, though the TODO comment about fixing the URL should be addressed.
Consider updating the TODO comment that mentions fixing the URL to the specification. Either provide the correct URL or remove the comment if it's no longer needed.
- // TODO fix the url + // See https://specs.kroma.network/fault-proof/challenge.html#state-diagrampackages/contracts/contracts/test/ValidatorManager.t.sol (1)
16-17: Ensure non-zero addresses.Consider adding validation checks for
_validatorPooland_validatorManageraddresses to avoid accidental zero address assignment.packages/contracts/contracts/test/CommonTest.t.sol (1)
222-223: Consider making these variablesimmutableorconstant.
IfguardianPeriodandmaxClockDurationremain fixed, usingimmutableorconstantcan improve clarity and potentially reduce gas costs.- uint256 guardianPeriod = 3.5 days; - uint256 maxClockDuration = 3.5 days; + uint256 public constant GUARDIAN_PERIOD = 3.5 days; + uint256 public constant MAX_CLOCK_DURATION = 3.5 days;packages/contracts/contracts/L1/interfaces/IColosseum.sol (3)
7-11: Structure definition forZkVmProof.
Consider adding inline documentation for fields (publicValues, etc.) to clarify their purpose. Also confirm that large byte arrays are handled efficiently.
13-20: EnumChallengeStatusis straightforward.
Consider adding a short comment for each status value to guide integrators.
39-79: Events look comprehensive but consider indexing additional fields.
Indexing fields like_timestampis optional but can aid off-chain queries, especially for time-based events.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (1)
8-21: Event definitions are clear and consistent.
Optionally indexl2BlockNumberor other fields if frequent off-chain filtering is required.packages/contracts/contracts/L1/interfaces/IValidatorPool.sol (4)
1-6: Interface design looks good but lacks documentation.This new interface establishes a clear contract between components. However, it would benefit from NatSpec documentation to explain its purpose and relationship to the overall system.
Consider adding comprehensive NatSpec documentation like:
// SPDX-License-Identifier: MIT pragma solidity 0.8.15; import { Types } from "../../libraries/Types.sol"; +/** + * @title IValidatorPool + * @notice Interface for the ValidatorPool contract that manages validator bonds and operations + * @dev This interface is used by contracts like L2OutputOracle and KromaPortal to interact with the ValidatorPool + */ interface IValidatorPool {
46-86: Function signatures are appropriately defined but could benefit from NatSpec documentation.The function signatures are correctly defined with proper visibility and return types. However, adding NatSpec documentation would significantly improve developer experience and reduce integration errors.
Consider adding function-level documentation, for example:
+/** + * @notice Creates a bond for a specific L2 output + * @param _outputIndex The index of the L2 output to bond + * @param _expiresAt The timestamp at which the bond expires + */ function createBond(uint256 _outputIndex, uint128 _expiresAt) external;
46-86: Consider adding custom errors for better error reporting.The interface doesn't define any custom errors, which could provide more descriptive error messages when operations fail.
Add custom errors to improve error reporting:
interface IValidatorPool { // Events... + /** + * @notice Errors that may be thrown by the ValidatorPool contract + */ + error InsufficientBond(uint256 required, uint256 provided); + error UnauthorizedValidator(address account); + error InvalidOutputIndex(uint256 outputIndex); + // Additional errors... // Functions... }
1-86: Consider organizing functions by logical grouping.The functions appear to be somewhat randomly ordered. Organizing them into logical groups would improve readability and maintainability.
Consider grouping functions by their purpose:
interface IValidatorPool { // Events... // Constants function L2_ORACLE() external view returns (address); // Other constants... // Bond management function createBond(uint256 _outputIndex, uint128 _expiresAt) external; function getBond(uint256 _outputIndex) external view returns (Types.Bond memory); // Other bond functions... // Validator management function isValidator(address _addr) external view returns (bool); function nextValidator() external view returns (address); // Other validator functions... // Account management function deposit() external payable; function withdraw(uint256 _amount) external; // Other account functions... // Admin and utility functions function initialize() external; function version() external view returns (string memory); // Other utility functions... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/contracts/contracts/L1/Colosseum.sol(21 hunks)packages/contracts/contracts/L1/KromaPortal.sol(7 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(6 hunks)packages/contracts/contracts/L1/ValidatorManager.sol(0 hunks)packages/contracts/contracts/L1/interfaces/IColosseum.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IValidatorPool.sol(1 hunks)packages/contracts/contracts/echidna/FuzzKromaPortal.sol(1 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/AssetManager.t.sol(2 hunks)packages/contracts/contracts/test/Colosseum.t.sol(30 hunks)packages/contracts/contracts/test/CommonTest.t.sol(9 hunks)packages/contracts/contracts/test/KromaPortal.t.sol(8 hunks)packages/contracts/contracts/test/L1ERC721Bridge.t.sol(0 hunks)packages/contracts/contracts/test/L2OutputOracle.t.sol(5 hunks)packages/contracts/contracts/test/ValidatorManager.t.sol(11 hunks)packages/contracts/contracts/test/ValidatorPool.t.sol(3 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol(1 hunks)
💤 Files with no reviewable changes (2)
- packages/contracts/contracts/test/L1ERC721Bridge.t.sol
- packages/contracts/contracts/L1/ValidatorManager.sol
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/contracts/contracts/echidna/FuzzKromaPortal.sol
- packages/contracts/contracts/L1/KromaPortal.sol
- packages/contracts/contracts/test/L2OutputOracle.t.sol
- packages/contracts/contracts/test/mock/MockColosseum.sol
- packages/contracts/contracts/test/ValidatorPool.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (68)
packages/contracts/contracts/test/AssetManager.t.sol (2)
67-70: Constructor parameter type update to addresses.The constructor parameters for
MockL2OutputOraclehave been updated to pass addresses (address(pool)andaddress(valMgr)) instead of the contract instances themselves. This change aligns with the updated constructor parameter types in the mainL2OutputOraclecontract and follows best practices by relying on interfaces rather than concrete implementations.
103-103: Time warping approach changed.The
vm.warpcall that previously warped to the finalization timestamp has been commented out. Instead, the test now usesmockOracle.mockSetNextFinalizeOutputIndex()to simulate the finalization state. This is likely related to changes in the assertion/finalization protocol.Are there any side effects from this change in testing approach? Make sure the test still properly verifies that the oracle's finalized state is correctly handled.
packages/contracts/contracts/test/KromaPortal.t.sol (7)
8-8: Added import for IColosseum interface.Added import for the new
IColosseuminterface, which is used for interacting with the Colosseum contract - a key component of the updated challenge protocol.
20-22: Added proper superclass setup call.The setUp function now correctly calls
super.setUp(), ensuring proper initialization from the parent class. This is a critical fix to prevent potential initialization issues.
312-333: Refactored output finalization testing to use IColosseum.The test has been updated to:
- Use
Types.Assertionstructure instead of the previous approach- Mock the Colosseum contract's
getAssertioncall instead of the previous oracle methods- Test that index 0 is always considered finalized (special case)
- Test finalization based on the assertion's timestamp + finalization period
This aligns with the new assertion-based finalization logic in the L2OutputOracle contract.
414-423: Added setup for handling index 0 finalization.Additional setup code has been added to ensure consistent test behavior with the new rule that "the output at index 0 is always finalized". This change is important to maintain the test's integrity with the updated protocol.
799-801: Renamed test function for clarity.The test function name has been updated from
test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_revertstotest_finalizeWithdrawalTransaction_ifOutputIsNotFinalized_reverts, better reflecting the changed focus from timestamp-based finalization to assertion-based finalization.
817-823: Updated mock calls to use IColosseum.The test now mocks the Colosseum contract's
getAssertionmethod instead of the previous approach. This ensures testing consistency with the updated assertion-based finalization model.
994-998: Added mock calls for oracle.isFinalized.New mock calls for the
isFinalizedmethod have been added to properly test the finalization logic. This ensures the tests properly verify the updated finalization mechanism.Also applies to: 1074-1078
packages/contracts/contracts/libraries/Types.sol (2)
149-167: Added new Challenge and Segment structs.Two new structs have been added:
Challenge- Tracks challenge state with fields for turns, time management, participants, and segment informationSegment- Represents a segment of data with output, position, and range informationThese additions are fundamental components of the new challenge protocol, enabling more sophisticated challenge mechanisms.
172-189: Renamed and enhanced Challenge struct to Assertion.The previous
Challengestruct has been renamed toAssertionand enhanced with additional fields:
startL2BlockNumberandstartOutputRootto define the assertion scope- Timestamp fields (
assertedAt,acceptedAt,rejectedAt)- Status tracking
- Counter for number of challenges
This represents a significant design improvement, more clearly separating assertions (claims) from challenges (disputes of those claims).
packages/contracts/contracts/L1/L2OutputOracle.sol (4)
10-11: Updated imports to use interfaces.Imports have been updated to use the
IValidatorPoolandIColosseuminterfaces, which aligns with best practices by preferring interfaces over concrete implementations.
24-24: Updated VALIDATOR_POOL type and added COLOSSEUM field.
- The VALIDATOR_POOL type has been changed from concrete implementation to interface type (IValidatorPool)
- A new immutable COLOSSEUM field has been added to store the address of the Colosseum contract
These changes support the architectural shift toward using interfaces and incorporating the new Colosseum contract in the protocol.
Also applies to: 34-34
120-122: Updated constructor to accept addresses.The constructor now accepts addresses for
_validatorPooland_validatorManagerinstead of contract instances, casting them to their respective interface types during assignment.This is a good architectural change that:
- Reduces coupling between contracts
- Follows the principle of depending on abstractions rather than concrete implementations
- Makes testing easier
Also applies to: 135-137
273-275: Added assertion creation for non-zero output indices.When a new output is submitted with an index greater than 0, the contract now calls
createAssertionon the Colosseum contract, connecting output submission with assertion creation in the new protocol.This is a critical part of the new challenge protocol, enabling assertions to be created for each submitted output.
packages/contracts/contracts/test/ValidatorManager.t.sol (12)
4-11: Adoption of new library imports.No issues found. The updated imports from these libraries appear consistent and do not introduce any conflicts.
25-34: Constructor parameter alignment.The constructor parameter expansion looks properly aligned with the new arguments. Ensure all call sites match this new signature.
148-149: Initialization of MockL2OutputOracle.Passing
address(pool)andaddress(valMgr)is consistent with the constructor changes. No concerns.
174-179: Mocking oracle finalization for testing.The usage of
vm.mockCalland subsequentvm.warpis valid. Ensure that this mock does not interfere with other tests.
400-406: Repeated mock for oracle finalization.This is a standard pattern to simulate finalization. No issues found.
443-448: Oracle finalization mock.Looks consistent and correct. No concerns.
475-480: Simulation of finalize condition.These lines replicate the finalization mock/warp flow. No issues found.
494-498: Dynamic mock for nextFinalizeOutputIndex.Approach is consistent across tests. No concerns.
816-820: Challenged output finalization mock.The finalization logic is tested thoroughly. Approve.
1073-1073: No code changes.No functional changes introduced here.
1079-1080: New constructor arguments.Matches the pattern from earlier code. No issues.
1118-1123: Mock call on finalization in MptTransitionTest.Helpful for verifying transitions. Looks good.
packages/contracts/contracts/test/Colosseum.t.sol (12)
52-54: Added role-based constants.Defining
CHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEclarifies the challenge flow. Approved.
62-82: Introduction of new events for challenge lifecycle.These events can improve off-chain monitoring. However, confirm no event name collisions or impact on existing watchers.
Please ensure these event names haven't replaced older events with different signatures.
137-138: Forced assertion creation in setup.Ensure that automatically calling
_createAssertion()in the test setup won't mask other potential test flows.
145-145: Manual override of targetBlockNumber.Assigning an invalid block number is suitable for negative scenario testing. No concerns.
148-148: Returning a predetermined previous root.Ensures the negative scenario path is tested. No issues.
153-153: Overriding block-based output root.This allows for custom negative scenario testing. Good approach.
166-186: Refined fault detection logic.The bisection approach and off-by-one checks appear correct. Verify that
start - 1does not underflow ifstart == 0.Would you like a script to confirm no test path triggers underflow here?
199-214: Private assertion creation for test.Implementation is consistent with mocking an L2 output submission. No immediate issues detected.
228-229: ChallengeCreated event emission.Provides clarity on the challenge lifecycle. The event parameters match the function arguments.
297-298: Capturing public input hash.Storing
_doProveFaultresult ensures future steps reference it correctly. No problems.
313-313: Helper function signature extension.This private function enhances test clarity. No concerns found.
316-316: Invoking proveFaultWithZkVm.Ensures direct coverage of the public function. LGTM.
packages/contracts/contracts/L1/Colosseum.sol (14)
8-8: Switch to IL2OutputOracle interface import.Interface-based import fosters decoupling. Approved.
52-55: Introduction of MAX_CLOCK_DURATION.Used for timeouts. Ensure no zero or excessively large values.
60-60: Using L2_ORACLE as an interface reference.Storing the oracle as
immutableis a cost-effective approach. No issues.
68-71: Documentation for L2_ORACLE_SUBMISSION_INTERVAL.Provided clarity on block interval usage. Implementation is consistent with prior references.
74-76: SECURITY_COUNCIL definition.Introducing an immutable reference to the security council ensures a stable trust anchor. No concerns.
79-81: Guardian period introduction.This clarifies the timeframe for external verification. Approved.
83-105: Legacy spacer fields and new mappings.Adding placeholders for legacy mappings suggests a safe upgrade path. The
verifiedPublicInputsanddeletedOutputsmappings appear logically correct.
116-119: Restricting function calls to the L2OutputOracle.Ensures that only the oracle can invoke certain operations. No issues found.
122-132: AssertionCreated event definition.Helps track the creation of new assertions. The parameters match the function logic in
createAssertion.
397-398: Handling assertion creation for non-genesis outputs only.Rejecting
_outputIndex == 0is correct to avoid messing with the genesis output.
400-404: Shifting finalizedOutputIndex if > 0.Ensures we consider the last finalizable index accurately. No issues.
405-405: Retrieving the previously finalized output.Loading from L2_ORACLE is correct for the assertion start point.
407-410: Check for existing assertion.Prevents double creation of the same
_outputIndex.
413-418: Populating new Assertion fields.The stored block number, root, and asserter are set properly.
packages/contracts/contracts/test/CommonTest.t.sol (6)
45-45: New import looks correct.
No immediate concerns; this import aligns with references toZKProofVerifierelsewhere in the codebase.
330-331: Verify placeholder addresses.
Passingaddress(0)for_zkMerkleTrieand_colosseummay lead to unintended outcomes if used before assignment. Confirm that these fields are initialized later in the lifecycle.
381-393: Deployment and upgrade logic appears correct.
The sequence of creating a new proxy, deploying an implementation, and upgrading is consistent with other upgradable patterns. Ensure integration tests confirm thatColosseumis properly initialized after deployment.
396-397: Confirm correct contract references for_validatorPooland_validatorManager.
These parameters appear necessary for the updated L2 output logic. Double-check that usingaddress(pool)andaddress(valMgr)is valid at runtime.
510-511: Check usage offinalizationPeriod.
finalizationPeriodis declared but not visibly used within this snippet. Confirm it’s either used downstream or remove if unnecessary.
545-547: Validate sum of guardian and clock durations.
Ensure thatportal.COLOSSEUM()is a valid reference and that summingGUARDIAN_PERIODandMAX_CLOCK_DURATIONdoes not introduce overflow or unexpected large values.packages/contracts/contracts/L1/interfaces/IColosseum.sol (3)
1-3: SPDX license and pragma are standard.
No issues with licensing declaration or compiler version choice.
22-38: Error definitions are well-structured.
They provide clear, specific revert reasons. This is helpful for debugging.
80-145: Function declarations match intended challenge framework, but confirm implementation alignment.
Particularly, verify thatproveFaultWithZkVmwith_posis consistently implemented in the main contract. This parameter was a known point of mismatch in past reviews.packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (3)
1-3: SPDX license and pragma version are standard.
No concerns specific to licensing or compiler usage.
22-33: Read-only interfaces for external references.
Exposing addresses forCOLOSSEUM,VALIDATOR_POOL, and others improves clarity. All good here.
34-88: Extensive function set for output submissions and queries appears appropriate.
Confirm in usage that finalization, reverts, or_l2BlockHashreferences align with the actual L1 block headers. This interface change should be tested thoroughly with downstream logic.packages/contracts/contracts/L1/interfaces/IValidatorPool.sol (3)
7-22: Events are well-structured with appropriate indexed parameters.The event definitions follow best practices with proper indexing of key parameters for efficient filtering. This will enable effective off-chain monitoring and analytics.
24-45: Constants exposure through view functions follows good practices.The interface properly exposes constant values through view functions, allowing other contracts to access important configuration parameters in a type-safe manner.
1-86: Good use of interfaces for abstraction and modularity.Creating this interface is a positive architectural change that improves modularity, testability, and upgradability of the system. This aligns with the PR objectives of modifying the protocol while maintaining clear component boundaries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
packages/contracts/contracts/L1/KromaPortal.sol (3)
72-76: New immutable reference forColosseumDefining
COLOSSEUMas an immutable interface pointer is clear and aligns with best practices. Consider adding a code size check (e.g.,require(_colosseum.code.length > 0, ...)) for defensive programming in the constructor.
187-189: Constructor parameter_colosseumand assignmentAccepting
_colosseumas anaddressand casting toIColosseumis fine. As a further safeguard, you could verify_colosseumis a valid deployed contract to avoid misconfiguration.+ require(_colosseum.code.length > 0, "Invalid Colosseum address"); COLOSSEUM = IColosseum(_colosseum);Also applies to: 195-195
383-383: Clarify the TODO commentThere is a TODO note asking “why is this necessary?”. Please add a brief explanation or remove the comment if no longer needed.
packages/contracts/contracts/test/Colosseum.t.sol (4)
62-82: Introduction of new eventsAdding
AssertionCreated,ChallengeCreated,ChallengerTimedOut, andChallengeCanceledclarifies the Colosseum’s challenge flow in test logs. Ensure these events are also covered with dedicated test assertions.
137-138: Assertion helper callDirectly invoking
_createAssertion()during test setup makes tests more concise. Consider verifying that the assertion event logs are expected.
184-185: Midpoint calculationUsing
(start + end) / 2is standard. Keep an eye out for potential off-by-one errors, though it looks fine for this integer-based approach.
199-202: Local_createAssertionfunction definitionEncapsulating assertion creation for test clarity is a good approach. Encourages a DRY pattern across multiple test cases.
packages/contracts/contracts/L1/Colosseum.sol (6)
71-71: NewL2_ORACLE_SUBMISSION_INTERVALCapturing submission intervals from the constructor clarifies the workflow. Ensure that large intervals do not hamper finality checks.
369-376: Constructor with updated parametersEnsuring
GUARDIAN_PERIODandMAX_CLOCK_DURATIONare finalized in the constructor is consistent. Consider checking_l2Oraclecode size to guarantee it’s deployed.
383-383: Finalinitialize()callEmpty initializer ensures upgradability. Confirm you won’t need a second-phase init if new storage variables are added in future releases.
397-419:createAssertionfunctionAllows the L2Oracle to register an assertion. This is a necessary expansion for the new multi-challenge system. Logic looks good, but consider verifying no overlap with already accepted outputs.
516-526:bisectfunction updatesIntroduces verification for the correct turn, adjusting the segment boundaries, and rejecting invalid
_posvalues. Solid approach. Avoid off-by-one pitfalls by validating midpoint logic carefully.
757-758: Slicing outsrcOutputfrom publicValuesExtracting subsets within the 32-byte boundaries is clear. Documenting the indexing offsets further ensures maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/contracts/contracts/L1/Colosseum.sol(20 hunks)packages/contracts/contracts/L1/KromaPortal.sol(6 hunks)packages/contracts/contracts/test/Colosseum.t.sol(30 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (49)
packages/contracts/contracts/L1/KromaPortal.sol (4)
17-18: Additions of essential importsThe newly added
TypesandIColosseumimports cleanly introduce the required interfaces and data structures needed for portal-colosseum interactions.
409-410: Requiring checkpoint output finalizationThe requirement ensures the corresponding L2 output is finalized before finalizing the withdrawal. This is logically sound and prevents race conditions.
539-541: Getter function refactorExposing
isOutputFinalizedto call the new internal_isOutputFinalizedmethod simplifies readability without changing functionality.
542-543: Internal utility for output finalization checkGood separation of concerns:
_isOutputFinalizeddelegates toL2_ORACLE.isFinalizedcleanly. No concerns here.packages/contracts/contracts/test/Colosseum.t.sol (18)
52-55: New constants for challenge rolesDeclaring
CHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEimproves readability by providing explicit numeric labels.
92-92: Constructor param usagePassing
address(oracle)toMockColosseumis consistent with pattern usage, ensuring a reliable reference for L2 outputs.
95-97: Additional constructor argumentsIncluding
address(securityCouncil),guardianPeriod, andmaxClockDurationin the test deployment better aligns with real usage scenarios in the Colosseum. No issues found.
102-102: MockColosseum assignmentRetrieving the upgraded contract via
MockColosseum(address(colosseum))is standard practice for test setups.
145-145: AssigningINVALID_BLOCK_NUMBERUsing
ZkVmTestData.INVALID_BLOCK_NUMBERis a straightforward approach to simulate an invalid checkpoint scenario for challenge tests.
166-167: Declaration of local variablesInitializing
startandendclearly for the_detectFaultfunction is a neat style improvement, avoiding shadowed or reused variables.
168-169: Conditional logic in_detectFaultAssigning
startifsegment.outputmatches_getOutputRoot(...)is a standard approach. No issues found.
170-171: Continuation of_detectFaultlogicCarrying over
endassignment. Logic remains consistent with the function’s fault detection approach.
172-173: Branch-based segment boundingRefining conditionals to adjust
startorenddepending on correctness is neatly structured, supporting further bisect logic.
176-181: Edge case for consecutive blocks in_detectFaultGood coverage of the scenario where
start + 1 == endto pinpoint the exact faulty position. No concerns here.
188-189: New challenger registrationRe-funding a new address with
10 etherand depositing the entire balance ensures a fresh challenger environment. Appropriately tested.
193-193: Pooling deposit for new challengerStraightforward deposit flow, no issues found.
196-197: Challenger trackingAdding
isChallenger[newAddr] = true;for quick identification is a simpler design pattern.
213-213: Expect event emission
vm.expectEmit(true, true, false, true);demonstrates a precise check forAssertionCreated. Encourages thorough coverage.
215-215: Final step of_createAssertionThe assertion is effectively submitted. This test flow is consistent with the actual usage.
403-416: Testing challenger timeout logicVerifying bisection flow, warping time, and confirming revert with
ChallengerTimeoutErrorthoroughly checks the fallback path. Good coverage.
418-435: Asserter timeout pathConfirming equivalence to
ChallengerTimeoutErrorscenario but for asserters. Ensures symmetrical testing of both roles’ timeouts.
437-452:test_challengerTimeout_succeedsblockThis test further ensures that, once a challenger’s time left expires, the flow is correct (event emission, bond handling, etc.). No issues observed.
packages/contracts/contracts/L1/Colosseum.sol (27)
8-8: Switch toIL2OutputOracleimportThis abstraction aligns with a more flexible design and decouples from concrete implementations.
52-55:MAX_CLOCK_DURATIONintroducedStoring a single global timeout is straightforward for asserter/challenger time limits. Exposing it as
public immutableis consistent.
60-60: Public immutableL2_ORACLEStoring the oracle as an interface reference fosters decoupled logic. No issues found.
77-77: Security Council addressMaintaining an immutable reference helps ensure privileged operations remain restricted.
81-81: Guardian period for additional verificationAdding
GUARDIAN_PERIODin the constructor extends or ensures a final check. Implementation clarity is good.
85-85: Legacy spacer variablesLeaving older storage slots intact to preserve layout is prudent for upgradeable contracts.
91-91: IntroducingverifiedPublicInputsmappingTracks proven ZK public inputs to prevent re-verification. This approach mitigates replay attempts or double usage.
93-101:deletedOutputsmappingStoring deleted or forcibly removed outputs by index clarifies revert or slash flows, preserving a record.
104-104:assertionsmappingMaintains references to each assertion’s metadata. Good addition for new challenge logic.
336-336:ChallengeAlreadyCreatederrorAdding an explicit error clarifies user feedback when re-attempting existing challenges.
428-432: ExtendedcreateChallengeparametersThe
_l1BlockHash&_l1BlockNumberreorg check ensures no mismatch with chain state. Good approach to detect potential L1 reorgs.
445-445: Check forCHALLENGER_TIMEOUTbefore re-challengingAllows a new challenge if the old one ended with
CHALLENGER_TIMEOUT. Prevents indefinite reverts.
528-529: Challenge status check with_challengeStatusWell-structured early exit for canceled or unexpected states. Minimizes extraneous logic.
535-554: Timeout checks & turn updatesStrong and explicit handling of time left for each participant. Emitting
BisectedorReadyToProveclarifies state transitions.
557-568: Dynamic segment start/end updatesAdjusting segments on midpoint checks is core to the bisection. The validations help avoid leaps or incorrect segment boundaries.
570-580: Advancing turn and verifying readinessIncrementing
challenge.turn, storing_output, and optionally firingReadyToProveare consistent with your challenge flow design.
582-582:Bisectedevent emissionSignaling the new turn fosters testable events. Solid for debugging if unexpected states arise.
732-732: Private_proveFaultusage fromproveFaultWithZkVmEncapsulating repeated logic is clean. Confirm the internal arc correctly sets or reverts final states.
739-739:_proveFaultfunction refactorHandles key logic: verifying proof, updating
deletedOutputs, requesting validation fromSecurityCouncil. Implementation is well-structured for complex flows.
765-766: Handling mismatch in public inputReverting immediately on
InvalidPublicInputis a safe design. No partial updates occur on error.
767-772: Delegating proof verification toZK_PROOF_VERIFIERCentralizing ZK logic in a separate contract is wise, ensuring minimal chance of duplicating or scattering critical proof checks.
774-775: Double-verification preventionPreventing repeated usage of the same
publicInputHashwithAlreadyVerifiedPublicInputis a key security measure.
801-809: Validator system branchingConditionally slashing or merging bonds based on the
ValidatorPooltermination state is a good approach to uni CEB7 fy or differentiate between a legacy vs. new system.
813-814: MarkingrejectedAton the assertionRecording a rejection timestamp helps track invalidation times or measure final resolution intervals.
831-857: Cancelling challenge if output is already deletedA graceful exit for stale or moot challenges. Proper events are emitted, and bond refunds or releases are handled.
867-893: Handling challenger timeout gracefullyWhen the challenger runs out of time, remove the challenge, slash or reassign bonds, and keep the process consistent with finalization logic.
985-994: Actor-based timeout checks in_isTimeoutSplitting logic for
challengerTimeLeftorasserterTimeLeftis straightforward. Revert on unknown actor to reduce silent failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/contracts/contracts/libraries/Types.sol (2)
191-212: Assertion struct needs documentation improvementsThe Assertion struct implementation looks good, but some documentation fields need improvements:
- Line 194:
startL2BlockNumberdescription is ambiguous - clarify exactly what this represents- Line 195:
startOutputRootdescription is incomplete - just says "output."Please provide more detailed descriptions for these important fields.
214-233: Fix the TODO comment for AssertionStatus documentationThe AssertionStatus enum implementation is good, but there's a TODO comment on line 217 to fix a URL. This should be resolved before finalization.
- // TODO fix the url + // See https://specs.kroma.network/fault-proof/challenge.html#state-diagramThe enum defines a comprehensive set of states for assertions, which aligns well with the transition logic described in the comments.
packages/contracts/contracts/L1/KromaPortal.sol (2)
17-18: Remove duplicate Types import on line 17There's a duplicate import of the Types library. The same import already exists on line 9.
-import { Types } from "../libraries/Types.sol";
72-76: Add explanation for COLOSSEUM's relationship with KromaPortalThe COLOSSEUM variable has been added, but there's no explanation of how KromaPortal interacts with it. Consider adding more details to the comment about the relationship between these contracts and why KromaPortal needs to reference Colosseum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/contracts/contracts/L1/KromaPortal.sol(5 hunks)packages/contracts/contracts/L1/ValidatorPool.sol(0 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/ValidatorPool.t.sol(0 hunks)
💤 Files with no reviewable changes (2)
- packages/contracts/contracts/L1/ValidatorPool.sol
- packages/contracts/contracts/test/ValidatorPool.t.sol
🔇 Additional comments (4)
packages/contracts/contracts/libraries/Types.sol (2)
149-170: Challenge struct updated with new time tracking fieldsThe Challenge struct has been modified to implement a chess clock model for timing. This is a good enhancement for fairness, as it separately tracks time for asserters and challengers. The
segmentfield now references a singleSegmentstruct instead of what was previously likely an array of segments.
172-189: Good job extracting Segment into separate structCreating a separate
Segmentstruct to group output-related values is a sensible approach to avoid stack too deep errors. The struct encapsulates all the necessary data for segment management during challenges.packages/contracts/contracts/L1/KromaPortal.sol (2)
187-189: Constructor updated to include Colosseum address parameterThe constructor has been correctly updated to accept and store the Colosseum address. This change aligns with the previous addition of the COLOSSEUM immutable variable.
Note: Per the previous PR comment from kangsorang asking "Do we need the colosseum address in the portal?", it would be helpful to document why this integration is necessary.
Also applies to: 195-195
537-543: Refactored isOutputFinalized to use internal methodGood refactoring of
isOutputFinalizedto use an internal_isOutputFinalizedmethod, which delegates toL2_ORACLE.isFinalized(). This improves code organization and reusability.
3cba996 to
17d0f7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/contracts/contracts/libraries/Types.sol (1)
152-155: Update documentation for the Challenge struct fieldsThe new Challenge struct has been restructured to implement a chess-clock model for time management during challenges. However, the documentation could be improved to better explain:
- How the turn mechanism works (what value represents whose turn)
- The time unit for
asserterTimeLeftandchallengerTimeLeft(seconds?)- The purpose and impact of the
updatedAttimestampConsider adding more details to the documentation:
- @custom:field turn The current turn. - @custom:field asserterTimeLeft Total remaining time for the asserter in the challenge, based on the chess clock model. - @custom:field challengerTimeLeft Total remaining time for the challenger in the challenge, based on the chess clock model. - @custom:field updatedAt Timestamp of the last update to the challenge. + @custom:field turn The current turn (0 for asserter, 1 for challenger). + @custom:field asserterTimeLeft Total remaining time in seconds for the asserter in the challenge, based on the chess clock model. + @custom:field challengerTimeLeft Total remaining time in seconds for the challenger in the challenge, based on the chess clock model. + @custom:field updatedAt Unix timestamp of the last update to the challenge, used for calculating time spent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/contracts/contracts/L1/KromaPortal.sol(5 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/ValidatorPool.t.sol(0 hunks)
💤 Files with no reviewable changes (1)
- packages/contracts/contracts/test/ValidatorPool.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts/contracts/L1/KromaPortal.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (5)
packages/contracts/contracts/libraries/Types.sol (5)
159-168: LGTM: Challenge struct field updatesThe Challenge struct has been refactored to support the new bisection protocol with improved time management. The replacement of fixed segment fields with a structured Segment reference provides better organization.
191-212: Improve naming consistency and documentation in Assertion structThe field naming and documentation in the Assertion struct need improvement for clarity and consistency.
- @custom:field startL2BlockNumber start l2 block number for bisection. - @custom:field startOutputRoot output. + @custom:field startL2BlockNumber Starting L2 block number for bisection. + @custom:field startOutputRoot Starting L2 output root for bisection.Also consider adding fields to track:
- The final L2 block number of the assertion (if applicable)
- The final output root being asserted
214-233: Fix TODO and complete documentation for AssertionStatus enumThe TODO comment and URL placeholder should be addressed before merging.
- // TODO fix the url - * See the https://specs.kroma.network/fault-proof/challenge.html#state-diagram + * See the https://specs.kroma.network/fault-proof/protocol.html#state-diagramAlso, the state transition documentation could be improved with more details about each transition condition and its implications.
223-227:❓ Verification inconclusive
Verification of state transition logic
The current documentation of state transitions may not fully cover all possible paths.
Verify that these transitions are correctly implemented in the Colosseum contract:
- Are there any conditions where an assertion can directly go from IN_PROGRESS to RESTORED?
- When exactly does the RESTORED state get used?
🏁 Script executed:
#!/bin/bash # Check Colosseum contract for the implementation of these transitions echo "Checking implementation of AssertionStatus transitions in Colosseum..." rg -A 10 "AssertionStatus\." --type sol packages/contracts/contracts/Length of output: 250
Attention: Manual Verification Required for State Transition Logic
It appears that the initial verification command didn’t yield output because the Solidity (.sol) file type wasn’t recognized. To further investigate the implementation of AssertionStatus transitions in the Colosseum contract, please run the following updated shell commands manually:
#!/bin/bash # Re-run the verification search for AssertionStatus transitions using glob filtering for Solidity files echo "Verifying AssertionStatus transitions in .sol files..." rg -A 10 "AssertionStatus\." -g "*.sol" packages/contracts/contracts/Additionally, to ensure we capture how the RESTORED state is utilized, consider running:
#!/bin/bash # Search for the use of the RESTORED state echo "Searching for usage of RESTORED state in .sol files..." rg -A 10 "RESTORED" -g "*.sol" packages/contracts/contracts/Please review the output from these commands to verify:
- If there’s any scenario where an assertion can move directly from IN_PROGRESS to RESTORED.
- The exact conditions triggering the RESTORED state in the Colosseum contract.
Once you have confirmed these details, update the documentation accordingly if any discrepancies are found.
172-189:❓ Verification inconclusive
Ensure proper validation for Segment struct in consuming contracts
The new Segment struct nicely encapsulates all the properties related to bisection segments, which should help avoid stack too deep errors as mentioned in the documentation.
When using this struct in contracts like Colosseum, ensure proper validation of these fields:
- Check that
start<pos<end- Verify that
output,startOutput, andendOutputare properly calculated and validated
🏁 Script executed:
#!/bin/bash # Find where the Segment struct is used to verify validation echo "Searching for Segment usage in contracts..." rg -A 5 "Segment\s+\w+" --type sol packages/contracts/contracts/Length of output: 191
Below is a new shell script that adds Solidity as a recognized file type for ripgrep and searches for usages of the
Segmentstruct in the contracts. This will help verify whether consuming contracts—such as Colosseum—apply the proper validations (e.g., ensuring thatstart<pos<endand thatoutput,startOutput, andendOutputare correctly calculated).Please run the following script:
After obtaining the results, please manually verify that:
- The
Segmentstruct is consumed in contracts like Colosseum.- The validations for the fields (i.e.,
start<pos<endand proper output calculations) are implemented correctly.
🏁 Script executed:
#!/bin/bash # Add Solidity as a recognized file type for ripgrep and search for 'Segment' usage. rg --type-add 'sol:*.sol' --type sol -A 10 "Segment" packages/contracts/contracts/Length of output: 1879
[Action Required] Please verify that consuming contracts (e.g., Colosseum) implement proper validations for the
Segmentstruct.
- The search results indicate that the
Segmentstruct is defined and used inpackages/contracts/contracts/libraries/Types.sol, but there is no clear evidence of its usage in other consuming contracts.- Manually confirm in contracts like Colosseum that:
- The fields satisfy the constraint:
start < pos < end.- The outputs (
output,startOutput, andendOutput) are correctly calculated and validated.
|
The |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
packages/contracts/contracts/test/KromaPortal.t.sol (1)
800-834: 🛠️ Refactor suggestionReverted finalization for unfinalized checkpoint.
Intest_finalizeWithdrawalTransaction_ifOutputIsNotFinalized_reverts, we see a new mock call toIColosseum.getAssertion. If the assertion was recently created, we revert. Confirm that the contract under test expects the finalization to wait for the entire period.Implement a helper function to reduce duplication of finalization checks across tests, if the pattern recurs frequently.
packages/contracts/contracts/L1/Colosseum.sol (1)
753-821:⚠️ Potential issueValidate length checks on
_zkVmProof.publicValues.Line 764 slices bytes
[8:40]. If_zkVmProof.publicValuesis shorter than 40 bytes, this will revert without a clear error. Explicitly check the length to enhance clarity.- bytes32 srcOutput = bytes32(_zkVmProof.publicValues[8:40]); + require(_zkVmProof.publicValues.length >= 40, "Insufficient publicValues length"); + bytes32 srcOutput = bytes32(_zkVmProof.publicValues[8:40]);packages/contracts/contracts/L1/interfaces/IColosseum.sol (1)
1-145:⚠️ Potential issueMismatch in
proveFaultWithZkVmsignature and possible minor naming discrepancy.
- The interface defines
proveFaultWithZkVm(uint256, uint256, ZkVmProof), but the implementation inColosseum.solomits the_posparameter. This leads to an interface vs. implementation mismatch, potentially breaking external calls or tooling.- In the
OutputForceDeletedevent, the parameter is spelledasseterwhereas other references useasserter. Consider aligning naming for consistency.- function proveFaultWithZkVm(uint256 _outputIndex, uint256 _pos, ZkVmProof memory _zkVmProof) external; + // or remove `_pos` to match the contract’s actual function signature function proveFaultWithZkVm(uint256 _outputIndex, ZkVmProof memory _zkVmProof) external;- event OutputForceDeleted(uint256 indexed outputIndex, address indexed asseter, uint256 timestamp); + event OutputForceDeleted(uint256 indexed outputIndex, address indexed asserter, uint256 timestamp);
🧹 Nitpick comments (7)
packages/contracts/contracts/libraries/Types.sol (3)
152-160: Clarify doc comments for Challenge struct fields.
These doc strings describe the new fields but do not mention how the clock model resets or decrements. It might be useful to explicitly note that these times decrement as part of a chess-clock mechanism.Consider adding a brief line indicating how "turn" interacts with asserterTimeLeft and challengerTimeLeft.
191-213: Ensure Assertion lifecycle states are consistently updated.
The newAssertionstruct has fields likerejectedAtandacceptedAtbut no mention of validations that only one can be set. Ensure the contract code properly manages these timestamps and thenumberOfChallenges.- uint256 acceptedAt; - uint256 rejectedAt; + // Optionally track finalTimestamp to unify acceptedAt or rejectedAt logic if needed.
214-234: Add or fix reference to the challenge state diagram URL.
The doc mentions “// TODO fix the url” in the comment forAssertionStatus. Consider providing a valid link or removing it if the specs are internal.Would you like me to create a new doc comment block with the correct link or remove the placeholder?
packages/contracts/contracts/test/KromaPortal.t.sol (2)
312-334: Validate zero index finalization logic.
The code sets index 0 as “always finalized” and checks indexes 1+ for the finalization period. This is consistent with how the L2 outputs might handle the genesis block. Ensure that external documentation or references note this special-case logic.Add a quick comment clarifying that index 0 is a special-case finalized output.
995-999: Mocking oracle.isFinalized to return true.
This line ensures the test environment simulates a finalized output. Double-check that no subsequent lines incorrectly rely on the realL2OutputOracle.isFinalizedlogic.Consider naming the mock scenario clearly (e.g.,
mockFinalizedOutput()) to improve readability.packages/contracts/contracts/test/Colosseum.t.sol (1)
137-138: Private helper_createAssertion()usage.
Line 137 calls_createAssertion()immediately insetUp(). This pre-creates an assertion for the default flow. If you want a fresh environment in certain tests, consider making_createAssertion()optional or specifying why it’s always necessary.packages/contracts/contracts/L1/Colosseum.sol (1)
609-625: No event on acceptance of assertion.
acceptAssertionupdates the assertion status toACCEPTEDbut doesn’t emit an event (e.g.AssertionAccepted). Adding one may improve transparency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/contracts/.storage-layout(1 hunks)packages/contracts/contracts/L1/Colosseum.sol(22 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(6 hunks)packages/contracts/contracts/L1/interfaces/IColosseum.sol(1 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/Colosseum.t.sol(30 hunks)packages/contracts/contracts/test/CommonTest.t.sol(7 hunks)packages/contracts/contracts/test/KromaPortal.t.sol(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/contracts/L1/L2OutputOracle.sol
- packages/contracts/contracts/test/CommonTest.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (16)
packages/contracts/.storage-layout (2)
13-14:✅ Verification successful
Ensure these spacers are intentional for upgradability.
The newly addedspacer_1_0_32andspacer_2_0_32preserve space for future changes, which is a common pattern for maintaining storage compatibility in upgradable contracts. Please confirm that this is indeed intentional and aligns with the overall upgrade strategy.Would you like me to scan the repository for references or usage documentation of these spacer variables to confirm they match the intended layout design?
🏁 Script executed:
#!/bin/bash # Looking for references to the spacer fields in the codebase. rg --json "spacer_1_0_32|spacer_2_0_32"Length of output: 1193
Verification of Spacer Fields Confirmed
After scanning the repository, we found the declarations of
spacer_1_0_32andspacer_2_0_32inpackages/contracts/contracts/L1/Colosseum.sol(lines 86 and 91). This aligns with the expected pattern for reserving storage slots in upgradable contracts. Please confirm that this design fits with your overall upgrade strategy.
17-18:✅ Verification successful
Verify new mappings' ordering for storage layout.
The introduction ofassertionsat slot 5 andchallengesat slot 6 seems to reorder some storage variables. This can be risky in upgradable contracts if those slots were previously in use. Ensure no prior layout was dependent on these specific slot positions.
🏁 Script executed:
#!/bin/bash # Description: Verify references to older mappings or layout usage that might conflict. rg -A 5 "assertions" rg -A 5 "challenges"Length of output: 77384
Storage Layout Verified – No Conflicts Detected
The new mappings for
assertions(slot 5) andchallenges(slot 6) have been verified. Searches in the codebase (notably incontracts/L1/Colosseum.sol) confirm that these mappings are declared and referenced consistently, with no evidence of previous dependencies on these specific slot positions.
- In
Colosseum.sol, the mappings are introduced at the end of the storage layout and are used in expected ways.- No other file or test revealed reliance on older storage ordering that would conflict with the new layout.
Based on this verification, the storage ordering change appears safe for upgradeable contracts.
packages/contracts/contracts/libraries/Types.sol (2)
162-169:❓ Verification inconclusive
Confirm initialization patterns for new Challenge fields.
New fields (turn,asserterTimeLeft,challengerTimeLeft, etc.) require correct initialization. Verify that whenever aChallengestruct is instantiated in the Colosseum logic, theturnand time fields are properly set.
🏁 Script executed:
#!/bin/bash # Searching for Challenge struct instantiations to inspect initialization. ast-grep --pattern $'NewExpression (type_identifier (identifier) @t (#eq? @t "Challenge"))'Length of output: 92
Below is an updated review comment that reflects our current findings. The initial AST search produced no output, so please run a broader check (or manually inspect) for how the
Challengestruct is instantiated (for example, via a pattern likeChallenge({). This extra step is critical to verify that all new fields (includingturn,asserterTimeLeft,challengerTimeLeft, etc.) are appropriately initialized within the Colosseum logic.Action: Confirm initialization of Challenge fields
- Verify that every instantiation of the
Challengestruct (e.g. usingChallenge({ … })) correctly assigns values to the new fields.- If the automated search (e.g. using:
) does not return results, please perform a manual inspection of the Colosseum logic to ensure proper initialization.#!/bin/bash rg "Challenge\s*\(\{" -g "*.sol"
Below is an updated review comment that asks for further verification of the initialization patterns. Since our initial AST search returned no output, please run the following shell script to search for any instantiations of the
Challengestruct using a different pattern. This will help confirm whether the new fields are properly initialized in the Colosseum logic:Action: Confirm proper initialization of new
Challengefields
- Ensure that every instance where the
Challengestruct is instantiated correctly assigns values toturn,asserterTimeLeft,challengerTimeLeft, and the other new fields.- If the script above produces no results, please manually inspect the Colosseum logic to verify that the struct initialization (possibly via a literal struct initializer like
Challenge({ ... })) includes all the necessary field assignments.
🏁 Script executed:
#!/bin/bash # Search for Challenge struct instantiations using a literal pattern (e.g., Challenge({ ...) rg "Challenge\\s*\\(\\{" --context 3 -g "*.sol"Length of output: 168
Below is an updated review comment incorporating the current findings and a request for manual verification. Since our automated searches (both via AST and literal pattern scanning) did not return any results, it's important to have the developer manually confirm that all instances where the
Challengestruct is instantiated in the Colosseum logic correctly initialize the new fields.Action: Verify initialization of Challenge fields manually
- Ensure that every instantiation of the
Challengestruct (e.g., via literal initializers likeChallenge({ ... })) includes proper assignment forturn,asserterTimeLeft,challengerTimeLeft, and other new fields.- If possible, review the Colosseum logic where the
Challengestruct is created to confirm that no fields are left uninitialized.
172-190:❓ Verification inconclusive
Check for edge cases in Segment struct usage.
TheSegmentstruct introducespos,start, andendfields that appear critical for bisection logic. Confirm that off-by-one scenarios (e.g., whenpos == startorpos == end) are handled gracefully in the challenge contract.
🏁 Script executed:
#!/bin/bash # Look for usage of Segment struct to see how pos, start, end are validated. rg "Segment" -A 10Length of output: 78754
Edge-case verification for Segment usage in bisection logic
- The
Segmentstruct inpackages/contracts/contracts/libraries/Types.solis defined as expected.- Our investigation shows that the challenge contract (in files like
kroma-validator/challenger.goandkroma-validator/challenge/segments.go) uses the struct in its bisection logic. In particular, if the very first segment fails to match (i.e. when using a computed fault position that would be negative because it subtracts 1), the code explicitly errors out—ensuring the case wherepos == startis handled.- The next segments’ range is computed based on the offset (using
s.Start + position*s.Degree), which suggests that typical off-by-one issues are being minimized.- It would still be prudent to add or verify unit tests specifically covering scenarios where
posequals the starting boundary and where it may approach the ending boundary to ensure that all extreme cases (including a potentialpos == endscenario) are gracefully managed.packages/contracts/contracts/test/KromaPortal.t.sol (2)
20-22: Good practice calling super.setUp()
The override tosetUp()now correctly includessuper.setUp(), ensuring that any essential parent test setup logic is not skipped.
342-352:✅ Verification successful
Check partial finalization steps.
Multiple blocks are submitted, and finalization is tested at different indexes. It looks correct, but confirm there's no off-by-one error in referencing thenextOutputIndexand newly submitted outputs.
🏁 Script executed:
#!/bin/bash rg -A 5 "oracle\.nextOutputIndex"Length of output: 8977
No Off-by-One Issue Detected
After running the grep search across the tests, the usage of
oracle.nextOutputIndex()in KromaPortal.t.sol (lines 342-352) is consistent with similar patterns found in AssetManager.t.sol, L2OutputOracle.t.sol, and other test files. The logic for submitting outputs and updating indexes shows no sign of any off-by-one error.packages/contracts/contracts/test/Colosseum.t.sol (2)
52-55: Use self-explanatory constant names.
The new constantsCHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEadd clarity over using raw numbers. Looks good for readability, no concerns.
62-83: Added events improve debugging.
AssertionCreated,ChallengeCreated,ChallengerTimedOut, andChallengeCanceledare valuable for external monitoring of challenge states. Solid addition.packages/contracts/contracts/L1/Colosseum.sol (8)
8-8: Good use of immutable parameters and aligned storage.These newly added fields (
L2_ORACLE,MAX_CLOCK_DURATION_SECONDS,L2_ORACLE_SUBMISSION_INTERVAL,SECURITY_COUNCIL,GUARDIAN_PERIOD, etc.) and the spacer variables for layout preservation appear well-structured. Marking them asimmutableis a recommended best practice to reduce gas costs and ensure their values can’t be changed after construction.Also applies to: 52-56, 60-60, 68-71, 74-74, 79-81, 83-83, 86-86, 88-88, 91-91
93-101: Mappings for public inputs and deleted outputs look good.Using a boolean mapping for
verifiedPublicInputsis straightforward. Likewise, storing rolled-back outputs indeletedOutputsclarifies state transitions. Ensure thorough testing for re-entrancy or repeated rollback attempts.
104-104: NewassertionsandonlyL2OutputOraclemodifier.Defining
mapping(uint256 => Types.Assertion) public assertionsis clear for referencing each output’s assertion data. TheonlyL2OutputOraclemodifier effectively restricts privileged calls to your oracle contract.Also applies to: 106-106, 114-119
122-132: Event definitions align well with contract semantics.The
AssertionCreatedandOutputForceDeletedevents (with newly indexed parameters) enrich on-chain observability. Indexing theasserteraddress is especially useful for filtering logs.Also applies to: 160-160, 198-198, 203-203
289-350: New errors and revert messages are descriptive.These new error declarations (
L1Reorged,AlreadyVerifiedPublicInput,CannotCancelChallenge, etc.) accurately capture likely failure scenarios. Grouping them clarifies potential edge cases.
386-414: Check the range for_outputIndexincreateAssertion.Currently, there is no explicit upper bound check to ensure
_outputIndexis within valid range compared to whatL2_ORACLEcan handle. Consider verifying_outputIndex <= L2_ORACLE.latestOutputIndex()if relevant.
424-495: Consider potential underflow in challenge time-left calculations.When subtracting timings (
challenge.asserterTimeLeft -= elapsed, etc.), Solidity 0.8.x will revert on underflow, which is safe. However, be sure to add test coverage for boundary conditions whereelapsed == challenge.asserterTimeLeftor similar.
881-908: Time-out and bisection checks appear robust.The
_challengerTimeoutlogic properly decreasesassertion.numberOfChallengesand handles slash or bond release. The helpers_isTimeoutand_isAbleToBisectare straightforward.Also applies to: 918-918, 999-1010
| @@ -100,7 +100,7 @@ contract AssetManagerTest is ValidatorSystemUpgrade_Initializer { | |||
| for (uint256 i = oracle.nextOutputIndex(); i <= terminateOutputIndex; i++) { | |||
| _submitOutputRoot(pool.nextValidator()); | |||
| } | |||
| vm.warp(oracle.finalizedAt(terminateOutputIndex)); | |||
| // vm.warp(oracle.finalizedAt(terminateOutputIndex)); | |||
There was a problem hiding this comment.
Is it should removed? or did you leave this line on purpose?
There was a problem hiding this comment.
I deleted this source code since it no longer gets finalized simply after a certain period of time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/contracts/contracts/libraries/Types.sol (3)
162-168: Potential integer optimization.
Usinguint256forturnmight be overkill if the number of turns is bounded, but it doesn't introduce a functional bug. If efficiency is a concern, you could consider a smaller integer type. Otherwise, the structure here looks correct.
172-189: Segment struct is clearly defined.
All fields are well-documented, making usage unambiguous. Consider smaller data types (e.g.uint64) for fields likepos,start, andendif they represent block numbers or similarly bounded values, to save gas.
212-231: Replace the TODO in the docstring.
The enum fields and usage are clear. There's a TODO comment about fixing the URL for reference. Consider updating or removing it to keep docs accurate.Would you like me to open a new issue for updating the documentation URL?
packages/contracts/contracts/L1/L2OutputOracle.sol (1)
485-516: isFinalized logic review.
The function checksRESTORED→ final,ACCEPTED→ wait for guardian period,IN_PROGRESS→ no challenges + time spent. This covers all new statuses. The fallthrough tofalseforREJECTEDor unknown statuses is sound.Consider adding an explicit case for
REJECTEDfor clarity, though it’s technically captured by the final else block.packages/contracts/contracts/L1/Colosseum.sol (2)
86-91: Legacy space placeholders.
These dummy spacers are presumably for storage layout preservation. Confirm they don't cause confusion or break future upgrades.
1012-1024: _isTimeout logic.
Comparesblock.timestamp - updatedAtto the relevant participant’s remaining time. This is correct for a chess-clock model.Consider clarifying in docstrings that “timestamp differences” can be manipulated slightly if blocks are produced artificially quickly or slowly, depending on chain conditions.
packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol (1)
1-92: Add NatSpec comments for clarity.This interface is quite extensive. Adding NatSpec comments (e.g.,
@dev,@param, and@return) for each function and event will improve clarity for future maintainers, making it easier for them to understand the intended usage and expected behavior of each item in the interface.packages/contracts/contracts/test/Colosseum.t.sol (1)
346-347: Placeholder test functiontest_initialize_succeeds().Currently empty, but no errors. Consider adding future checks once initialization logic is introduced.
Would you like help adding coverage for any initialization logic to ensure everything is thoroughly tested?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/contracts/contracts/L1/Colosseum.sol(25 hunks)packages/contracts/contracts/L1/L2OutputOracle.sol(7 hunks)packages/contracts/contracts/L1/ValidatorManager.sol(1 hunks)packages/contracts/contracts/L1/interfaces/IL2OutputOracle.sol(1 hunks)packages/contracts/contracts/libraries/Types.sol(1 hunks)packages/contracts/contracts/test/Colosseum.t.sol(30 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts/contracts/L1/ValidatorManager.sol
🔇 Additional comments (57)
packages/contracts/contracts/libraries/Types.sol (2)
152-159: Docstring alignment looks good.
The field annotations match the newly introduced fields in theChallengestruct. No issues found.
191-210: New Assertion struct is coherent.
All fields (e.g.,assertedAt,acceptedAt,rejectedAt) align with the challenge/verification flow described. Ensure all status updates happen consistently to avoid partial states.packages/contracts/contracts/L1/L2OutputOracle.sol (6)
10-11: Imports look valid.
ImportingIValidatorPoolandIColosseumis consistent with the references in this contract.
22-24: Minor docstring update.
Renaming or clarifying “validator pool contract” in the docstring is fine. The usage of the field looks correct.
27-27: Docstring clarity.
Stating “Validator manager contract” is consistent. No functional issues.
120-121: Constructor parameters refactor.
Switching to addresses for_validatorPooland_validatorManagerbefore casting is standard. Ensure these addresses always point to valid deployed contracts to prevent misconfiguration.Please confirm non-zero code size at deployment or provide a comment guaranteeing safe contract initialization.
Also applies to: 135-136
273-275: Assertion creation call.
CallingcreateAssertionin the Colosseum only whenoutputIndex > 0effectively excludes the genesis output. This follows the new “genesis is auto-finalized” logic.
312-336: New finalization retrieval functions.
getLatestFinalizeOutputandgetLatestFinalizeOutputIndexhelp clarify currently finalized outputs. Implementation is succinct.packages/contracts/contracts/L1/Colosseum.sol (14)
8-8: New IL2OutputOracle import.
No issues. The contract references it appropriately.
53-55: Docstring & timeout variable.
IntroducingMAX_CLOCK_DURATION_SECONDSis a clear improvement for the chess-clock approach. Keep an eye on overflow if large.
58-60: L2_ORACLE reference updated.
Using an interface is a good abstraction. Ensure external calls handle potential reverts gracefully.
63-65: ZKProofVerifier reference.
Similar interface-based pattern. Implementation detail separation is good.
79-81: Guardian period definition.
Security Council logic referencesGUARDIAN_PERIODeffectively. This aligns with the finalization approach.
103-112: Merging assertion & challenge mappings.
Storingassertions[_outputIndex]andchallenges[_outputIndex][challenger]is logical. Access patterns appear straightforward.
113-119: onlyL2OutputOracle modifier.
RestrictingcreateAssertionto the L2OutputOracle contract helps enforce correct call flow. Implementation is good.
136-139: Updated constructor doc comments.
They now match the actual parameters. No functional issues.
374-374: Constructor parameter & assignment.
Casting_l2OracletoIL2OutputOracleis standard. Check code-size if needed. SettingGUARDIAN_PERIODandMAX_CLOCK_DURATION_SECONDSis consistent with the system design.Please confirm these addresses consistently reference valid bytecode in production deployment.
Also applies to: 379-379, 381-381, 385-385, 386-386
390-410: createAssertion function.
Excludes genesis outputs by design. Properly reverts if assertion already exists. Marks newAssertionasIN_PROGRESS. Straightforward logic.
442-448: Rejecting challenge creation if assertion is REJECTED/RESTORED.
Prevents unnecessary challenges. This ensures consistent status management.
462-486: Challenge initialization logic.
- The chess-clock approach is enforced by subtracting elapsed time from the challenger’s available time.
- The midpoint calculations for
segment.posuse integer division. Watch for off-by-one issues but the code clarifies next steps in bisection.Consider adding tests for odd/even block ranges ensuring the midpoint is correct.
622-638: acceptAssertion function.
Requires zero challenges and thatblock.timestamphas exceededMAX_CLOCK_DURATION_SECONDSfromassertedAt. This properly finalizes the assertion flow.
986-988: getAssertion function.
A simple getter for external use. Helps with cross-contract calls or off-chain lookups.packages/contracts/contracts/test/Colosseum.t.sol (35)
52-54: Constants naming is clear.Defining constants for
CHALLENGER_TURN,ASSERTER_TURN, andREADY_TO_PROVEclarifies role-based logic within the challenge sequence. No issues here.
62-67: Event definition forChallengeCreated.The event parameters are well-structured, clearly indicating who created the challenge, for which output index, and the timestamp. Looks good.
68-72: Event definition forChallengerTimedOut.Properly clarifies who timed out, on which output index, and the relevant timestamp. No issues found.
73-77: Event definition forChallengeCanceled.The parameters help identify which challenge was canceled and when. Straightforward usage; looks fine.
87-92: Constructor arguments in test setup.Passing additional parameters (
address(securityCouncil),guardianPeriod,maxClockDuration) to theMockColosseumconstructor seems consistent with the new challenge structure. Ensure that the underlying contract references these parameters correctly. Otherwise, no concerns.
132-133: Helper call_createAssertion().Invoking
_createAssertion()immediately after test fixture setup is a neat way to prepare the environment with an initial assertion. No issues here.
140-148: Logic within_getOutputRoot().This function returns different root values based on the
_se 10000 nder,_blockNumber, and test data constants. This approach is suitable for simulating valid/invalid outputs during testing. The branching appears logically correct, and no obvious errors stand out.
157-181: Fault detection_detectFault().Partitioning the range
[start, end]based on which output is correct is a good approach. The step to bisect or return a single location looks consistent with the standard fault bisection technique. No issues identified.
194-214: Assertion creation_createAssertion().The test ensures a new L2 output is submitted before storing the assertion. Emitting
OutputSubmittedevent is verified viavm.expectEmit(), which is a thorough approach. Looks correct.
216-251: Challenge creation_createChallenge().You retrieve final outputs, verify an invalid output root, and emit
ChallengeCreated. The function ensures proper setup ofTypes.Challenge. The checks for existing challenge statuses are appropriate. No issues found.
253-286: Bisect logic_bisect().By computing
positionoutside and verifying it belongs to the correct half, you effectively narrow down the segment. Trackingturnand ensuring time constraints are consistent with typical challenge protocols. The structure appears correct.
288-312: Proof of fault_proveFault().Internally uses
_doProveFault()to update states, ensuring the replaced output belongs to the challenger upon proof. This aligns with the invalid output scenario. Looks solid.
314-322: Implementation of_doProveFault().Invoking
proveFaultWithZkVmand verifying the proof's public input hash ensures correct ZK proof mechanism. Straightforward approach. No concerns here.
404-418: Test casetest_bisect_afterChallengerTimedOut_reverts().The test intentionally pushes past
challengerTimeLeftthen tries to bisect. Reverting withChallengerTimeoutis correct. No issues.
420-436: Testtest_bisect_afterAsserterTimedOut_reverts().Similarly finalizes time-based logic for asserter timeouts. The revert outcome aligns with
AsserterTimeout. Good coverage.
438-454: Testtest_challengerTimeout_succeeds().Ensures the challenger can be timed out if they fail to proceed before
challengerTimeLeftruns out. EmittedChallengerTimedOutevent is confirmed. Solid verification.
461-462: Testtest_createChallenge_notSubmittedOutput_reverts().Appropriately checks the revert scenario when challenging a non-existent output index. Basic but important negative test.
464-476: Testtest_createChallenge_afterChallengeProven_reverts().Validates that once a challenge for a given output is resolved (proven fault), no new challenge can be recreated. Logical restriction enforced.
478-502: Testtest_createChallenge_afterDismissed_reverts().Verifies that an output which had its challenge dismissed cannot be challenged again. Properly ensures final state is not challengeable. No concerns.
509-510: Testtest_createChallenge_wrongFork_reverts().Checks for mismatch in the L1 block reference. Enforcing
L1Reorgedrevert is a necessary guard. Good negative coverage.
522-537: Testtest_bisect_withBadPos_reverts().Ensures that an out-of-bounds or invalid
positionduring bisect triggersInvalidPosrevert. Proper boundary validation.
544-552: Testtest_bisect_ifNotYourTurn_reverts().Makes sure only the correct party can proceed with a bisect step. A solid concurrency safeguard in the challenge protocol.
561-565: Testtest_bisect_whenAsserterTimedOut_reverts().Proves that once the asserter has timed out, they cannot proceed. Matches the protocol flow. Looks correct.
644-645: Withintest_proveFaultWithZkVm_succeeds().Setting
outputIndexfromtargetOutputIndexfor the challenge test. Straightforward indexing usage. No issue.
664-692: Continuation oftest_proveFaultWithZkVm_succeeds().Bisection steps are exhausted until the challenge reaches
READY_TO_PROVE. Then_proveFault()ensures the malicious output root is replaced. Logic is well tested.
727-728: Withintest_proveFault_cancelChallenge_succeeds().Shows how a separate challenger’s successful fault proof triggers a scenario where another challenge for the same output can be canceled. Good test coverage.
731-735: Bond refund logic intest_proveFault_cancelChallenge_succeeds().Invoking
proveFaultWithZkVmagain to finalize bond refunds. Clear demonstration of the final step for challenge cancellation. Looks valid.
752-752: Line 752 referencingtest_dismissChallenge_succeeds().Calling
test_proveFaultWithZkVm_succeeds()to replicate a proven fault scenario before dismissing. This is a neat reusability of existing tests.
770-771: Testtest_dismissChallenge_notSecurityCouncil_reverts().Ensures that only the security council can dismiss challenges. Proper role-based restriction.
807-808: Testtest_dismissChallenge_invalidOutputGiven_reverts()block checks.Guarantees revert if the given output root mismatch is found. Good coverage for the valid/invalid root scenario.
811-812: Continuation intest_dismissChallenge_invalidOutputGiven_reverts().Second revert check. This is thorough coverage ensuring correct input validation on multiple calls.
829-830: Testtest_dismissChallenge_invalidAddressGiven_reverts().Rejects an empty address for either the new or old submitter. Ensures that the dismissal logic is referencing real addresses.
902-905: Testtest_cancelChallenge_noChallenge_reverts().Correctly reverts if there’s no active challenge to cancel. Basic but essential boundary test.
939-946: Testtest_cancelChallenge_whenChallengerTimedOut_reverts().When the challenger is already timed out, the challenge can’t be canceled by them. Matches the contract state logic.
995-1002: Testtest_forceDeleteOutput_alreadyDeletedOutput_reverts().Ensures double-deletions of the same output are prevented. The revert helps maintain consistent states.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/contracts/contracts/test/Colosseum.t.sol (2)
133-134: _createAssertion clarity.This helper method is a useful abstraction for setup, but confirm it aligns precisely with
Colosseum.createAssertion. Consider adding coverage for re-assertion attempts or zero output indices to confirm the contract’s revert conditions.
693-717: test_proveFaultWithZkVm_succeeds coverage.
- This test ensures final status transitions to
NONE, indicating the challenge is resolved.- Verifies that the old L2 output root is replaced and stored in
deletedOutputs, matching the updated contract logic.- Confirms the public input hash matches.
Add edge case coverage for scenarios like immediateREADY_TO_PROVEafter a single bisection or minimal clock usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/contracts/contracts/L1/Colosseum.sol(24 hunks)packages/contracts/contracts/test/Colosseum.t.sol(30 hunks)packages/contracts/contracts/test/CommonTest.t.sol(7 hunks)packages/contracts/contracts/test/mock/MockColosseum.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts/contracts/test/mock/MockColosseum.sol
- packages/contracts/contracts/test/CommonTest.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run tests
🔇 Additional comments (11)
packages/contracts/contracts/L1/Colosseum.sol (7)
52-56: Clarify usage of MAX_CLOCK_DURATION_SECONDS.This new immutable ensures a deadline for asserter or challenger actions. Consider adding more detail in the NatSpec on how it interacts with other time-dependent variables, such as
CHALLENGE_GRACE_PERIOD.
399-419: Ensure comprehensive testing for createAssertion.The new
createAssertionfunction looks well-defined. However, verify proper coverage of edge cases like repeated assertions, zero_outputIndex, and calls from unexpected external contracts. Confirm that subsequent calls in your test suite fully validate timing and status transitions.
508-599: Review the bisect function carefully for boundary conditions.
_pos != (challenge.segment.start + challenge.segment.end) / 2triggersInvalidPos, ensuring the midpoint is correct. Odd intervals need thorough testing to confirm no off-by-one issues.- Time accounting (lines 543-553) updates only the current actor's clock. Make sure tests verify correct time consumption across multiple consecutive bisections.
- The revert conditions for
AsserterTimeoutandChallengerTimeoutappear correct, but confirm coverage for near-edge scenarios (e.g., time left equals 0 exactly).
767-846: Assess the _proveFault function for safety and clarity.
- Verification step ultimately calls out to the
ZK_PROOF_VERIFIER. Confirm that a failing verification reverts properly at the library call level.- The function modifies critical states (e.g., sets
assertion.status = REJECTED, replaces the L2 output root). Verify that partial reverts or halting at any point cannot leave the system in an inconsistent state.- The logic for checkpointing the old output in
deletedOutputs[_outputIndex]is helpful, but ensure you handle reentrant or cross-function calls gracefully.
995-997: Return type verification.
getAssertionstraightforwardly returns a struct. This is clear and correct, but note that external consumers will see the entireAssertionstructure. Verify that no sensitive data is privately stored in this struct that needs restricting.
1021-1037: Validate timeouts in _isTimeout.
- The chess-clock logic is split between asserter and challenger segments, plus an extra
CHALLENGE_GRACE_PERIODif bisection is no longer possible. Ensure that the mixed usage of different offsets is fully tested.- The function reverts on an unexpected actor, but consider whether a more descriptive revert might aid debugging.
429-506:❓ Verification inconclusive
Double-check state transitions in createChallenge.
- The function reverts if a challenge is still active (and not timed out) but then calls
_challengerTimeout. Make sure the logic flow (lines 445-449) is explicit in tests to avoid confusion.- The new midpoint calculation
(start + end) / 2(line 492-493) is subject to integer division truncation. Confirm this is the intended behavior by adding test scenarios with odd intervals.- Consider whether retrieving
targetOutput.submitteron line 464 and disallowing the same address as the challenger is sufficient.
Double-check state transitions and integer division in
createChallenge.
- Verify that the function correctly reverts when an active challenge exists (i.e., when
_challengeStatus(challenge) != ChallengeStatus.CHALLENGER_TIMEOUT), and that calling_challengerTimeoutin those cases is intentional. Please add tests that make the different state transitions explicit (especially covering lines 445–449).- Confirm that the midpoint calculation
(start + end) / 2(lines 492–493) uses integer division as intended. If truncation is acceptable, include test cases for odd-numbered intervals; otherwise, consider revising the logic.- Ensure that the check
if (msg.sender == targetOutput.submitter)(line 464) reliably prevents the submitter from also being the challenger. Review edge cases to verify this restriction is sufficiently robust.packages/contracts/contracts/test/Colosseum.t.sol (4)
52-55: Constants definition matches contract logic.The newly introduced constants (
CHALLENGER_TURN,ASSERTER_TURN,READY_TO_PROVE) closely mirror the contract statuses. Keep them updated in sync with the main contract’sChallengeStatusto avoid confusion.
62-77: Event emissions in tests.The events
ChallengeCreated,ChallengerTimedOut, andChallengeCanceledare now explicitly tested. Ensure each test thoroughly checks both the event fields and the final on-chain state (e.g., updated statuses, bond changes).
218-234: _createChallenge enhancements.The test uses
_createChallengeto trigger multiple validations:
- Checking the new event emission via
vm.expectEmit.- Ensuring clock times are derived properly from
assertion.assertedAt.- Validating updated segment boundaries (start, end, pos).
It appears robust, but consider corner cases (e.g., only 1 block difference between
startandend) to capture off-by-one scenarios in the test.
796-815: Check dismissChallenge logic with proven faults.
- The test verifies that
dismissChallengereverts if the output is not deleted, aligning with the contract’s guard conditions.- Confirm that re-initializing
verifiedPublicInputs[publicInputHash]to false is tested for repeated calls todismissChallengeor attempts at re-challenging.- It might be useful to show the final status of the assertion after dismissal in the test to confirm the code flows as expected.
| @@ -426,106 +502,113 @@ contract Colosseum is Initializable, ISemver { | |||
| L2_ORACLE.VALIDATOR_POOL().addPendingBond(_outputIndex, msg.sender); | |||
| } | |||
There was a problem hiding this comment.
Remove these lines L421-L427 except L424
There was a problem hiding this comment.
It makes sense to handle this in a separate PR as you suggested.
|
Coderabbit is going wild… Is there anything we can do to stop it? |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
I disabled a coderabbit in this repository. |
e391b14
into
feat/stage-1-fault-proof-improvement
Description
To achieve stage 1, this PR modifies the bisection process and updates the criteria for output finalization. Additionally, this PR only includes changes to the contract components. The validator will be developed in a separate PR.