search

cartesi / rollups

Cartesi Rollups
30 stars 12 forks source link

optimize quorum #222

Closed ZzzzHui closed 1 year ago

pedroargento commented 1 year ago

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 https://github.com/cartesi/rollups/pull/222#discussion_r1300372898:

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 https://github.com/cartesi/rollups/pull/222#discussion_r1300375358:

 /// @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 https://github.com/cartesi/rollups/pull/222#discussion_r1300380976:

-/// @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 https://github.com/cartesi/rollups/pull/222#discussion_r1300383273:

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 https://github.com/cartesi/rollups/pull/222#discussion_r1300384878:

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 https://github.com/cartesi/rollups/pull/222#discussion_r1300411551:

     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 https://github.com/cartesi/rollups/pull/222#discussion_r1300414691:

     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 https://github.com/cartesi/rollups/pull/222#pullrequestreview-1587437428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF7D7SY4W7QHUV2TK46CIDXWOJCRANCNFSM6AAAAAA3YSBGYE . You are receiving this because your review was requested.Message ID: @.***>

guidanoli commented 1 year ago

Maybe we could drop the "Submit", and instead keep "Vote for a claim to be submitted".

Just in the documentation, right? Because the function has to be submitClaim to implement IConsensus.

Yes, I am referring to the documentation only. :-)

guidanoli commented 1 year ago

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.

guidanoli commented 1 year ago

This has been migrated to https://github.com/cartesi/rollups-contracts/pull/56.