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

(libp2p-identity) Confusing deprecation(s) on making PeerId from ed25519 PublicKey bytes #3802

Closed
DougAnderson444 opened this issue Apr 18, 2023 · 4 comments · Fixed by #3805

Comments

@DougAnderson444
Copy link
Contributor

Background

Suppose we want to create a new PeerId from someone's public key bytes, we can do this:

use libp2p_identity::ed25519;
use libp2p_identity::PeerId;
use libp2p_identity::PublicKey;

let pub_key = ed25519::PublicKey::decode(pb_bytes)
    .map(PublicKey::Ed25519)
    .unwrap();

let binary_peer_id = PeerId::from_public_key(&pub_key).to_bytes();

Which works as expected ✔️.
...But with a deprecation notice says to use PublicKey::into_ed25519 instead of the PublicKey::Ed25519 enum:

pub enum PublicKey {
/// A public Ed25519 key.
#[cfg(feature = "ed25519")]
#[deprecated(
since = "0.1.0",
note = "This enum will be made opaque in the future, use `PublicKey::into_ed25519` instead."
)]
Ed25519(ed25519::PublicKey),

Which doesn't do the same thing, and also has it's own deprecation direction here:

#[cfg(feature = "ed25519")]
#[deprecated(
since = "0.2.0",
note = "This method name does not follow Rust naming conventions, use `Keypair::try_into_ed25519` instead."
)]
pub fn into_ed25519(self) -> Option<ed25519::Keypair> {
self.try_into().ok()
}

...which is also deprecated since v0.2.0, which is odd because identity is at v0.1.1 🤔 .

THAT deprecation notice says to use Keypair::try_into_ed25519, which doesn't seem to exist.

So I'm sure you can sense my confusion.

The issue is:

It's really confusing as to why the method that works points to 2 others which don't work or don't exist.

Potential Solution

Unless there is a clear reason or working deprecation path, remove the deprecations? I'm not privy to the logic as to why this is all here.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Apr 19, 2023

Sorry for the confusion! Are you depending on master by any chance?

The latter deprecation warnings are not released yet I think?
I agree that the version numbers should be removed though, they are so easy to get wrong if PRs merge in a different order.

You should be able to use .into to promote your ed25519::PublicKey to an identity::PublicKey. That is missing from the deprecation warnings :/

A PR would be much appreciated and sorry for the confusion!

@DougAnderson444
Copy link
Contributor Author

Thanks @thomaseizinger for understanding and the explanation! I'll send a PR to hopefully clear it up.

I will add impl From<> to PublicKey for the .from/.into to work

@thomaseizinger
Copy link
Contributor

I will add impl From<> to PublicKey for the .from/.into to work

Does that not work already? That was definitely an oversight, sorry for that!

@DougAnderson444
Copy link
Contributor Author

No worries!

To answer your question about which version I was using, it was 0.1.1 from crates.io

I did try .into() and ::from() but neither worked until I added the impl From, but it's all good!

@mergify mergify bot closed this as completed in #3805 Apr 22, 2023
mergify bot pushed a commit that referenced this issue Apr 22, 2023
This patch removes the `version 0.2.0` for deprecations as libp2p-identity is only at `0.1.1` and this can be confusing to the reader.

It also adds `impl From<ed25519::PublicKey> for PublicKey` (et al.) so that `PublicKey::from(ed25519::PublicKey)` works.

Fixes #3802.

Pull-Request: #3805.
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 a pull request may close this issue.

2 participants