Propagate ParseError when retrieving ohttp parameter

[shinghim]

Not sure if you guys are accepting contributions, but I recently came across payjoins and thought it was a cool idea and wanted to see if i could contribute.

This PR will update the ohttp() getter to propagate the parse errors as part of #399. If this looks good, i can submit another patch to do the same for exp()

[DanGould]

Heck yes we are. Seeing PRs open like this in the morning is like seeing flower buds appear in the garden. Thanks for the contribution @shinghim ✨

Check out the contributing section of the readme to figure out what the CI expects for formatting. You can also run tests locally as long as Docker is running with .contrib/test_local.sh. I'd sincerely appreciate a patch to do the same on exp() or anything else you're interested in pushing.

[shinghim]

Gotcha, I was waiting till I could figure out the testing bit before making this ready for review - I’ll work on that when I get some free time and the ‘exp’ part as well

[DanGould]

I do think this is probably an error variant that belongs in url_ext and NOT ohttp.rs. Only url_ext has the concept of the parameter being "missing." This type that gets defined in url_ext will likely include whatever error variant exp or other fragment parameters have. Maybe it's a pub enum FragmentError.

[shinghim]

That makes sense. I had the same question while doing this, but I saw here that MissingPubkey is in the same ParseReceiverPubkeyError. Does it make sense to move ParseOhttpKeysError to the error module?

[DanGould]

Hmm the url ParseReceiverPubkeyError that you linked to seems to be better form to me since it's specifically regarding parsing a pubkey out of the receiver pubkey from the Url fragment, and handles the variant regarding the actual pubkey contents as InvalidPubkey(crate::hpke::HpkeError).

I don't think you want to move ParseOhttpKeysError but rather create a new variant that contains it, following the same pattern. I can see how the name of the parse error in ohttp follows the same pattern a the parse receiver pubkey [param] error and that is confusing.

I'd make a ParseOhttpKeysParamError that has an InvalidOhttpKeys(crate::ohttp::ParseOhttpKeysError) variant as well as the missing keys variant, and consider renaming ParseReceiverPubkeyError into ParseReceiverPkParamError or similar.

ParseOhParamError and ParseRkParamError etc. naming by the HRP could work too.

I'm not 100% on the names but I think this explains the difference between the layers of abstraction and why the existing names may have caused confusion.

[coveralls]

Pull Request Test Coverage Report for Build 12200340472

Details

• 24 of 31 (77.42%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 61.944%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/error.rs	0	1	0.0%
payjoin/src/uri/url_ext.rs	21	22	95.45%
payjoin/src/uri/error.rs	0	5	0.0%

Totals
Change from base Build 12184059779:	0.1%
Covered Lines:	2881
Relevant Lines:	4651

---

💛 - Coveralls

[shinghim]

Fixed the formatting and integration tests

[DanGould]

tACK accdaca

Thanks for seeing this one through! Looking forward to reviewing any follow up for exp if you've got it in you as stated in OP. Let's go @shinghim 🔥