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

Make HpkeError::Secp256k1 variant an opaque HpkeError::InvalidPublicKey error #498

Open
DanGould opened this issue Jan 20, 2025 · 2 comments · May be fixed by #511
Open

Make HpkeError::Secp256k1 variant an opaque HpkeError::InvalidPublicKey error #498

DanGould opened this issue Jan 20, 2025 · 2 comments · May be fixed by #511

Comments

@DanGould
Copy link
Contributor

arguably the HpkeError::Secp256k1 variant also leaks implementation details

@nothingmuch

I have no problem exposing this as an opaque error variant without an encapsulated secp256k1::Error if you also find that more appropriate. From what I can tell these are always thrown from secp256k1::PublicKey::from_slice as Err(InvalidPublicKey), and our own HpkeError::InvalidPublicKey variant would handle much the same way as encapsulating secp256k1::Error currently does

Originally posted by @DanGould in #496 (comment)

@nothingmuch
Copy link
Collaborator

nothingmuch commented Jan 20, 2025

Hmm, I'm pretty agnostic to this, if anything I'm leaning on the side of this is an OK detail to leak (so #wontfix on this issue i guess) because realistically there isn't going to be another production quality implementation of this curve, if one materializes we can just bump the right semver digit as we agonize of that ;-)

I really only pointed it out for consistency

@DanGould
Copy link
Contributor Author

I also think unless you really need the internal error, you should not accumulate the complexity of encapsulating it. So this is a problem to fix in #403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants