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

remove usage of Code::Identity in core/src/peer_id.rs #3311

Closed
wants to merge 1 commit into from
Closed

remove usage of Code::Identity in core/src/peer_id.rs #3311

wants to merge 1 commit into from

Conversation

jmmaloney4
Copy link
Contributor

Description

Remove usage of Code::Identity in core/src/peer_id.rs, to fix #3276.

Links to any relevant issues

#3276

Open Questions

I'm unsure how to test this change thoroughly. I couldn't get cargo test to compile, even on the master branch.

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


PeerId { multihash }
PeerId {
multihash: Code::Sha2_256.digest(&key.to_protobuf_encoding()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break lots of code without alerting maintainers through compile errors, i.e. this is really bad: Actyx for example only supports ed25519 and then makes use of the fact that the public key can be recovered from the PeerId.

Changing the bit format of PeerId (for the same key) also is a network breaking change, so such a proposal should come with a thorough analysis of the intended and unintended effects as well as a migration plan. And if the only benefit is to avoid a deprecated internal function, then I don’t think this is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for commenting @rkuhn! There was a misunderstanding on the original issue that I've hopefully been able to clarify! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks!

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

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

@thomaseizinger
Copy link
Contributor

Closing in favor of #3514.

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.

Don't depend on the multihash identity feature
3 participants