Skip to content
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

[Merged by Bors] - Simplify GossipTopic -> String conversion #3722

Closed

Conversation

michaelsproul
Copy link
Member

Proposed Changes

With a few different changes to the gossip topics in flight (light clients, Capella, 4844, etc) I think this simplification makes sense. I noticed it while plumbing through a new Capella topic.

@michaelsproul michaelsproul added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! Networking labels Nov 14, 2022
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you were looking at the logs for this change?

The other thing we need to be careful of is the metrics. I think most of the metrics may not use this, but I know in some of the dash's that we filter on the fork and use the specific topic string format.

Have you seen how the metrics behave after this change?

@michaelsproul
Copy link
Member Author

Have you seen how the metrics behave after this change?

Nah, I just noticed that the Display implementation and the from<GossipTopic> for String were exactly the same (I think?). So there shouldn't be any semantic change due to this PR.

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, didn't look.

LGTM

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 15, 2022
@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Nov 15, 2022
## Proposed Changes

With a few different changes to the gossip topics in flight (light clients, Capella, 4844, etc) I think this simplification makes sense. I noticed it while plumbing through a new Capella topic.
@bors bors bot changed the title Simplify GossipTopic -> String conversion [Merged by Bors] - Simplify GossipTopic -> String conversion Nov 15, 2022
@bors bors bot closed this Nov 15, 2022
@michaelsproul michaelsproul deleted the simplify-gossip-topic branch November 15, 2022 08:32
macladson pushed a commit to macladson/lighthouse that referenced this pull request Jan 5, 2023
## Proposed Changes

With a few different changes to the gossip topics in flight (light clients, Capella, 4844, etc) I think this simplification makes sense. I noticed it while plumbing through a new Capella topic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-hanging-fruit Easy to resolve, get it before someone else does! Networking ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants