-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Metadata V2 Object and Interface #8962
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #8962 +/- ##
===========================================
- Coverage 60.90% 60.88% -0.02%
===========================================
Files 530 531 +1
Lines 37510 37536 +26
===========================================
+ Hits 22845 22855 +10
- Misses 11382 11401 +19
+ Partials 3283 3280 -3 |
proto/beacon/p2p/v1/messages.proto
Outdated
attnets: Bitvector[ATTESTATION_SUBNET_COUNT] | ||
) | ||
*/ | ||
message MetaDataV2 { |
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.
This is not related to this PR, at least by looking at the title and the description. Please update the description
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.
done
"google.golang.org/protobuf/proto" | ||
) | ||
|
||
// MetadataV1 is a convenience wrapper around our metadata protobuf object. |
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.
// MetadataV1 is a convenience wrapper around our metadata protobuf object. | |
// MetadataV0 is a convenience wrapper around our metadata protobuf object. |
I wonder whether using V0
and V1
is better given we mostly start with versioning 0. pb.MetaData
without any versioning makes me think it's closer to 0 than to 1 🤔
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.
good question lol, would technically have to change every reference in that case. I can try and changing it to a 0 and 1 to see how much work it would take.
) | ||
|
||
// Metadata returns the interface of a p2p metadata type. | ||
type Metadata interface { |
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.
Do you want to add SyncBitfield() as well if you were to include V2 in the same PR
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.
For the general metadata interface it should not have anything from V2.
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 was thinking of adding this in our altair branch instead of here, given that is where it will be used.
What type of PR is this?
In Preparation for Altair Hardfork
What does this PR do? Why is it needed?
Which issues(s) does this PR fix?
Part of #8638
Other notes for review