Change HpkeError::Secp256k1 into the opaque InvalidPublicKey error

[shinghim]

Making this into an opaque error will prevent leaking implementation details

Closes #498

[coveralls]

Pull Request Test Coverage Report for Build 12968934364

Details

• 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 78.528%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/hpke.rs	0	3	0.0%

Totals
Change from base Build 12968704432:	0.0%
Covered Lines:	3650
Relevant Lines:	4648

---

💛 - Coveralls

[nothingmuch]

cACK, but undecided about the details of From impl

[nothingmuch]

Hmm, I'd be more comfortable if this matched value.

if InvalidPublicKey is really only variant we do expect here, i'd also be happy with something that just asserts that value is secp256k1::Error::InvalidPublicKey before discarding it

afaict that is the case, because InvalidEllSwift (the only other variant that seems plausibly relevant) would only be generated from a string encoding, but construct it directly from a fixed size array and i believe every 64 byte array can be interpreted as an EllSwift encoding

if someone objects to maintaining a introducing a panic I'd settle for a comment justifying ignoring the specific variant, and waiting a few years until never_patterns experimental feature hits the project's MSRV ;-) oops i think that wouldn't actually work. anyway, just something to warn future maintainers in case secp256k1 crate gets more error variants that we should account for

[DanGould]

Only so many errors can actually occur at the Secp256k1 layer here. I think matching generally is probably fine. If you want it to be a general catch I'm also ok with naming the error Secp256k1 dropping the internal variant, and making a general statement in Display's implementation. I'd like more information @nothingmuch what other variants beyond InvalidEllSwift could ever happen or where would discarding the inner var and catching all cause problems?

Maintaining a panic and match on the InvalidPublicKey variant is OK for me but I'm not sure it's strictly necessary.

A Secp256k1 => write!(f, "Secp256k1 key parsing failed), seems just as suitable and incurs less maintenance burden (over the tiniest maintenance item ever anyhow)

edit: Listen to @nothingmuch. The changes he requested to match on errors or panic must be made to accept this PR

[nothingmuch]

I'd like more information @nothingmuch what other variants beyond InvalidEllSwift could ever happen or where would discarding the inner var and catching all cause problems?

I don't even think InvalidEllSwift will be relevant, only InvalidPublicKey, so I think i'm most in favor of panicking if anything other than that variant is ever encountered, on the grounds that diagnosing that crash should be straightforward, but if a different error is silently assumed to be InvalidPublicKey when in fact it wasn't (because of some oversight), that would be very confusing to debug

[nothingmuch]

oh and to spell out the specific scenario where I expect such a bug might arise:

1. any upstream changes to secp256k1 crate that introduce a new variant, or change existing ones, updating the dependency will not cause a compilation or runtime error even if behavior changes

2. new contexts in which secp256k1 might be used in relation to hpke stuff, in the future, because From is called implicitly by the compiler as necessary, that is easy to miss. the docs for From indicate conversion should be lossless, which this would be if it is only ever invoked for the InvalidPublicKey variant, that's the main rationale i have for panicking in case it isn't since the type signature can't represent any lossless conversion that the compiler might try to do automatically

[DanGould]

I'm persuaded by @nothingmuch. The changes he requested to match on errors or panic must be made to accept this PR