Feature/collective key broadcast #17
Conversation
konradkonrad
force-pushed
the
feature/collective-key-broadcast
branch
2 times, most recently
from
b52e134 to
c60d843
Compare
This adds a new contract and contract role to the Keyper contracts: The contract `EonKeyPublish` is the entrypoint for keypers to announce a new Eon key. The contract tracks that the configured threshold of keypers agreed on the same eon key, and only then forwards it to the `KeyBroadcastContract`. The existing logic was changed, so that only a contract, that implements the interface `EonKeyPublisher`, is allowed to call the broadcasting contract. A publisher must be set for each `KeyperSet` before it gets finalized.
konradkonrad
force-pushed
the
feature/collective-key-broadcast
branch
from
c60d843 to
dff2b70
Compare
This incorporates most of the PR feedback from https://github.com/shutter-network/shop-contracts/pull/17/files - Renamed the `broadcaster` role to `publisher` - Get rid of `getKeyperIndex` method - Simplify counting logic in `EonKeyPublish.sol`
konradkonrad
force-pushed
the
feature/collective-key-broadcast
branch
from
73809bd to
f3e0c27
Compare
If an eonkey is already above the confirmation threshold, there should be no failure. We now instead emit an event that let's the caller know, that the eon key was already broadcast.
src/KeyBroadcastContract.sol
Outdated
| !KeyperSet(keyperSetManager.getKeyperSetAddress(eon)) | ||
| .isAllowedToBroadcastEonKey(msg.sender) | ||
| ) { | ||
| if (!EonKeyPublisher(keyperSet.getPublisher()).eonKeyConfirmed(key)) { |
There was a problem hiding this comment.
This change means the key broadcaster only works with the publisher contract, but for testing we'll still often want to use an EOA for instance. It also doesn't seem necessary since it's already checked in the publisher. So I think this should be removed.
There was a problem hiding this comment.
See https://github.com/shutter-network/shop-contracts/pull/17/files#diff-3d3a21aeab5a8ee50b2a9156a6d68881191ec03ebee386493b1299fa2646c92fR9-R13 for how I envisoned that use case. A bit cumbersome, but works for the testing use case. Removing this would allow a keyper to call broadcast directly, I believe.
There was a problem hiding this comment.
Removing this would allow a keyper to call broadcast directly, I believe.
there's still the !keyperSet.isAllowedToBroadcastEonKey(msg.sender) check for access control, no?
src/KeyBroadcastContract.sol
Outdated
| @@ -23,12 +27,16 @@ contract KeyBroadcastContract { | |||
| revert InvalidKey(); | |||
| } | |||
| if (keys[eon].length > 0) { | |||
| revert AlreadyHaveKey(); | |||
| emit AlreadyHaveKey(eon, key); | |||
There was a problem hiding this comment.
I think reverting is preferable here. One reason is that the caller won't know about the event, so they'll assume the key has been broadcast. This is misleading, especially if the new key would be different from the old one. Reverting gives the caller the option to either let the error propagate or to catch it.
So I think we should revert with AlreadyHaveKey here as before and handle the error in a try-catch block in EonKeyPublish.publishEonKey.
There was a problem hiding this comment.
TIL about solidity try-catch
This adds a new contract
and contract roleto the Keyper contracts:The contract
EonKeyPublishis the entrypoint for keypers to announce anew Eon key. The contract tracks that the configured threshold of
keypers agreed on the same eon key, and only then forwards it to the
KeyBroadcastContract. The existing logic was changed, so that only acontract, that implements the interface
EonKeyPublisher, is allowed tocall the broadcasting contract. A publisher must be set for each
KeyperSetbefore it gets finalized.The role of the
broadcasterwas changed topublisher, to reflect that callingEonKeyPublisheris only part of a (threshold controlled) collective action that will only in the successful case (threshold achieved) result in a "broadcast".After the latest rework, this is still missing a few things:
tests for the confirmation logictest for the newly introduced eventEonVoteRegisteredchange how we deal with eon votes after the threshold was reached and thebroadcastevent was emitted (should not fail)