VERY DRAFT Multisig verifier #1643
Conversation
| ) external override { | ||
| require(signingThreshold[_chainAddress] == 0, "Chain requires verifiers signatures for commit"); | ||
| _recordBatchCommitment(_chainAddress, _processBatchFrom, _processBatchTo); | ||
| _propagateToZKChain(_chainAddress); |
There was a problem hiding this comment.
nit: it would be nice if you used inheritance here i.e. invoked super.
| uint256 _processBatchFrom, | ||
| uint256 _processBatchTo, | ||
| bytes calldata _batchData | ||
| ) external override { |
There was a problem hiding this comment.
no access control
|
|
||
| _recordBatchCommitment(chainAddress, _processBatchFrom, _processBatchTo); | ||
| // we cannot use _propagateToZKChain here, becouse function signature is altered | ||
| IExecutor(chainAddress).commitBatchesSharedBridge(chainAddress, _processBatchFrom, _processBatchTo, _batchData); |
There was a problem hiding this comment.
it is a bit unfortunate that we do a full ABI-encoding here. It will come at a cost, but hopefully it wont be too much:
- for rollups we'd use blobs and their commitments are small
- for validiums/alt-DA we'd use some small commitments too
|
|
||
|
|
||
| //TODO proper errors | ||
| //TODO maybe combine with ValidatorTimelock |
There was a problem hiding this comment.
This is a good point to discuss. In my opinion, unless there's a reason to have two separate contracts (i.e. MultisigCommiter and ValidatorTimelock) I'd do one. Having different ValidatorTimelocks on different CTMs will make things more complex (e.g. explaining people why we have two contracts; but also keeping track which CTM has which contract).
There was a problem hiding this comment.
Also if a CTM owner wants to use the 2FA feature at some point it's easy to turn on in case the feature is already there (vs upgrading the implementation).
|
|
||
| //TODO proper errors | ||
| //TODO maybe combine with ValidatorTimelock | ||
| contract MultisigCommiter is ValidatorTimelock, EIP712Upgradeable { |
There was a problem hiding this comment.
We need a new name for this contract (I mean ValidatorTimelock) since it's not just a timelock after we introduce this change
There was a problem hiding this comment.
Maybe ValidatorRegistry
What ❔
Why ❔
Checklist