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

Consider relaxing NodeId: Default bound #890

Open
xDarksome opened this issue Jul 2, 2023 · 4 comments
Open

Consider relaxing NodeId: Default bound #890

xDarksome opened this issue Jul 2, 2023 · 4 comments
Assignees

Comments

@xDarksome
Copy link

xDarksome commented Jul 2, 2023

Is your feature request related to a problem? Please describe.

We're currently integrating libp2p with openraft and having a small issue of libp2p::PeerId not implementing Default (and conceptually I don't think it should).

Describe the solution you'd like

It would be great if you could remove the bound

Describe alternatives you've considered

For now, we're going to use a newtype on top of libp2p::PeerId or some intermediary type.

Additional context

It seems that the Default is being used quite extensively across the codebase. However, I don't think it's actually necessary - Option<T> should suffice.

@github-actions
Copy link

github-actions bot commented Jul 2, 2023

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@drmingdrmer drmingdrmer self-assigned this Jul 3, 2023
@drmingdrmer
Copy link
Member

I'm afraid removing NodeId: Default from the current codebase may not be straightforward. Types like LeaderId need to offer a zero value for uninitialized states or testing purposes. Utilizing Option would introduce unnecessary error handling using unwrap().

For your specific scenario, a potential and straightforward solution is to encapsulate libp2p::PeerId within another struct that implements Default:

struct P2PNodeId(libp2p::PeerId);
impl Default for P2PNodeId {
  fn default() -> Self { /* ... */ }
}

@xDarksome
Copy link
Author

Yes, that's exactly how we're handing it now, it works fine, aside from some additional boilerplate of converting the types back and forth.

If there's ever going to be a refactoring/redesign of how you handle LeaderId internally, please, consider this issue.

@drmingdrmer
Copy link
Member

Glad to hear that the workaround is functioning well for you.
If there's ever an opportunity to refactor, I will definitely keep your suggestion in mind.

Thank you for raising this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants