Attempt to remove circular dependencies#149
Conversation
WalkthroughOwnership and explicit initialization patterns were introduced to Changes
Sequence Diagram(s)sequenceDiagram
participant Owner
participant CheckpointTracker
participant ProverManager
Owner->>CheckpointTracker: deploy (with deployer address)
Owner->>CheckpointTracker: updateProverManager(proverManager)
CheckpointTracker-->>Owner: ProverManagerUpdated event
ProverManager->>CheckpointTracker: proveTransition(...)
CheckpointTracker->>ProverManager: require proverManager set
sequenceDiagram
participant Owner
participant TaikoInbox
participant ProposerFees
participant User
Owner->>TaikoInbox: deploy (with deployer address)
Owner->>TaikoInbox: updateProposerFees(proposerFees)
TaikoInbox-->>Owner: ProposerFeesInitialized event
User->>TaikoInbox: publish(...)
TaikoInbox->>User: require proposerFees set (if not zero)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Changes to gas cost
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/protocol/taiko_alethia/TaikoInbox.sol (1)
35-39: Consider consistent spelling throughout the codebase.The error message uses British spelling "initialised" while the modifier name uses American spelling "Initialized". Consider standardizing on one spelling convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/libs/LibProvingPeriod.sol(2 hunks)src/protocol/BaseProverManager.sol(1 hunks)src/protocol/CheckpointTracker.sol(2 hunks)src/protocol/ICheckpointTracker.sol(1 hunks)src/protocol/IInbox.sol(2 hunks)src/protocol/taiko_alethia/TaikoInbox.sol(3 hunks)test/CheckpointTracker.t.sol(3 hunks)test/mocks/MockCheckpointTracker.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
test/mocks/MockCheckpointTracker.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
src/protocol/BaseProverManager.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
src/libs/LibProvingPeriod.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
src/protocol/CheckpointTracker.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
test/CheckpointTracker.t.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
🔇 Additional comments (24)
src/libs/LibProvingPeriod.sol (1)
55-55: LGTM! Spelling corrections improve consistency.The spelling corrections from "initialized" to "initialised" align with the British English spelling convention used throughout the codebase and maintain consistency with similar changes in other files.
Also applies to: 90-90
src/protocol/IInbox.sol (2)
20-21: Well-designed event for initialization transparency.The
ProposerFeesInitialisedevent provides clear visibility into when the proposer fees contract address is set, which is important for the new initialization pattern.
42-44: Good interface design for breaking circular dependencies.The
initializeProposerFeesfunction effectively addresses the circular dependency issue by allowing post-deployment initialization of the proposer fees contract address. This enables contracts to be deployed independently and then linked together.Ensure that implementations of this interface include proper access controls (owner-only) and prevent re-initialization to maintain security.
src/protocol/ICheckpointTracker.sol (2)
21-23: Excellent use of indexed event parameter.The
ProverManagerInitialisedevent with indexedproverManagerparameter enables efficient filtering and searching, which is particularly useful for tracking initialization across multiple deployments.
28-30: Consistent initialization pattern for breaking circular dependencies.The
initializeProverManagerfunction follows the same pattern as the IInbox interface, providing a consistent approach to resolving circular dependencies between CheckpointTracker and ProverManager contracts.Verify that the implementation includes proper access controls and one-time initialization enforcement similar to the IInbox implementation.
src/protocol/BaseProverManager.sol (1)
74-74: LGTM! Spelling correction maintains consistency.The spelling correction from "initialized" to "initialised" aligns with the consistent British English spelling convention used throughout the codebase.
test/mocks/MockInbox.sol (1)
19-21: Consistent mock implementation pattern.The
initializeProposerFeesfunction follows the established mock pattern by immediately reverting withNotImplemented(), which is consistent with the existingpublishfunction and appropriate for test scenarios where initialization logic is not needed.test/mocks/MockCheckpointTracker.sol (1)
27-28: LGTM!The empty implementation of
initializeProverManageris appropriate for a mock contract and correctly implements the updatedICheckpointTrackerinterface.src/protocol/taiko_alethia/TaikoInbox.sol (5)
13-15: LGTM!The addition of
Ownableinheritance provides the necessary access control for the initialization pattern.
30-33: LGTM!The change from immutable to mutable
proposerFeeswith theproposerFeesInitialisedflag correctly implements the delayed initialization pattern.
48-60: LGTM!The constructor changes correctly remove the circular dependency by eliminating the
_proposerFeesparameter and adding ownership control.
62-69: LGTM!The
initializeProposerFeesfunction correctly implements one-time initialization with proper access control, validation, and event emission.
72-72: LGTM!Adding the
checkProposerFeesInitializedmodifier ensures thatproposerFeesis properly initialized before any publication operations.src/protocol/CheckpointTracker.sol (6)
9-11: LGTM!The addition of
Ownableinheritance provides the necessary access control for the initialization pattern.
22-24: LGTM!The change from immutable to mutable
proverManagerwith the initialization flag correctly implements the delayed initialization pattern.
26-30: LGTM!The modifier correctly enforces that the prover manager is initialized before use.
36-49: LGTM!The constructor changes correctly remove the circular dependency by eliminating the
_proverManagerparameter and adding ownership control.
51-58: Verify the intentional design of allowing zero address.The function explicitly allows setting
proverManagerto zero address, which would makeproveTransitioncallable by anyone (due to the check on line 70). Please confirm this is the intended behavior.
64-71: LGTM!The initialization check and permission logic correctly implement the design where a zero
proverManagerallows anyone to prove transitions.test/CheckpointTracker.t.sol (5)
18-33: LGTM!The test setup correctly reflects the new constructor signature and initialization pattern.
56-62: LGTM!The test correctly verifies that the prover manager can be initialized by the owner.
64-70: LGTM!The test correctly verifies that
proveTransitionreverts when the prover manager is not initialized.
72-78: LGTM!The test correctly verifies that only the owner can initialize the prover manager.
80-84: LGTM!The test correctly verifies that the prover manager cannot be initialized more than once.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/protocol/CheckpointTracker.sol(2 hunks)src/protocol/taiko_alethia/TaikoInbox.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/protocol/CheckpointTracker.sol (1)
Learnt from: nikeshnazareth
PR: #134
File: test/ProverManager/InvariantTest.t.sol:10-20
Timestamp: 2025-06-24T01:29:22.272Z
Learning: In ProverManager invariant tests, it's acceptable to rely on Solidity mapping behavior that returns zero values for non-existent keys when testing that periods are unused. The zero values serve as a proxy to confirm that a period is not initialized, which is the intended behavior for the invariant tests.
🔇 Additional comments (13)
src/protocol/taiko_alethia/TaikoInbox.sol (6)
13-14: LGTM!The import of OpenZeppelin's
Ownablecontract is appropriate for implementing ownership control.
15-15: LGTM!Inheriting from
Ownableenables proper ownership control for the initialization pattern.
30-30: LGTM!Converting
proposerFeesfromimmutableto mutable enables post-deployment initialization, which is necessary for breaking the circular dependency.
33-37: LGTM!The modifier properly enforces that
proposerFeesmust be initialized before use, providing a safety guard against calling functions with uninitialized dependencies.
46-58: LGTM!The constructor changes align with the circular dependency resolution approach by removing the direct
proposerFeesdependency and introducing owner-based initialization.
69-69: LGTM!The
checkProposerFeesInitializedmodifier ensures thatproposerFeesis initialized before the publish function can be called, maintaining contract safety.src/protocol/CheckpointTracker.sol (7)
9-10: LGTM!The import of OpenZeppelin's
Ownablecontract is appropriate for implementing ownership control.
11-11: LGTM!Inheriting from
Ownableenables proper ownership control for the initialization pattern.
22-25: LGTM!The design using a separate boolean to track initialization status is well-thought-out, especially since the prover manager can legitimately be set to the zero address, making address-based initialization checks insufficient.
27-31: LGTM!The modifier properly enforces that the prover manager must be initialized before use, providing essential safety for the contract's operation.
38-40: LGTM!The constructor changes align with the circular dependency resolution approach by removing the direct
proverManagerdependency and introducing owner-based initialization.
54-59: LGTM!The initialization function properly prevents double initialization and allows setting the prover manager to the zero address as intended. The explicit comment clarifying this behavior is helpful.
67-67: LGTM!The
checkProverInitializedmodifier ensures that the prover manager is initialized beforeproveTransitioncan be called, maintaining contract safety.
|
Sorry, for being slow on this - I just caught up. It seems a bit opinionated to me, so I have some suggestions: Firstly, what was the rationale for removing But more importantly, I don't think we need to insist on any prover or proposer fees initializer requirements. As I understand it:
So my suggested pattern is:
If anyone wants to deploy all contracts together atomically, they should create a single transaction that:
What do you think? I'm happy to make the change if you agree. |
I initially inherited the ownable contract but i didnt want to add to much bloat to the contract (adds renounce and change owner ..). Happy to put it back if you think its cleaner.
Yes technically the initialiser also allows a zero prover manager. I just added the "must be initialised" check to prevent mistakingly forgetting to set one. In this case. you would be forced to set it to zero if thats what you intended.
Yeah makes sense, i just wanted to keep the existing logic the same. Currently it would fail if you didnt set the prover manager so i kept that. |
|
@nikeshnazareth i made the changes you proposed please have a look! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
gas-reports/minimal_inbox_publish.json(1 hunks)snapshots/TaikoInboxTest.json(1 hunks)src/protocol/CheckpointTracker.sol(2 hunks)src/protocol/ICheckpointTracker.sol(1 hunks)src/protocol/IInbox.sol(2 hunks)src/protocol/taiko_alethia/TaikoInbox.sol(4 hunks)test/CheckpointTracker.t.sol(4 hunks)test/Gas/Inbox.t.sol(1 hunks)test/mocks/MockCheckpointTracker.sol(1 hunks)test/mocks/MockInbox.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- snapshots/TaikoInboxTest.json
🚧 Files skipped from review as they are similar to previous changes (8)
- test/mocks/MockInbox.sol
- test/mocks/MockCheckpointTracker.sol
- src/protocol/ICheckpointTracker.sol
- test/Gas/Inbox.t.sol
- src/protocol/IInbox.sol
- src/protocol/CheckpointTracker.sol
- src/protocol/taiko_alethia/TaikoInbox.sol
- test/CheckpointTracker.t.sol
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nikeshnazareth
PR: OpenZeppelin/minimal-rollup#146
File: src/protocol/ERC20Bridge.sol:129-135
Timestamp: 2025-08-01T09:46:40.439Z
Learning: In ERC20Bridge and similar bridge contracts, when deploying bridged tokens, it's important to check both _processed[id] for the specific initialization and _deployedTokens[key] for the original token address. The _processed check alone is insufficient because token upgrades that change metadata (name, symbol, decimals) would generate different initialization IDs, potentially allowing multiple bridged tokens for the same original token address.
🪛 Biome (2.1.2)
gas-reports/minimal_inbox_publish.json
[error] 1-1: Property key must be double quoted
(parse)
[error] 1-2: Property key must be double quoted
(parse)
3 participants
This is an example of how to fix #126.
Main issue is adding only modifier which adds overhead.
To make it even more 'trustless' could set owner to zero address after initialising prover manager.
Summary by CodeRabbit
New Features
Tests