Parse and set ohttp= parameter from pj URL fragment
#300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanGould
changed the title
ohttp= parameter from pj URL fragment
DanGould
force-pushed
the
fragment
branch
5 times, most recently
from
9f0e415 to
101c429
Compare
Merged
DanGould
added a commit
that referenced
this pull request
Jul 2, 2024
Refactor `OhttpKeys` so that #300 can just focus on semantics. - First Commit removes a clippy warning - 2nd reduces unnecessary base64 verbosity. base64 dependency was being used where psbt.to_string was fine, and the custom configs were being built where confs could do - 3rd removes a vestigial PartialEq implementation - 4th disambiguates `Display` and `Serialize` roles. [OHTTP KeyConfigs encoding](https://www.ietf.org/rfc/rfc9458.html#name-key-configuration-encoding) for serialization is well specified by the rfc. We're using string representations in URLs and configs just for Payjoin V2 which is a separate thing
DanGould
force-pushed
the
fragment
branch
5 times, most recently
from
0bbef22 to
b2c5127
Compare
spacebear21
reviewed
Jul 8, 2024
There was a problem hiding this comment.
Looks good. It would be helpful to add a unit test that parses a full BIP21 URI, in addition to just the pj URL. Specifically, a BIP21 URI with multiple parameters and & separators to validate the logic in set_ohttp
spacebear21
approved these changes
Jul 9, 2024
|
Thanks for the approval but this is probably still not the correct abstraction, which is why I haven't re-requested review (I don't think I did at least). I wrote the tests you recommended locally and they would fail because this doesn't yet percent-encode the fragment '=' and '&' characters. They conflict with the bip21 params. So I think that such tess should be working to justify our new abstraction before it's merged |
Use InternalPjparseError so as not to require so much boilerplate.
This extension trait defines functions to parse and set the ohttp parameter in the fragment of a `pj=` URL. Close payjoin#298
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #298
This introduces a
Urlextension trait calledpayjoin::uri::UrlExtwhich can set and getohttp=as a fragment rather than a URI parameter. Doing so allows BIP 77 payjoin v2 parameters to be compliant with any BIP 21 bitcoin URI parser that already supports &pj= without introducing new params. This seems to simplify the v1/v2 feature gates too.This change also handles subdirectory parsing differently since the URL no longer ends with the subdirectory, but also the fragment. This introduces new V2 errors. On my first attempt I added subdirectory parsing and setting to
UrlExt, but opted to remove that since it's out of scope for fragment handling and it would probably need to be considered to be feature gated behindsend/receivewhich could make theurimodule messier. (But also perhaps not sinceUriExtis private andsend/receivegates can be enforced in those higher module abstractions whereUriExtis depended upon).The
UriExtis unit tested.I'm not sure how to best handle the difference between Ul paths with or without a trailing slash. Technically the latter is a true subdirectory but for our purposes, we're actually using HTTP endpoints that don't differentiate. Our parser should probably prefer one to another. I'm inclined to prefer fewer characters for simplicity.
e.g.
vs
This was done as prerequisite to #299 since I believe
expshould be a payjoin-specific fragment parameter as well that gets added toUriExt