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.

Sign up for GitHub

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

Jump to bottom

feat(identity): add From impls for specific keypair types #3626

Merged
merged 6 commits into from
Mar 16, 2023
Merged

feat(identity): add From impls for specific keypair types #3626

merged 6 commits into from
Mar 16, 2023

Conversation

creativcoder
Copy link
Contributor

@creativcoder creativcoder commented Mar 16, 2023 •
edited by thomaseizinger
Loading

Description

Closes #3618.

Notes & open questions

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

@creativcoder creativcoder changed the title Add From impls for keypairs feat(identity): Add From impls for keypairs Mar 16, 2023
thomaseizinger
thomaseizinger requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

One request, otherwise LGTM.

Can you add a changelog entry please and bump the patch version?

identity/src/ecdsa.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title feat(identity): Add From impls for keypairs feat(identity): add From impls for specific keypair types Mar 16, 2023
@creativcoder
Copy link
Contributor Author

Sure, changelog.md at core/ or should i create a new one under identity/ ?

@thomaseizinger
Copy link
Contributor

Sure, changelog.md at core/ or should i create a new one under identity/ ?

New one in identity please. Add a link the root changelog to it too please!

thomaseizinger
thomaseizinger requested changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Slight adjustment, otherwise LGTM

identity/src/keypair.rs Outdated
Comment on lines 267 to 273
#[cfg(feature = "ecdsa")]
impl From<crate::ecdsa::Keypair> for self::Keypair {
fn from(kp: crate::ecdsa::Keypair) -> Self {
#[allow(deprecated)]
crate::Keypair::Ecdsa(kp)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "ecdsa")]
impl From<crate::ecdsa::Keypair> for self::Keypair {
fn from(kp: crate::ecdsa::Keypair) -> Self {
#[allow(deprecated)]
crate::Keypair::Ecdsa(kp)
}
}
#[cfg(feature = "ecdsa")]
impl From<ecdsa::Keypair> for Keypair {
fn from(kp: ecdsa::Keypair) -> Self {
#[allow(deprecated)]
Keypair::Ecdsa(kp)
}
}

I believe we can make this a bit more concise, same applies to the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

thomaseizinger
thomaseizinger reviewed Mar 16, 2023
View reviewed changes
identity/CHANGELOG.md Outdated Show resolved Hide resolved
identity/CHANGELOG.md Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
@divagant-martian divagant-martian mentioned this pull request Mar 16, 2023
Closed
thomaseizinger
thomaseizinger approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for your work and patience :)

@mergify mergify bot merged commit 14292c4 into libp2p:master Mar 16, 2023
@thomaseizinger
Copy link
Contributor

@mxinden Do you mind if I have a go at releasing this as soon as it is merged?

mxinden
mxinden reviewed Mar 16, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

No objections to releasing, though I have a question below.

For the release process, I suggest removing the [unreleased], then merging master into v0.51 (fast forward, no merge commit needed), and then releasing from there.

identity/src/keypair.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger mentioned this pull request Mar 18, 2023
Merged
4 tasks
@thomaseizinger
Copy link
Contributor

For the release process, I suggest removing the [unreleased], then merging master into v0.51 (fast forward, no merge commit needed), and then releasing from there.

Sounds good. I started with a prepare PR here: #3626 (review)

Once merged, I'll locally fast-forward merge master into v0.51, do a release and push the resulting commits and tags.

mergify bot pushed a commit that referenced this pull request Mar 18, 2023
@thomaseizinger
chore(identity): prepare 0.1.1 release
8f271ca 
As discussed in #3626 (review).

Pull-Request: #3631.
@thomaseizinger
Copy link
Contributor

Unfortunately, I couldn't finish the release because libp2p-identity only has you as an owner @mxinden: https://crates.io/crates/libp2p-identity

@thomaseizinger
Copy link
Contributor

The PR is merged and v0.51 is updated and pushed, all you'd need to do is run the release command please.

@thomaseizinger thomaseizinger mentioned this pull request Mar 18, 2023
Open
@mxinden
Copy link
Member

mxinden commented Mar 18, 2023

Unfortunately, I couldn't finish the release because libp2p-identity only has you as an owner @mxinden: https://crates.io/crates/libp2p-identity

My bad. Added the rust-libp2p-maintainers group just now.

The PR is merged and v0.51 is updated and pushed, all you'd need to do is run the release command please.

I will get to it next week. @thomaseizinger feel free to race me to it in case you have time beforehand.

@thomaseizinger
Copy link
Contributor

Tagged and published.

@mxinden
Copy link
Member

mxinden commented Mar 20, 2023

🎉 first release by @thomaseizinger 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow converting ed25519::Keypair to enum Keypair
3 participants
@creativcoder @thomaseizinger @mxinden