use compressed representation for reply key

[nothingmuch]

Closes #492

[coveralls]

Pull Request Test Coverage Report for Build 12867632160

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 22 of 23 (95.65%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 60.549%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/hpke.rs	22	23	95.65%

Totals
Change from base Build 12832868611:	0.1%
Covered Lines:	2956
Relevant Lines:	4882

---

💛 - Coveralls

[nothingmuch]

@DanGould thoughts on modifying the HPKE crate to support ellswift & compressed serializations more directly? arguably that's out of scope, but it wouldn't add any dependencies. alternatively, what about exposing a pub method that returns the internal secp256k1::PublicKey (but not making the type member itself pub). this is also a bit meh because it would expose implementation details of the hpke crate.

i think i'd still be in favor because pretending the bitcoin hpke stuff is agnostic to secp256k1 seems kind of pedantic, and this conversion is a bit roundabout (harming readability), adds boilerplate, and is also a bit wasteful (although i think the wastefulness can be mostly addressed by using the unchecked variants when going through the uncompressed form, saving a modular inversion on the field which is the expensive part of checking that a point is on the curve).

[DanGould]

thoughts on modifying the HPKE crate to support ellswift & compressed serializations more directly? arguably that's out of scope, but it wouldn't add any dependencies.

I'm cool with it. Seems like other applications might have a use for it, and the abstraction seems more appropriate and ergonomic there.

what about exposing a pub method that returns the internal secp256k1::PublicKey

Opposed because like you say then it varies a bit more from the generic HPKE implementation, but in favor because that bitcoin_hpke crate is specifically about the use case here AND for potential nostr OpenMLS implementations, which I reckon would benefit from the secp256k1 specifics.

I'd need to more deeply consider how big the change is, and especially the maintenance cost of diverging from rust-hpke in that way.

[DanGould]

ACK e9c7050

[DanGould]

May you import bitcoin::secp256k1 to abbreviate calls here and elsewhere in the file?

[nothingmuch]

initially I did but it makes it kinda confusing as there's already these types in hpke.rs:

pub type PublicKey = <SecpK256HkdfSha256 as hpke::Kem>::PublicKey;
pub struct HpkePublicKey(pub PublicKey);
pub type EncappedKey = <SecpK256HkdfSha256 as hpke::Kem>::EncappedKey;

all of which are representations of public keys that are actually meaningful, whereas this one is just for the serialization

maybe importing only in the functions to emphasize that makes sense? although it doesn't actually reduce the boilerplate

[nothingmuch]

oh wait do you mean only use bitcoin::secp256k1 and write it as secp256k1::PublicKey to omit the bitcoin:: prefix?

[DanGould]

Yes the latter use bitcoin:secp256k1 to omit the bitcoin:: prefix

[nothingmuch]

Opposed because like you say then it varies a bit more from the generic HPKE implementation, but in favor because that bitcoin_hpke crate is specifically about the use case here AND for potential nostr OpenMLS implementations, which I reckon would benefit from the secp256k1 specifics.

Hmm, arguably the HpkeError::Secp256k1 variant also leaks implementation details

[DanGould]

Hmm, arguably the HpkeError::Secp256k1 variant also leaks implementation details

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

[DanGould]

ACK c5b6c13