Yet another batch of improvements for Coordinator #107
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cygnusv
changed the title
cygnusv
requested review from
KPrasch,
piotr-roslaniec,
jMyles,
theref and
manumonti
| address authority; | ||
| uint16 dkgSize; | ||
| uint16 threshold; |
There was a problem hiding this comment.
This is going to help declutter API in nucypher-ts
piotr-roslaniec
approved these changes
Aug 24, 2023
…or the moment We currently define it as the lowest value that produces a threshold set that's strictly greater than the 50% of the overall size. See nucypher/nucypher#3095
We can revert to a composition model later, but for the moment this will make our lives easier
manumonti
approved these changes
Aug 25, 2023
| struct Ritual { | ||
| address initiator; | ||
| uint32 initTimestamp; | ||
| uint32 endTimestamp; | ||
| uint16 totalTranscripts; | ||
| uint16 totalAggregations; | ||
| // |
There was a problem hiding this comment.
It's not a important thing, but why this empty line is commented?
| bool aggregationMismatch; | ||
| // |
There was a problem hiding this comment.
It's not a important thing, but why this empty line is commented?
| function getRitualIdFromPublicKey( | ||
| BLS12381.G1Point memory dkgPublicKey | ||
| ) external view returns (uint32 ritualId) { | ||
| // If public key is not registered, result will produce underflow error |
There was a problem hiding this comment.
An underflow error sounds like a too cryptic message if you just used a wrong ritualId, doesn't it?
What about using a require here? Something like
Suggested change
| // If public key is not registered, result will produce underflow error | |
| require(getRitualState(ritualId) != RitualState.INVALID, "Ritual not found") |
KPrasch
approved these changes
Aug 27, 2023
derekpierre
reviewed
Aug 29, 2023
| @@ -338,21 +342,43 @@ contract Coordinator is AccessControlDefaultAdminRules { | |||
| keccak256(ritual.aggregatedTranscript) != aggregatedTranscriptDigest | |||
| ) { | |||
| ritual.aggregationMismatch = true; | |||
| delete ritual.publicKey; | |||
| bytes32 digest // signed message hash | ||
| uint32 ritualId, | ||
| bytes memory evidence, // supporting evidence for authorization | ||
| bytes memory ciphertextHeader // data to be signed by authorized |
There was a problem hiding this comment.
👏 .
@cygnusv Is the ciphertext header a fixed size? If so, any benefits that we can leverage from that fact like bytes32 or ...?
This was referenced Sep 18, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1 + dkg size/2. See Should DKG Threshold be specified for Ritual nucypher#3095FeeModelandCoordinatorGlobalAllowlist (Closes Optimze allow list storage lookups #97)IEncryptionAuthorizerAPI to requireciphertext_headerinstead ofdigest, to further strengthen verification.