feat(protocol): introduce ForkManager to improve protocol fork management #18508
Conversation
|
The storage layout of ForkManager is compatible with the layout of TaikoL1. 
|
feat(protocol): introduce ForkManager to improve protocol fork management
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| /// routing logic. | ||
| /// @param _selector The function selector of the call. | ||
| /// @return A boolean value indicating whether the call should be routed to the old fork. | ||
| function shouldRouteToOldFork(bytes4 _selector) internal pure virtual returns (bool) { } |
There was a problem hiding this comment.
If a function has the same selector in oldFork and newFork, how can we clarify?
| /// PROXY -> FORK_MANAGER --| | ||
| /// +--> oldFork | ||
|
|
||
| contract ForkManager is UUPSUpgradeable, Ownable2StepUpgradeable { |
There was a problem hiding this comment.
Do we need an init method here for proxy deployment?
| /// PROXY -> FORK_MANAGER --| | ||
| /// +--> oldFork | ||
|
|
||
| contract ForkManager is UUPSUpgradeable, Ownable2StepUpgradeable { |
There was a problem hiding this comment.
An instance of this ForkManager (even behind proxy) is not really upgradeable, or at least not the 2 main variables (oldFork, newFork).
Since you placed here a constructor and not init, i suppose it is intentional from your side, but in this case we dont need the Upgradeable pattern. (Or if we do want to do that, just remove the immutability of the 2 fields).
| return true; | ||
| } | ||
|
|
||
| function _fallback() internal virtual { |
There was a problem hiding this comment.
Should not we restrict reentrancy ? 🤔 Not sure tho, cause propose/prove already protected.
Currently, the TaikoL1 protocol required each fork's codebase to contain implementations for both the current fork and the next fork within the same contract. This created unnecessary code complexity and tight coupling between fork versions.
With the new ForkManager, we can:
This change improves code maintainability and makes the fork upgrade process more modular.
BTW, I'm aware of Diamond proxy design but I'm in favor of this ForkManager implementation for its simplicity.