Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing REST endpoints for SignMessage and VerifyMessage #2148

Merged
merged 1 commit into from Dec 4, 2018

Conversation

@xsb
Copy link
Contributor

@xsb xsb commented Nov 4, 2018

Fixes #2096

rpc VerifyMessage (VerifyMessageRequest) returns (VerifyMessageResponse);
rpc VerifyMessage (VerifyMessageRequest) returns (VerifyMessageResponse) {
option (google.api.http) = {
post: "/v1/verifymessage"
Copy link
Member

@Roasbeef Roasbeef Nov 5, 2018

Perhaps this should be a GET instead as it doesn't mutate any state, doesn't produce anything new, and is also a stateless operation?

Copy link
Contributor Author

@xsb xsb Nov 5, 2018

Both SignMessage and VerifyMessage could in theory be implemented as GET. Problem is, as GET requests are not supposed to have a body this implies the message needs to go in the URL. Which produces potentially very long URLs and in certain scenarios the user could find limits on the size of the message that can be passed to these functions. What do you think?

Copy link
Contributor Author

@xsb xsb Nov 5, 2018

Thinking more about it, URL size limits are defined in clients not servers, and afaik only old browsers are problematic in that sense. So I guess this is not a real issue? I'd prefer to use the correct http verb here but I am just not 100% sure it's not an issue to pass messages in URLs.

Copy link
Contributor Author

@xsb xsb Nov 12, 2018

@Roasbeef I can make it a GET but I have a question. The field msg is of type bytes. Should I change it to string instead as it needs to be passed through a URL? Looks like that would break other stuff.

Copy link
Collaborator

@wpaulino wpaulino Nov 12, 2018

bytes fields are encoded as base64 strings, but they're not URL safe, so perhaps it's better to keep it as a POST? Either that or we can note that the base64 string should be padded so that it's URL safe (https://stackoverflow.com/questions/1374753/passing-base64-encoded-strings-in-url), but this isn't very user friendly IMO.

@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented Nov 12, 2018

Needs a rebase!

@xsb xsb force-pushed the rest-endpoints branch from 6251123 to f9df845 Nov 12, 2018
@xsb xsb force-pushed the rest-endpoints branch from f9df845 to ebc34a9 Nov 12, 2018
@xsb
Copy link
Contributor Author

@xsb xsb commented Nov 12, 2018

@wpaulino rebased

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🔑

@Roasbeef Roasbeef merged commit 127bc71 into lightningnetwork:master Dec 4, 2018
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants