Closed guidanoli closed 1 year ago
Here, I'm using three really handy OpenZeppelin contracts:
AccessControlEnumerable
EnumerableSet
PaymentSplitter
(suggested by @pedroargento)I think the tricky part is that there is no coordination between the multiple nodes. They will submit claims at the exact same time. What would happen in that case? Imagine the "same" transaction, coming from multiple nodes, in the same block. Would it work?
Rebased and applied @pedroargento suggestion to use PaymentSplitter
.
Applied @pedroargento's suggestion of using ==
instead of >
for checking majority, and avoid submitting the same claim to the history twice.
Doesn't it make sense to make the number of necessary votes a parameter? Maybe one DApp wants to have a super majority or wants to accept only 2 signatures even with a lot of possible validators.
Doesn't it make sense to make the number of necessary votes a parameter? Maybe one DApp wants to have a super majority or wants to accept only 2 signatures even with a lot of possible validators.
If we see a demand for such fine-grained configuration, sure. For now, a simple majority scheme should suffice for an MVP.
I think the tricky part is that there is no coordination between the multiple nodes. They will submit claims at the exact same time. What would happen in that case? Imagine the "same" transaction, coming from multiple nodes, in the same block. Would it work?
Great point. What will happen now that I've changed >
to ==
, is that it will only submit a claim once. Of course, it needs testing, but it should work (in my head).
By using PaymentSplitter
, the set of validators and shares are fixed at the constructor and cannot be changed later on. Do we wanna assume the quorum to be fixed?
By using
PaymentSplitter
, the set of validators and shares are fixed at the constructor and cannot be changed later on. Do we wanna assume the quorum to be fixed?
Yes, the PaymentSplitter
contract assumes the set of shares is constant.
I think we can optimize the codes a bit:
AccessControlEnumerable
is a combination of AccessControl
and EnumerableSet
. Since we don't have a dynamic quorum, we don't actually need to maintain a EnumerableSet
. With the current codes, the only time we call this EnumerableSet
is to check the size of the quorum, which can simply be done with a uint256
. As for access control, we can simplify to a mapping(address => bool) members
to indicate whether it's a member or not. The complete set of members is already shown in the constructor parameter, and we could emit an event if needed.EnumerableSet
is to record the set of members that voted yeas. Similarly, we don't actually need to maintain the set, we only need to record who has already voted yea. So instead of using this struct from EnumerableSet
struct Set {
bytes32[] _values;
mapping(bytes32 => uint256) _indexes;
}
, we can simplify it to
struct Yeas {
uint256 count;
mapping(address => bool) voted;
}
Furthermore, if we want to optimize further. We can use a uint256
and split it into 2 parts. | who_has_voted(248 bit) | count(8 bit) |
This way we can record up to 248 members, including who has voted and the current count of how many has voted. If there could be more than 248 members, then we could make it an array(mapping) of uint256
s. P.S., in part 1, we mentioned using mapping(address => bool) members
to indicate whether it's a member. Now it becomes mapping(address => uint256) members
to indicate a member's bit position (>0) in the uint256
struct, with 0 indicating not a member.
The drawback is the code becomes more complex for sure. Let me know what you think and we can work on this together :)
@xdaniortega Thanks for the feedback! And let us bring the discussion to the original issue, so that everyone can see and participate. :-)
This PR has been migrated to https://github.com/cartesi/rollups-contracts/pull/49.
Closes cartesi/rollups-contracts#48