Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol): introduce ForkManager to improve protocol fork management #18508

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/protocol/contract_layout_layer1.md
Original file line number Diff line number Diff line change
Expand Up @@ -948,3 +948,14 @@
| latestProofHash | mapping(uint256 => mapping(uint256 => bytes32)) | 255 | 0 | 32 | GuardianProver |
| __gap | uint256[45] | 256 | 0 | 1440 | GuardianProver |

## ForkManager
| Name | Type | Slot | Offset | Bytes | Contract |
|---------------|-------------|------|--------|-------|---------------------------------------------------|
| _initialized | uint8 | 0 | 0 | 1 | ForkManager |
| _initializing | bool | 0 | 1 | 1 | ForkManager |
| __gap | uint256[50] | 1 | 0 | 1600 | ForkManager |
| _owner | address | 51 | 0 | 20 | ForkManager |
| __gap | uint256[49] | 52 | 0 | 1568 | ForkManager |
| _pendingOwner | address | 101 | 0 | 20 | ForkManager |
| __gap | uint256[49] | 102 | 0 | 1568 | ForkManager |

71 changes: 71 additions & 0 deletions packages/protocol/contracts/layer1/fork/ForkManager.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";

/// @title ForkManager
/// @custom:security-contact [email protected]
/// @notice This contract serves as a base contract for managing up to two forks within the Taiko
/// protocol. By default, all function calls are routed to the newFork address.
/// Sub-contracts should override the shouldRouteToOldFork function to route specific function calls
/// to the old fork address.
/// These sub-contracts should be placed between a proxy and the actual fork implementations. When
/// calling upgradeTo, the proxy should always upgrade to a new ForkManager implementation, not an
/// actual fork implementation.
/// It is strongly advised to name functions differently for the same functionality across the two
/// forks, as it is not possible to route the same function to two different forks.
///
/// +--> newFork
/// PROXY -> FORK_MANAGER --|
/// +--> oldFork

contract ForkManager is UUPSUpgradeable, Ownable2StepUpgradeable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an init method here for proxy deployment?

Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

address public immutable oldFork;
address public immutable newFork;

error ForkAddressIsZero();
error InvalidParams();

constructor(address _oldFork, address _currFork) {
require(_currFork != address(0) && _currFork != _oldFork, InvalidParams());
oldFork = _oldFork;
newFork = _currFork;
}

fallback() external payable virtual {
_fallback();
}

receive() external payable virtual {
_fallback();
}

function isForkManager() public pure returns (bool) {
return true;
}

function _fallback() internal virtual {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not we restrict reentrancy ? 🤔 Not sure tho, cause propose/prove already protected.

address fork = shouldRouteToOldFork(msg.sig) ? oldFork : newFork;
require(fork != address(0), ForkAddressIsZero());

assembly {
calldatacopy(0, 0, calldatasize())
let result := delegatecall(gas(), fork, 0, calldatasize(), 0, 0)
returndatacopy(0, 0, returndatasize())

switch result
case 0 { revert(0, returndatasize()) }
default { return(0, returndatasize()) }
}
}

function _authorizeUpgrade(address) internal virtual override onlyOwner { }

/// @notice Determines if the call should be routed to the old fork.
/// @dev This function is intended to be overridden in derived contracts to provide custom
/// 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) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a function has the same selector in oldFork and newFork, how can we clarify?

}
1 change: 1 addition & 0 deletions packages/protocol/script/gen-layouts.sh
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ contracts_layer1=(
"contracts/layer1/team/tokenunlock/TokenUnlock.sol:TokenUnlock"
"contracts/layer1/provers/ProverSet.sol:ProverSet"
"contracts/layer1/provers/GuardianProver.sol:GuardianProver"
"contracts/layer1/fork/ForkManager.sol:ForkManager"
)

# Layer 2 contracts
Expand Down
69 changes: 69 additions & 0 deletions packages/protocol/test/layer1/fork/ForkManager.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import "../TaikoL1Test.sol";
import "src/layer1/fork/ForkManager.sol";

contract Fork is EssentialContract {
bytes32 private immutable __name;

constructor(bytes32 _name) {
__name = _name;
}

function init() external initializer {
__Essential_init(msg.sender);
}

function name() public view returns (bytes32) {
return __name;
}
}

contract ForkManager_RouteToOldFork is ForkManager {
constructor(address _fork1, address _fork2) ForkManager(_fork1, _fork2) { }

function shouldRouteToOldFork(bytes4 _selector) internal pure override returns (bool) {
return _selector == Fork.name.selector;
}
}

contract TestForkManager is TaikoL1Test {
address fork1 = address(new Fork("fork1"));
address fork2 = address(new Fork("fork2"));

function test_ForkManager_default_routing() public {
address proxy = deployProxy({
name: "main_proxy",
impl: address(new ForkManager(address(0), fork1)),
data: abi.encodeCall(Fork.init, ())
});

assertTrue(ForkManager(payable(proxy)).isForkManager());
assertEq(Fork(proxy).name(), "fork1");

// If we upgrade the proxy's impl to a fork, then alling isForkManager will throw,
// so we should never do this in production.
Fork(proxy).upgradeTo(fork2);
vm.expectRevert();
ForkManager(payable(proxy)).isForkManager();

Fork(proxy).upgradeTo(address(new ForkManager(fork1, fork2)));
assertEq(Fork(proxy).name(), "fork2");
}

function test_ForkManager_routing_to_old_fork() public {
address proxy = deployProxy({
name: "main_proxy",
impl: address(new ForkManager_RouteToOldFork(fork1, fork2)),
data: abi.encodeCall(Fork.init, ())
});

assertTrue(ForkManager(payable(proxy)).isForkManager());
assertEq(Fork(proxy).name(), "fork1");

Fork(proxy).upgradeTo(address(new ForkManager(fork1, fork2)));
assertTrue(ForkManager(payable(proxy)).isForkManager());
assertEq(Fork(proxy).name(), "fork2");
}
}