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
[network] Make NetworkId & NetworkContext copyable #9203
Conversation
e701801
to
78257a2
Compare
78257a2
to
3edd7bf
Compare
4881303
to
ba6106b
Compare
/canary |
ba6106b
to
52beb65
Compare
Cluster Test Result
Repro cmd:
🎉 Land-blocking cluster test passed! 👌 |
52beb65
to
5053d2e
Compare
5053d2e
to
7ab7039
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.
Looks great! Next cool thing to do (eventually, probably not anytime soon since we'd need another logging stack change) would be to using tracing's Span
concept to open a span with the network context data in it so all logging events inherently have this data instead of needing to pipe it through everywhere.
self == &NetworkId::Vfn | ||
} | ||
|
||
pub fn is_validator_network(&self) -> bool { | ||
matches!(self, NetworkId::Validator) | ||
self == &NetworkId::Validator |
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.
small nit (probably not worth fixing) Using a match statement or matches
! macro may be a bit more idiomatic
/land |
I removed the "private" network and replaced it with "vfn" so that we don't have to clone NetworkId everywhere. Given we have no other private networks, this should be fine for now. It did have to add an invalid type for backwards compatibility in conversion
Cluster Test Result
Repro cmd:
🎉 Land-blocking cluster test passed! 👌 |
7ab7039
to
3b5e28b
Compare
Motivation
I made a mistake when I first created NetworkId, making it have to be cloned everywhere because of the contained string. This made it more unwieldy and a bunch of extra work around using the NetworkId.
Now, we can copy and make everything much easier, no longer copying around the string "vfn". Additionally, cleans up some code and we are able to get rid of the
Arc<NetworkContext>
to justNetworkContext
.Migration Plan
Invalid
slot instead of the currentVfn
slot for readability purposesHave you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Take a look at my backwards compatible tests for serialization. They cause some pain around having an unused value in the enum, but everything works outside of that!
Related PRs
Related to #9191 and thinking about how we can standardize identifiers across networks. This PR definitely is blocked on that one (due to the ton of merge conflicts that are going to happen