Closed DanGould closed 1 month ago
I've addressed your main concerns, but I'd still like to add that compressed_public_key()
accessor to bitcoin-hpke
, need to update the lock files, and want to make sure I'm satisfied with the src/v2.rs mod function names and params
compressed key was tabled for now as non-critical and documented in an bitcoin-hpke repo issue. Otherwise this is ready for review
Nice catches. Why even have the abstraction if you're not going to use the helpers. DOH!
I haven't smoke tested due to existing payjoin directories not supporting this hpke configuration
since the integration and e2e tests spin up payjoin directories with this configuration, that won't be a problem, would it? We must discuss deploying the new directory. I don't know that anyone is relying on it in production, so "just do it" might be the strategy.
Agreed on shipping the new directory.
This PR effectively integrates bitcoin-hpke, replacing the custom aead algorithm for point #216 point 4
The biggest review points of this PR are the design of the abstraction layers between
payjoin
andbitcoin-hpke
(and to a lesser extentpayjoin
tobitcoin-ohttp
tobitcoin-hpke
).Some things to consider:
payjoin implements
serde::{Serialize,Deserialize}for
bitcoin-hpkekeys where they could be enabled with a feature, though that would further depart from
rust-hpke`'s main branch. I think it makes sense to upstream it. reviewer, what do you think?ctx.public_key()
orohttp_keys.public_key()
for easier extraction? I feel like what I've developed here to solve #344 is a bit of a hack but also a significant quality of life improvement for anyone handling bip21 URIs. Similar to upstreaming to bitcoin-hpke, what do you think of upstream change that departs from the mainohttp
branch @spacebear?