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

Ord, Eq, Hash instances for PublicKey for use with collections (sets, maps) #2910

Closed
futpib opened this issue Sep 17, 2022 · 3 comments · Fixed by #2915
Closed

Ord, Eq, Hash instances for PublicKey for use with collections (sets, maps) #2910

futpib opened this issue Sep 17, 2022 · 3 comments · Fixed by #2915

Comments

@futpib
Copy link
Contributor

futpib commented Sep 17, 2022

Description

Implement those traits so that PublicKey can be used as a key in BTreeSet, HashSet, BTreeMap and HashMap.

Motivation

I want to store a set of public keys for authorization. I can not get away with only storing PeerIds because I need to verify signatures.

Current Implementation

No implementation for PublicKey, but there are implementations for PeerId.

Are you planning to do it yourself in a pull request?

Yes if this enhancement is wanted by maintainers.

@dariusc93
Copy link
Contributor

The way I would generally do this is convert the public key (in my case, ed25519) into a string or bytes and use wrapper around that to implements those traits. Considering that ed25519-dalek public key doesnt implement Hash, one would have to do either bytes or string for it when implementing it. The other option might be to store the PeerId in a map (eg HashMap or BTreeMap) with the value containing the PublicKey from libp2p and you could use that for verifying signatures too, though that wont work with HashSet or BTreeSet due to the lack of implementation of those traits unless you use a wrapper of your own.

@thomaseizinger
Copy link
Contributor

I think it is fine to implement traits that we can simply forward to our internal implementations. Most of these missing traits are often just an oversight when wrapping types.

The fact that the dalek types don't implement Hash is a bit of a problem though. We'd have to make a decision on what is meaningful.

There is an open PR on ed25519-dalek: dalek-cryptography/ed25519-dalek#176

Unfortunately, that library is pretty much unmaintained from what I can gather so we shouldn't count on that being merged any time soon. However, we can emulate the implementation of Hash from what that PR would do.

Bottom-line: I am in favor of adding these traits. A PR would be very much appreciated!

@mxinden
Copy link
Member

mxinden commented Sep 21, 2022

My opinion: What @thomaseizinger said above.

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.

4 participants