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

deps(identity): update ed25519-dalek to 2.0 #4337

Merged
merged 8 commits into from
Aug 18, 2023

Conversation

jxs
Copy link
Member

@jxs jxs commented Aug 17, 2023

Description

Fixes #4327.

Notes & open questions

ed25519-dalek has deprecated some try_into methods by putting the slice length in the input type, to maintain identity's type signatures and therefore avoid breaking changes I used [0..32].try_into().
If prefered I can follow ed25519-dalek and change the methods.

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

@altonen
Copy link

altonen commented Aug 17, 2023

Is it possible to get this backported to 0.51?

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.

Looks great! Thanks for the PR.

identity/CHANGELOG.md Show resolved Hide resolved
identity/CHANGELOG.md Outdated Show resolved Hide resolved
identity/src/ed25519.rs Outdated Show resolved Hide resolved
libp2p/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Is it possible to get this backported to 0.51?

We can try! Are you open to fix up potential problems that come up in the backport PR? Let's get this merged first and then we can see.

@thomaseizinger
Copy link
Contributor

@Mergifyio backport v0.51

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

backport v0.51

✅ Backports have been created

@altonen
Copy link

altonen commented Aug 17, 2023

Is it possible to get this backported to 0.51?

We can try! Are you open to fix up potential problems that come up in the backport PR? Let's get this merged first and then we can see.

ok cool and yes I am

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.

Minus the clippy warnings, this looks good to me. Thank you for the work @jxs!

identity/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
[package]
name = "libp2p-identity"
version = "0.2.2"
version = "0.2.3"
Copy link
Member

@mxinden mxinden Aug 17, 2023

Choose a reason for hiding this comment

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

Needs a bump of libp2p-identity in the root Cargo.toml. See CI failure: https://github.com/libp2p/rust-libp2p/actions/runs/5891817364/job/15979771326?pr=4337

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I tried it, but if I do it all other semver-check workflows fail, see here

Copy link
Member

Choose a reason for hiding this comment

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

Maybe cargo-semver-check does not adhere to the override?

rust-libp2p/Cargo.toml

Lines 111 to 118 in cbdbaa8

[patch.crates-io]
# Patch away `libp2p-idnentity` in our dependency tree with the workspace version.
# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
# we import via `rust-multiaddr`.
# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
libp2p-identity = { path = "identity" }

Copy link
Contributor

@thomaseizinger thomaseizinger Aug 17, 2023

Choose a reason for hiding this comment

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

I am surprised that this is showing up now, how did we ever bump this version? Maybe this failure happens now with the recent version change of cargo semver-checks?

The issue with libp2p-identity is that we need to patch it across the workspace and I'd assume that the temporary workspace created by cargo semver-checks does not support that. To properly fix this, we should move libp2p-identity out of the mono-repo. We should very rarely make breaking changes to it anyway because it sits at the very bottom of the dependency chain of the ecosystem.

Copy link
Member

Choose a reason for hiding this comment

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

The issue with libp2p-identity is that we need to patch it across the workspace and I'd assume that the temporary workspace created by cargo semver-checks does not support that.

Maybe @obi1kenobi you have seen this issue before, i.e. that cargo-semver-checks does not adhere to the workspace patches. Note that this is only a suspicion. Issue might as well be on the rust-libp2p end.


@thomaseizinger @jxs I am fine merging here as is, i.e. with the failing cargo-semver-checks check on the libp2p-identity run. Once libp2p-identity v0.2.3 is released we can update our root Cargo.toml. Objections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't believe it copies patches. You can double-check and make sure by looking up the generated manifest inside target/semver-checks. But I'm pretty sure we never read patches from the source manifest so that's probably it.

If running cargo-semver-checks on an intermediate state that isn't right before a cargo publish, checking with patches included makes sense.

But is there risk that pre-publish checking while using patches might falsely say there are no semver issues, only for issues to come up after the crate gets published and used by users that don't have the patches locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated main Cargo.toml to identity 0.2.3

Copy link
Member Author

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks Max, addressed the clippy lints

@@ -1,6 +1,6 @@
[package]
name = "libp2p-identity"
version = "0.2.2"
version = "0.2.3"
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I tried it, but if I do it all other semver-check workflows fail, see here

identity/CHANGELOG.md Outdated Show resolved Hide resolved
identity/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Aug 17, 2023

I will merge and cut a release tomorrow morning.

@mxinden mxinden merged commit fb61697 into libp2p:master Aug 18, 2023
mergify bot pushed a commit that referenced this pull request Aug 18, 2023
Pull-Request: #4337.
(cherry picked from commit fb61697)

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	identity/CHANGELOG.md
#	identity/Cargo.toml
@mxinden
Copy link
Member

mxinden commented Aug 18, 2023

Is it possible to get this backported to 0.51?

We can try! Are you open to fix up potential problems that come up in the backport PR? Let's get this merged first and then we can see.

ok cool and yes I am

@altonen you can build on top of #4343 or cherry-pick yourself. Let me know if you need any help.

@mxinden
Copy link
Member

mxinden commented Aug 18, 2023

Tagged and published.

Thank you for the work @jxs.

thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
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.

RUSTSEC-2022-0093
5 participants