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

feat: introduce libp2p-identity crate #3350

Merged
merged 50 commits into from
Mar 12, 2023
Merged

feat: introduce libp2p-identity crate #3350

merged 50 commits into from
Mar 12, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jan 19, 2023

Description

This patch combines the libp2p_core::identity and libp2p_core::peer_id modules into a new crate: libp2p-identity.

Resolves #3349.

Notes

This initial patch only moves the code and adds feature flags. What is yet undecided is how we want to adjust the public API as part of this. The goal would be that the first release (0.1) is backwards compatible with what we currently ship. In a 2nd release, we then harden the public API such that breaking changes will be very unlikely in the future. As part of 0.2, I also want to remove the dependency on multiaddr to enable multiformats/rust-multiaddr#73 which requires multiaddr to depend on libp2p-identity.

What do people think about this plan? @mxinden @jxs @elenaf9

Links to any relevant issues

Open Questions

Further tasks

  • Seal public API by making Keypair and PublicKey opaque structs. I think we should not have conditional enum variants. Those are a hazard. Someone could be exhaustively matching on them with one feature combination and adding a dependency that activates another suddenly causes the program to fail to compile. Even worse, this can happen in dependencies that don't add the necessary cfgs.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@jxs
Copy link
Member

jxs commented Jan 26, 2023

Makes sense to me Thomas 👍

@thomaseizinger thomaseizinger marked this pull request as ready for review January 27, 2023 05:14
@mergify
Copy link
Contributor

mergify bot commented Feb 1, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@thomaseizinger
Copy link
Contributor Author

Change looks good to me minus the deprecation warning.

I've removed the deprecation warning.

Until we release these changes, we will have a broken CI because of #3350 (comment). Can you exercise your admin rights to merge this and make patch releases of all crates?

This PR attracts a lot of merge conflicts so I'd rather not keep it updated for a long time.

Friendly ping @mxinden.

@mergify

This comment was marked as resolved.

@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mxinden
Copy link
Member

mxinden commented Mar 10, 2023

Update here. I won't get to releasing this today, thus not yet merging. Otherwise other pull requests are red in the meantime.

I hope to get to this Saturday or Monday.

@thomaseizinger no need to resolve merge conflicts in the meantime. Sorry for the delay.

@mxinden
Copy link
Member

mxinden commented Mar 12, 2023

As discussed, semver check failure for libp2p-core is a false positive. Thus merging here. Will start the release process next.

@mxinden mxinden merged commit 2a14df2 into master Mar 12, 2023
@mxinden mxinden deleted the feat/libp2p-identity branch March 12, 2023 14:47
@thomaseizinger
Copy link
Contributor Author

Thank you for releasing this!

As part of the next breaking change, I want to remove the multiaddr dependency and clean up the API of libp2p-identity.

@drHuangMHT
Copy link
Contributor

This kind of breaks my own implementation lol. I tried to keep PeerId and Keypair in the same place to unify the use. But opaque Keypair seems to make it harder to export keys? Because you cannot know the algorithm used in the keypair at a glance and I have to try calling those methods to see what it is, which is not so ergonomic. If you don't know that the algorithm is, it will be harder to import keys.

@thomaseizinger
Copy link
Contributor Author

This kind of breaks my own implementation lol. I tried to keep PeerId and Keypair in the same place to unify the use. But opaque Keypair seems to make it harder to export keys? Because you cannot know the algorithm used in the keypair at a glance and I have to try calling those methods to see what it is, which is not so ergonomic. If you don't know that the algorithm is, it will be harder to import keys.

I am happy to add features to make exporting easier to use. I am not sure why our to_protobuf_encoding is actually unsupported for anything other than ed25519 (cc @mxinden ).

Getting the conditional compilation code right is extremely tricky with the current situation and I don't want to put that burden on any of our users hence the deprecation of the direct access to the enum. If you get that wrong and your code happens to be another library, you can easily produce unresolvable compile-errors for the users of your library.

We could add a KeypairKind enum that allows you to branch accordingly. Would that be an acceptable solution?

@thomaseizinger
Copy link
Contributor Author

The specs don't mention anything about key types being deprecated so I think we could add support for encoding the older key types via protobuf.

@mxinden may have more context here on why we don't currently support them.

What is your usecase for using non-ed25519 keys? Those should certainly be the preferred ones.

@drHuangMHT
Copy link
Contributor

drHuangMHT commented Mar 16, 2023

What is your usecase for using non-ed25519 keys? Those should certainly be the preferred ones.

That actually doesn't matter. I'll open a discussion and see what I can do there.

@mxinden
Copy link
Member

mxinden commented Mar 17, 2023

The specs don't mention anything about key types being deprecated so I think we could add support for encoding the older key types via protobuf.

@mxinden may have more context here on why we don't currently support them.

I introduced the protobuf functionality in #2090 in order to import IPFS configs in https://github.com/mxinden/rust-libp2p-server/. I only implemented ed25519 because I didn't need any other. I am not aware of any other reason why the others are not implemented. In other words, contributions welcome.

@thomaseizinger
Copy link
Contributor Author

I opened an issue here: #3630

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
This patch combines the `libp2p_core::identity` and `libp2p_core::peer_id` modules into a new crate: `libp2p-identity`.

Resolves libp2p#3349.

Pull-Request: libp2p#3350.
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

Successfully merging this pull request may close these issues.

Extract libp2p-identity crate
5 participants