Use More Semantic HTTP #346
Conversation
There was a problem hiding this comment.
I think following the REST guidelines re: the Location header is worthwhile.
If it's not a heavy lift I'd ask that we add a unit test in payjoin_directory/src/lib.rs that would help describe the expected response format for a 201 with location header and reason about it.
| format!("http://localhost:{}", port), | ||
| port, |
There was a problem hiding this comment.
I'm not sure I follow why port needs to be specified in the base_url and as a separate parameter. Is it just a testing/docker thing?
There was a problem hiding this comment.
In prod, you would just pass e.g. "https://payjo.in" where the port can be assumed by the scheme (http/https), but in testing, you need to pass the explicit port because it's non-default. The base_url could be a totally separate port to the one exposed by the server if it were behind a reverse proxy or something.
payjoin/src/v2.rs
Outdated
| println!("BUILDER"); | ||
| for field in m.header().iter() { | ||
| println!("Adding header: {:?}, {:?}", field.name(), field.value()); | ||
| builder = builder.header(field.name(), field.value()); | ||
| } | ||
| println!("BUILT"); |
There was a problem hiding this comment.
leftover debug printlns
These features required by payjoin-directory but worked unspecified because of a shared Cargo.lock with payjoin-cli previously specifying hyper/full. payjoin-directory would fail if run independently without Cargo.lock or this change, however.
A '/payjoin' suffix was used before this change instead of an HTTP method.
By using a method, we can leave control semantics mostly to the method and
have a clean {session_id} as the only path identifier to select the resource
which is augmented.
Include a 'location' header pointing to the new session resource
DanGould
force-pushed
the
semantic-http
branch
from
79d0562 to
11353f9
Compare
Close #345
These are both backwards-incompatible protocol changes (old v2 vs new v2)
The first patch changes 2 POST requests for sender/receiver to a POST on a session id that establishes the fallback_psbt and a PUT on the session id that updates it to a payjoin_psbt
The second patch has a session ID establishment POST return the new resource URL (session subdirectory) in the "location" header. I'm less sure of this change than the former since it complicates the code for the sake of following "semantic" HTTP POST, but it also prevents a payjoin-directory from creating a resource in another subdirectory on a domain that a client can't easily derive.
@spacebear21 I'm hoping you can weigh in.