optimize quorum #222
optimize quorum #222
Conversation
| mapping(bytes => EnumerableSet.AddressSet) internal yeas; | ||
| // Quorum members | ||
| // Map an address to true if it's a validator | ||
| mapping(address => bool) public validators; |
There was a problem hiding this comment.
This is very similar to how AccessControl is implemented. Maybe we should use it instead?
There was a problem hiding this comment.
Yes. The main idea is to reduce AccessControlEnumerable to only something like AccessControl. But the reason why I didn't use AccessControl is because AccessControl is most useful when there are different roles, while we only have one role, validator. So it doesn't make sense to me to do 1 extra storage read (through this variable) each time when we want to read the validators.
There was a problem hiding this comment.
This doesn't imply an extra storage read.
It just adds a new keccak in the calculation of the storage slot where the value resides.
Either way, I think it is still worth it to use AccessControl because it has been tested thoroughly.
There was a problem hiding this comment.
You are right there's no extra storage read. Sorry about my mistake.
But you see, all we need is a mapping(address => bool), set some addresses map to true in the constructor, plus we used it for the modifier. This is simple enough that it doesn't make much of a difference using existing codes or not, since it's only 2 lines of codes plus a modifier.
Another advantage is that it can be more flexible if we want to optimize further. If the current version is ok, I'm creating another PR based on the current version for the last step of optimization. We can decide on this after the other PR, ok? If the other PR is not gonna be merged, then we can use AccessControl if you think it's better.
There was a problem hiding this comment.
But you see, all we need is a
mapping(address => bool), set some addresses map to true in the constructor, plus we used it for the modifier.
You're right, but this logic has already been implemented. There is no need to re-implement it. Besides, for anyone reading this code, they will need to understand what you're trying to achieve with this mapping, the modifier, etc. But if you use AccessControl (which many smart contract developers are already familiar with), the code will be much clearer, and, most importantly, more trustworthy.
There was a problem hiding this comment.
Another advantage is that it can be more flexible if we want to optimize further.
That is true. But we need to find the balance between performance and simplicity.
| /// @dev This contract uses several OpenZeppelin contracts: | ||
| /// `AccessControlEnumerable`, `EnumerableSet`, and `PaymentSplitter`. | ||
| /// For more information on those, please consult OpenZeppelin's official documentation. |
There was a problem hiding this comment.
I think it's good to mention that we're using OpenZeppelin's contracts, because they are trustworthy.
| /// @dev Only validators can submit claims. | ||
| bytes32 public constant VALIDATOR_ROLE = keccak256("VALIDATOR_ROLE"); | ||
|
|
||
| /// In this version, we assume the quorum doesn't change after the constructor. |
There was a problem hiding this comment.
It's more than just an assumption, it's a system limitation.
We could say, instead:
In this version, the validator set is immutable.
|
|
||
| /// @notice The validator role. | ||
| /// @dev Only validators can submit claims. | ||
| bytes32 public constant VALIDATOR_ROLE = keccak256("VALIDATOR_ROLE"); |
There was a problem hiding this comment.
Should we use AccessControl, this variable would still be in use.
|
Just in the documentation, right? Because the function has to be
submitClaim to implement IConsensus.
Em seg., 21 de ago. de 2023 19:11, Guilherme Dantas <
***@***.***> escreveu:
… ***@***.**** commented on this pull request.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> import {PaymentSplitter} from ***@***.***/contracts/finance/PaymentSplitter.sol";
-
This empty line is meant to separate external contracts (from
OpenZeppelin) from internal ones.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> /// @notice The history contract.
/// @dev See the `getHistory` function.
IHistory internal immutable history;
- /// @notice For each claim, the set of validators that agree
- /// that it should be submitted to the history contract.
- mapping(bytes => EnumerableSet.AddressSet) internal yeas;
+ // Quorum members
+ // Map an address to true if it's a validator
+ mapping(address => bool) public validators;
This is very much how AccessControl
<https://docs.openzeppelin.com/contracts/4.x/api/access#AccessControl> is
implemented. Maybe we should use it instead?
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> -/// @dev This contract uses several OpenZeppelin contracts:
-/// `AccessControlEnumerable`, `EnumerableSet`, and `PaymentSplitter`.
-/// For more information on those, please consult OpenZeppelin's official documentation.
I think it's good to mention that we're using OpenZeppelin's contracts,
because they are trustworthy.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> import {IHistory} from "../../history/IHistory.sol";
/// @title Quorum consensus
/// @notice A consensus model controlled by a small set of addresses, the validators.
-/// @dev This contract uses several OpenZeppelin contracts:
-/// `AccessControlEnumerable`, `EnumerableSet`, and `PaymentSplitter`.
-/// For more information on those, please consult OpenZeppelin's official documentation.
-contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
- using EnumerableSet for EnumerableSet.AddressSet;
-
- /// @notice The validator role.
- /// @dev Only validators can submit claims.
- bytes32 public constant VALIDATOR_ROLE = keccak256("VALIDATOR_ROLE");
-
+/// In this version, we assume the quorum doesn't change after the constructor.
It's more than just an assumption, it's a system limitation.
We could say, instead:
In this version, the validator set is immutable.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> import {IHistory} from "../../history/IHistory.sol";
/// @title Quorum consensus
/// @notice A consensus model controlled by a small set of addresses, the validators.
-/// @dev This contract uses several OpenZeppelin contracts:
-/// `AccessControlEnumerable`, `EnumerableSet`, and `PaymentSplitter`.
-/// For more information on those, please consult OpenZeppelin's official documentation.
-contract Quorum is AbstractConsensus, AccessControlEnumerable, PaymentSplitter {
- using EnumerableSet for EnumerableSet.AddressSet;
-
- /// @notice The validator role.
- /// @dev Only validators can submit claims.
- bytes32 public constant VALIDATOR_ROLE = keccak256("VALIDATOR_ROLE");
Should we use AccessControl
<https://docs.openzeppelin.com/contracts/4.x/api/access#AccessControl>,
this variable would still be in use.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> for (uint256 i; i < _validators.length; ++i) {
- grantRole(VALIDATOR_ROLE, _validators[i]);
+ validators[_validators[i]] = true;
This doesn't check for duplicates. AccessControl does.
------------------------------
In onchain/rollups/contracts/consensus/quorum/Quorum.sol
<#222 (comment)>:
> history = _history;
}
- /// @notice Submits a claim for voting.
+ /// @notice Submits (Votes) a claim.
Maybe we could drop the "Submit", and instead keep "Vote *for* a claim to
be submitted".
—
Reply to this email directly, view it on GitHub
<#222 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF7D7SY4W7QHUV2TK46CIDXWOJCRANCNFSM6AAAAAA3YSBGYE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Yes, I am referring to the documentation only. :-) |
|
I think, by default, we should seek good programming practices such as code reusability, and then, with proof of considerable improvements in gas costs, optimize the code for performance. |
|
This has been migrated to cartesi/rollups-contracts#56. |
No description provided.