From dc313fff10e850cbda475f4e1edd4022988982cb Mon Sep 17 00:00:00 2001 From: Daniel Wang <99078276+dantaik@users.noreply.github.com> Date: Tue, 21 May 2024 22:28:13 +0800 Subject: [PATCH] refactor(protocol): merge Guardians.sol into GuardianProver.sol (#17285) Co-authored-by: dantaik --- packages/protocol/contract_layout.md | 1 - .../contracts/L1/provers/GuardianProver.sol | 141 ++++++++++++++++-- .../contracts/L1/provers/Guardians.sol | 137 ----------------- ...{Guardians.t.sol => GuardianProver1.t.sol} | 30 ++-- .../GuardianProver2.t.sol} | 6 +- 5 files changed, 150 insertions(+), 165 deletions(-) delete mode 100644 packages/protocol/contracts/L1/provers/Guardians.sol rename packages/protocol/test/L1/{Guardians.t.sol => GuardianProver1.t.sol} (68%) rename packages/protocol/test/{verifiers/GuardianProver.t.sol => L1/GuardianProver2.t.sol} (92%) diff --git a/packages/protocol/contract_layout.md b/packages/protocol/contract_layout.md index e4b67e53df6..68e89ea7b04 100644 --- a/packages/protocol/contract_layout.md +++ b/packages/protocol/contract_layout.md @@ -129,7 +129,6 @@ | version | uint32 | 254 | 0 | 4 | contracts/L1/provers/GuardianProver.sol:GuardianProver | | minGuardians | uint32 | 254 | 4 | 4 | contracts/L1/provers/GuardianProver.sol:GuardianProver | | __gap | uint256[46] | 255 | 0 | 1472 | contracts/L1/provers/GuardianProver.sol:GuardianProver | -| __gap | uint256[50] | 301 | 0 | 1600 | contracts/L1/provers/GuardianProver.sol:GuardianProver | ## TaikoToken | Name | Type | Slot | Offset | Bytes | Contract | diff --git a/packages/protocol/contracts/L1/provers/GuardianProver.sol b/packages/protocol/contracts/L1/provers/GuardianProver.sol index 74f351cb690..5fc797ae370 100644 --- a/packages/protocol/contracts/L1/provers/GuardianProver.sol +++ b/packages/protocol/contracts/L1/provers/GuardianProver.sol @@ -3,23 +3,37 @@ pragma solidity 0.8.24; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; - +import "../../common/EssentialContract.sol"; import "../../common/LibStrings.sol"; import "../../verifiers/IVerifier.sol"; -import "../tiers/ITierProvider.sol"; import "../ITaikoL1.sol"; -import "./Guardians.sol"; /// @title GuardianProver /// This prover uses itself as the verifier. /// @custom:security-contact security@taiko.xyz -contract GuardianProver is IVerifier, Guardians { +contract GuardianProver is IVerifier, EssentialContract { using SafeERC20 for IERC20; - error GV_PERMISSION_DENIED(); - error GV_ZERO_ADDRESS(); + /// @notice Contains the index of the guardian in `guardians` plus one (zero means not a + /// guardian) + /// @dev Slot 1 + mapping(address guardian => uint256 id) public guardianIds; + + /// @notice Mapping to store the approvals for a given hash, for a given version + mapping(uint32 version => mapping(bytes32 hash => uint256 approvalBits)) internal _approvals; + + /// @notice The set of guardians + /// @dev Slot 3 + address[] public guardians; + + /// @notice The version of the guardians + /// @dev Slot 4 + uint32 public version; - uint256[50] private __gap; + /// @notice The minimum number of guardians required to approve + uint32 public minGuardians; + + uint256[46] private __gap; /// @notice Emitted when a guardian proof is approved. /// @param addr The address of the guardian. @@ -35,6 +49,23 @@ contract GuardianProver is IVerifier, Guardians { bytes proofData ); + /// @notice Emitted when the set of guardians is updated + /// @param version The new version + /// @param guardians The new set of guardians + event GuardiansUpdated(uint32 version, address[] guardians); + + /// @notice Emitted when an approval is made + /// @param operationId The operation ID + /// @param approvalBits The new approval bits + /// @param minGuardiansReached If the proof was submitted + event Approved(uint256 indexed operationId, uint256 approvalBits, bool minGuardiansReached); + + error GP_INVALID_GUARDIAN(); + error GP_INVALID_GUARDIAN_SET(); + error GP_INVALID_MIN_GUARDIANS(); + error GV_PERMISSION_DENIED(); + error GV_ZERO_ADDRESS(); + /// @notice Initializes the contract. /// @param _owner The owner of this contract. msg.sender will be used if this value is zero. /// @param _addressManager The address of the {AddressManager} contract. @@ -42,6 +73,52 @@ contract GuardianProver is IVerifier, Guardians { __Essential_init(_owner, _addressManager); } + /// @notice Set the set of guardians + /// @param _newGuardians The new set of guardians + /// @param _minGuardians The minimum required to sign + function setGuardians( + address[] memory _newGuardians, + uint8 _minGuardians + ) + external + onlyOwner + nonReentrant + { + // We need at most 255 guardians (so the approval bits fit in a uint256) + if (_newGuardians.length == 0 || _newGuardians.length > type(uint8).max) { + revert GP_INVALID_GUARDIAN_SET(); + } + // Minimum number of guardians to approve is at least equal or greater than half the + // guardians (rounded up) and less or equal than the total number of guardians + if (_minGuardians == 0 || _minGuardians > _newGuardians.length) { + revert GP_INVALID_MIN_GUARDIANS(); + } + + // Delete the current guardians + for (uint256 i; i < guardians.length; ++i) { + delete guardianIds[guardians[i]]; + } + delete guardians; + + // Set the new guardians + for (uint256 i; i < _newGuardians.length; ++i) { + address guardian = _newGuardians[i]; + if (guardian == address(0)) revert GP_INVALID_GUARDIAN(); + // This makes sure there are not duplicate addresses + if (guardianIds[guardian] != 0) revert GP_INVALID_GUARDIAN_SET(); + + // Save and index the guardian + guardians.push(guardian); + guardianIds[guardian] = guardians.length; + } + + // Bump the version so previous approvals get invalidated + ++version; + + minGuardians = _minGuardians; + emit GuardiansUpdated(version, _newGuardians); + } + /// @notice Enables unlimited allowance for Taiko L1 contract. /// param _enable true if unlimited allowance is approved, false to set the allowance to 0. function enableTaikoTokenAllowance(bool _enable) external onlyOwner { @@ -77,12 +154,12 @@ contract GuardianProver is IVerifier, Guardians { returns (bool approved_) { bytes32 hash = keccak256(abi.encode(_meta, _tran, _proof.data)); - approved_ = approve(_meta.id, hash); + approved_ = _approve(_meta.id, hash); emit GuardianApproval(msg.sender, _meta.id, _tran.blockHash, approved_, _proof.data); if (approved_) { - deleteApproval(hash); + _deleteApproval(hash); ITaikoL1(resolve(LibStrings.B_TAIKO, false)).proveBlock( _meta.id, abi.encode(_meta, _tran, _proof) ); @@ -100,4 +177,50 @@ contract GuardianProver is IVerifier, Guardians { { if (_ctx.msgSender != address(this)) revert GV_PERMISSION_DENIED(); } + + /// @notice Returns if the hash is approved + /// @param _hash The hash to check + /// @return true if the hash is approved + function isApproved(bytes32 _hash) public view returns (bool) { + return _isApproved(_approvals[version][_hash]); + } + + /// @notice Returns the number of guardians + /// @return The number of guardians + function numGuardians() public view returns (uint256) { + return guardians.length; + } + + function _approve(uint256 _blockId, bytes32 _hash) internal returns (bool approved_) { + uint256 id = guardianIds[msg.sender]; + if (id == 0) revert GP_INVALID_GUARDIAN(); + + uint32 _version = version; + + unchecked { + _approvals[_version][_hash] |= 1 << (id - 1); + } + + uint256 _approval = _approvals[_version][_hash]; + approved_ = _isApproved(_approval); + emit Approved(_blockId, _approval, approved_); + } + + function _deleteApproval(bytes32 _hash) private { + delete _approvals[version][_hash]; + } + + function _isApproved(uint256 _approvalBits) private view returns (bool) { + uint256 count; + uint256 bits = _approvalBits; + uint256 guardiansLength = guardians.length; + unchecked { + for (uint256 i; i < guardiansLength; ++i) { + if (bits & 1 == 1) ++count; + if (count == minGuardians) return true; + bits >>= 1; + } + } + return false; + } } diff --git a/packages/protocol/contracts/L1/provers/Guardians.sol b/packages/protocol/contracts/L1/provers/Guardians.sol deleted file mode 100644 index 2fa886abbb1..00000000000 --- a/packages/protocol/contracts/L1/provers/Guardians.sol +++ /dev/null @@ -1,137 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.24; - -import "../../common/EssentialContract.sol"; - -/// @title Guardians -/// @notice A contract that manages a set of guardians and their approvals. -/// @custom:security-contact security@taiko.xyz -abstract contract Guardians is EssentialContract { - /// @notice Contains the index of the guardian in `guardians` plus one (zero means not a - /// guardian) - /// @dev Slot 1 - mapping(address guardian => uint256 id) public guardianIds; - - /// @notice Mapping to store the approvals for a given hash, for a given version - mapping(uint32 version => mapping(bytes32 hash => uint256 approvalBits)) internal _approvals; - - /// @notice The set of guardians - /// @dev Slot 3 - address[] public guardians; - - /// @notice The version of the guardians - /// @dev Slot 4 - uint32 public version; - - /// @notice The minimum number of guardians required to approve - uint32 public minGuardians; - - uint256[46] private __gap; - - /// @notice Emitted when the set of guardians is updated - /// @param version The new version - /// @param guardians The new set of guardians - event GuardiansUpdated(uint32 version, address[] guardians); - - /// @notice Emitted when an approval is made - /// @param operationId The operation ID - /// @param approvalBits The new approval bits - /// @param minGuardiansReached If the proof was submitted - event Approved(uint256 indexed operationId, uint256 approvalBits, bool minGuardiansReached); - - error INVALID_GUARDIAN(); - error INVALID_GUARDIAN_SET(); - error INVALID_MIN_GUARDIANS(); - - /// @notice Set the set of guardians - /// @param _newGuardians The new set of guardians - /// @param _minGuardians The minimum required to sign - function setGuardians( - address[] memory _newGuardians, - uint8 _minGuardians - ) - external - onlyOwner - nonReentrant - { - // We need at most 255 guardians (so the approval bits fit in a uint256) - if (_newGuardians.length == 0 || _newGuardians.length > type(uint8).max) { - revert INVALID_GUARDIAN_SET(); - } - // Minimum number of guardians to approve is at least equal or greater than half the - // guardians (rounded up) and less or equal than the total number of guardians - if (_minGuardians == 0 || _minGuardians > _newGuardians.length) { - revert INVALID_MIN_GUARDIANS(); - } - - // Delete the current guardians - for (uint256 i; i < guardians.length; ++i) { - delete guardianIds[guardians[i]]; - } - delete guardians; - - // Set the new guardians - for (uint256 i; i < _newGuardians.length; ++i) { - address guardian = _newGuardians[i]; - if (guardian == address(0)) revert INVALID_GUARDIAN(); - // This makes sure there are not duplicate addresses - if (guardianIds[guardian] != 0) revert INVALID_GUARDIAN_SET(); - - // Save and index the guardian - guardians.push(guardian); - guardianIds[guardian] = guardians.length; - } - - // Bump the version so previous approvals get invalidated - ++version; - - minGuardians = _minGuardians; - emit GuardiansUpdated(version, _newGuardians); - } - - /// @notice Returns if the hash is approved - /// @param _hash The hash to check - /// @return true if the hash is approved - function isApproved(bytes32 _hash) public view returns (bool) { - return isApproved(_approvals[version][_hash]); - } - - /// @notice Returns the number of guardians - /// @return The number of guardians - function numGuardians() public view returns (uint256) { - return guardians.length; - } - - function approve(uint256 _blockId, bytes32 _hash) internal returns (bool approved_) { - uint256 id = guardianIds[msg.sender]; - if (id == 0) revert INVALID_GUARDIAN(); - - uint32 _version = version; - - unchecked { - _approvals[_version][_hash] |= 1 << (id - 1); - } - - uint256 _approval = _approvals[_version][_hash]; - approved_ = isApproved(_approval); - emit Approved(_blockId, _approval, approved_); - } - - function deleteApproval(bytes32 _hash) internal { - delete _approvals[version][_hash]; - } - - function isApproved(uint256 _approvalBits) internal view returns (bool) { - uint256 count; - uint256 bits = _approvalBits; - uint256 guardiansLength = guardians.length; - unchecked { - for (uint256 i; i < guardiansLength; ++i) { - if (bits & 1 == 1) ++count; - if (count == minGuardians) return true; - bits >>= 1; - } - } - return false; - } -} diff --git a/packages/protocol/test/L1/Guardians.t.sol b/packages/protocol/test/L1/GuardianProver1.t.sol similarity index 68% rename from packages/protocol/test/L1/Guardians.t.sol rename to packages/protocol/test/L1/GuardianProver1.t.sol index 8f7021c5d9a..e21f8c606b0 100644 --- a/packages/protocol/test/L1/Guardians.t.sol +++ b/packages/protocol/test/L1/GuardianProver1.t.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.24; import "../TaikoTest.sol"; -contract DummyGuardians is Guardians { +contract DummyGuardianProver is GuardianProver { uint256 public operationId; function init() external initializer { @@ -11,12 +11,12 @@ contract DummyGuardians is Guardians { } function approve(bytes32 hash) public returns (bool) { - return super.approve(operationId++, hash); + return _approve(operationId++, hash); } } -contract TestGuardianProver is TaikoTest { - DummyGuardians target; +contract TestGuardianProver1 is TaikoTest { + DummyGuardianProver target; function getSigners(uint256 numGuardians) internal returns (address[] memory signers) { signers = new address[](numGuardians); @@ -27,38 +27,38 @@ contract TestGuardianProver is TaikoTest { } function setUp() public { - target = DummyGuardians( + target = DummyGuardianProver( deployProxy({ name: "guardians", - impl: address(new DummyGuardians()), - data: abi.encodeCall(DummyGuardians.init, ()) + impl: address(new DummyGuardianProver()), + data: abi.encodeCall(DummyGuardianProver.init, ()) }) ); } - function test_guardians_set_guardians() public { - vm.expectRevert(Guardians.INVALID_GUARDIAN_SET.selector); + function test_guardian_prover_set_guardians() public { + vm.expectRevert(GuardianProver.GP_INVALID_GUARDIAN_SET.selector); target.setGuardians(getSigners(0), 0); - vm.expectRevert(Guardians.INVALID_MIN_GUARDIANS.selector); + vm.expectRevert(GuardianProver.GP_INVALID_MIN_GUARDIANS.selector); target.setGuardians(getSigners(5), 0); - vm.expectRevert(Guardians.INVALID_MIN_GUARDIANS.selector); + vm.expectRevert(GuardianProver.GP_INVALID_MIN_GUARDIANS.selector); target.setGuardians(getSigners(5), 6); } - function test_guardians_set_guardians2() public { + function test_guardian_prover_set_guardians2() public { address[] memory signers = getSigners(5); signers[0] = address(0); - vm.expectRevert(Guardians.INVALID_GUARDIAN.selector); + vm.expectRevert(GuardianProver.GP_INVALID_GUARDIAN.selector); target.setGuardians(signers, 4); signers[0] = signers[1]; - vm.expectRevert(Guardians.INVALID_GUARDIAN_SET.selector); + vm.expectRevert(GuardianProver.GP_INVALID_GUARDIAN_SET.selector); target.setGuardians(signers, 4); } - function test_guardians_approve() public { + function test_guardian_prover_approve() public { address[] memory signers = getSigners(6); target.setGuardians(signers, 4); diff --git a/packages/protocol/test/verifiers/GuardianProver.t.sol b/packages/protocol/test/L1/GuardianProver2.t.sol similarity index 92% rename from packages/protocol/test/verifiers/GuardianProver.t.sol rename to packages/protocol/test/L1/GuardianProver2.t.sol index 442033c9654..3aede68a4e9 100644 --- a/packages/protocol/test/verifiers/GuardianProver.t.sol +++ b/packages/protocol/test/L1/GuardianProver2.t.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.24; import "../L1/TaikoL1TestBase.sol"; /// @author Kirk Baird -contract TestGuardianProver is TaikoL1TestBase { +contract TestGuardianProver2 is TaikoL1TestBase { function deployTaikoL1() internal override returns (TaikoL1) { return TaikoL1(payable(deployProxy({ name: "taiko", impl: address(new TaikoL1()), data: "" }))); @@ -16,7 +16,7 @@ contract TestGuardianProver is TaikoL1TestBase { } // Tests `verifyProof()` with the correct prover - function test_verifyProof() public view { + function test_guardian_prover_verifyProof() public view { // Context IVerifier.Context memory ctx = IVerifier.Context({ metaHash: bytes32(0), @@ -45,7 +45,7 @@ contract TestGuardianProver is TaikoL1TestBase { } // Tests `verifyProof()` with the wrong prover - function test_verifyProof_invalidProver() public { + function test_guardian_prover_verifyProof_invalidProver() public { // Context IVerifier.Context memory ctx = IVerifier.Context({ metaHash: bytes32(0),