Skip to content

Provide basic HTTP headers including status code and content-type#23

Closed
sticktrk wants to merge 1 commit into
lnurl:masterfrom
sticktrk:patch-1
Closed

Provide basic HTTP headers including status code and content-type#23
sticktrk wants to merge 1 commit into
lnurl:masterfrom
sticktrk:patch-1

Conversation

@sticktrk

Copy link
Copy Markdown
  • The purpose of this pull request is to specify some appropriate HTTP headers - mainly HTTP/1.1 400 Bad Request and HTTP/1.1 200 OK and show them in the examples.
  • Explicitly expressing Content-Type: application/json;
  • Standardize response formatting

The purpose of this pull request is to specify some appropriate HTTP headers - mainly `HTTP/1.1 400 Bad Request` and `HTTP/1.1 200 OK` and show them in the examples. 

Also explicitly expressing `Content-Type: application/json;`
@fiatjaf

fiatjaf commented Nov 23, 2019

Copy link
Copy Markdown
Collaborator

If wallets begin to rely on this to determine what to do with the response they will break because many of the existing services already return 200 for errors. They should then check for both status: "ERROR" and the HTTP code, which means the HTTP code is useless and a nuisance.

I think instead there should be a note on HTTP status codes: "It shouldn't matter. Just return 200. We're bypassing HTTP conventions here to create something much better."

@sticktrk

sticktrk commented Dec 8, 2019

Copy link
Copy Markdown
Author

If wallets begin to rely on this to determine what to do with the response they will break because many of the existing services already return 200 for errors.

Existing wallets will not break, they will disregard the status code as they do now (if they are following spec)

They should then check for both status: "ERROR" and the HTTP code, which means the HTTP code is useless and a nuisance.

There is no ambiguity with a HTTP status code. The current implementation has {"status":"ERROR"} naturally being a string - it can interpreted case insensitive/sensitive. It's too late to change this {"status"} field, but correct status codes will help push in the right direction going forward.

Example: BlueWallet expects case sensitive where as almost every other supporting wallet supports case insensitive. In my experience with web dev, this is not a situation where case should matter - as the interpreting language method of string compare can vary.

https://github.com/BlueWallet/BlueWallet/blob/4f50e08411e7bc129f7c5cb1cd87010316c319de/screen/lnd/lndCreateInvoice.js#L80

Or maybe this is all because PHP has disambiguation issues :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants