-
Notifications
You must be signed in to change notification settings - Fork 7
feat: validator node version attestation with observer demotion #86
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
base: develop
Are you sure you want to change the base?
Changes from all commits
6a69084
1efd025
568fb87
7bdd7af
85f95ca
b5a3212
088195c
f1d5148
3c0eb2d
16cba7d
7c71910
a555277
363af86
671f10a
c71914d
4515be9
bf65553
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,9 @@ message ValidatorData { | |
| string Name = 24 [json_name = "name"]; | ||
| string Logo = 25 [json_name = "logo"]; | ||
| map<string, string> URIs = 26 [json_name = "uris"]; | ||
|
|
||
| string AttestedVersion = 27 [json_name = "attestedVersion"]; | ||
| uint32 AttestedEpoch = 28 [json_name = "attestedEpoch"]; | ||
|
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Fix the new protobuf field naming before merge. The added fields use PascalCase source names and fail Buf's
🧰 Tools🪛 Buf (1.71.0)[error] 29-29: Field name "AttestedVersion" should be lower_snake_case, such as "attested_version". (FIELD_LOWER_SNAKE_CASE) [error] 30-30: Field name "AttestedEpoch" should be lower_snake_case, such as "attested_epoch". (FIELD_LOWER_SNAKE_CASE) 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| } | ||
|
|
||
| message PeerData { | ||
|
|
||
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.
💡 Suggestion (F9): these are the only lines this PR touches that no test covers
The
return errbodies inside both epoch loops (lines 714-716 here, and 743-745 inProcessEconomicsEndOfEpochV2) have zero coverage. The gap predates this PR, but because the call lines change here (the newenforcementargument), they now land in the new-code coverage denominator that the quality gate measures. Every other executable line this PR touches in non-generated code is covered.The volume is small (two statements), so this is a cheap fix rather than an important one: one test per version that makes
processValidatorEpochV1/V2fail, for example through a peer account load error, and asserts the error surfaces fromProcessEconomicsEndOfEpochV1/V2. There is a recent precedent for this shape of test ondevelop(94bc4b1, pinningSetNodeserror propagation).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.
Fixed in 7c71910: added
TestProcessEconomicsEndOfEpoch_PropagatesPerValidatorError, table-driven over V1/V2, which forcesLoadPeerCalledto return an error and asserts it surfaces viarequire.ErrorIsfromProcessEconomicsEndOfEpoch. Mutation-tested by swallowing the loop error in each version independently to confirm the test actually catches the regression.