-
Notifications
You must be signed in to change notification settings - Fork 143
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
feat: Network and NodeMetadata Protobuf #16581
Conversation
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #16581 +/- ##
==========================================
Coverage ? 63.39%
Complexity ? 20193
==========================================
Files ? 2538
Lines ? 94260
Branches ? 9873
==========================================
Hits ? 59753
Misses ? 30905
Partials ? 3602
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. 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.
It isn't entirely clear (even after reading the linked issue and further linked Notion page) what these messages are for.
General suggestions:
- Detail in the PR description all of "what", "why", and "how" of the PR.
- What is being done here, design/feature wise
- Why is this needed, and why is it designed in this way
- How is this to be used and how does it meet future needs
- Make sure that
proto
file specification always explains all of the relevant requirements for each message and field. - In a proto file comment, add any relevant additional detail, possibly including the "what/why/how" from above, as applied to that particular file.
- Writing too much description is, in my experience, almost never a problem. The common problem is PRs, specifications, design documentation (i.e. code files), and related text that is woefully short and imprecise.
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
@jsync-swirlds. Agreed. I've updated the description of the PR to include more detailed description of the motivation and design and give some more background on the what/why/how. Hopefully that should provide a little more clarity now |
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
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.
Well executed and thorough.
Thanks.
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
de26b5c
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.
Review applies to:
hapi/build.gradle.kts
hedera-node/hedera-app-spi/build.gradle.kts
Description:
config.txt
file, fully encapsulating the information required to bootstrap a network with Threshold Signature Scheme (TSS) enabled and with the legacy address book completely replaced by RosterDesign and Motivations:
TssEncryptionKeys
, Roster entries, andledgerIds
.NodeMetadata
, which includes fields such as theTssEncryptionKey
(now removed from the Roster) and other information previously found in the Address Book.tssEncryptionKey
is required here because it has been removed from the Roster and will need to be create here with Tss Key MaterialLedgerId
is required, because it needs to be validated against when you start the new networkRelated issue(s):
Fixes #16549