-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade lynx Coordinator #312
Conversation
7488cc6
to
c3ec58f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎸
cfd7a7b
to
20b6456
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only strong comment is regarding the JSON registry format, but apart from it looks good to me! The rest it's just nitpicking.
address initiator, | ||
uint32 initTimestamp, | ||
uint32 endTimestamp, | ||
uint16 totalTranscripts, | ||
uint16 totalAggregations, | ||
// | ||
address authority, | ||
uint16 dkgSize, | ||
uint16 threshold, | ||
bool aggregationMismatch, | ||
// | ||
IEncryptionAuthorizer accessController, | ||
BLS12381.G1Point memory publicKey, | ||
bytes memory aggregatedTranscript, | ||
IFeeModel feeModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about we create a dummy struct for this? Something like RitualView
or along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt it will look any better, but if you want I can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't think it's worth it, then it's fine, let's keep it this way
deployment/artifacts/lynx.json
Outdated
@@ -1,7057 +1,7064 @@ | |||
{ | |||
"11155111": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be an issue here with the number of spaces for tabs, which is producing a huge diff. I'd prefer if the same indentation convention is preserved, so change history is better understood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was the pre-commit
linter kicking in.
Initially, our registries used their original format without any pre-commit linter, and then once the pre-commit linter job kicked in, we didn't want to have a massive diff, and so we continued to ignore the linter update.
Some options:
- Modify the pre-commit linter to ignore these files
- Update the file at some point (now or later) based on the linter, and continue to let the linter ensure the format remains the same. If we update to the linter's format, we should probably also modify this line - https://github.com/nucypher/nucypher-contracts/blob/main/deployment/registry.py#L20 - to use an indent of 2.
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, if we go with keeping the original format, @vzotova you can use the normalize_registry
script to revert the file back to what it was before the linter kicked in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #319
20b6456
to
ea207b5
Compare
ea207b5
to
e751891
Compare
Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Co-authored-by: David Núñez <david@nucypher.com>
e751891
to
6b8420a
Compare
@cygnusv I believe your original concerns have been addressed.
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: