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

Node API V2 #28

Open
wants to merge 20 commits into
base: master
from

Conversation

@quentinlesceller
Copy link
Member

quentinlesceller commented Oct 1, 2019

@quentinlesceller quentinlesceller changed the title [WIP] Node API V2 Node API V2 Oct 9, 2019
@quentinlesceller quentinlesceller marked this pull request as ready for review Oct 9, 2019
@quentinlesceller

This comment has been minimized.

Copy link
Member Author

quentinlesceller commented Oct 9, 2019

Ready for review.

@kargakis

This comment has been minimized.

Copy link
Contributor

kargakis commented Oct 9, 2019

How do you return errors in the new API? Are they typed or do you only rely on HTTP statuses? Can you add an example?

@quentinlesceller

This comment has been minimized.

Copy link
Member Author

quentinlesceller commented Oct 9, 2019

@kargakis I'll add a few examples of errors you can get!

@quentinlesceller

This comment has been minimized.

Copy link
Member Author

quentinlesceller commented Oct 9, 2019

@kargakis added a few examples. But basically it's like the v2/v3 grin-wallet API.

@quentinlesceller quentinlesceller referenced this pull request Oct 11, 2019
11 of 11 tasks complete
Copy link
Contributor

lehnberg left a comment

@quentinlesceller really nice job on this - I'd gladly shepherd this RFC through the process of getting merged, if okay. Just gave some minor feedback inline.

More generally, as this will be a reference document if merged, it's good to write the RFC in present tense like it has already been approved and merged. And avoid having language like "if accepted" or "proposed" in it, as it will only be live if accepted already, if that makes sense.

See a similar discussion on RFC0005 here.

text/0000-node-api-v2.md Show resolved Hide resolved
text/0000-node-api-v2.md Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
Copy link
Contributor

lehnberg left a comment

Hi @quentinlesceller I took another detailed pass on the document, see comments of minor tweaks.

The actual API documentation looks really good to me - reads clear. I only had suggestions to make the RFC be more "future proof" so that it remains up to date and accurate even after it's been merged, and some minor tweaks here and there.

text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved
text/0000-node-api-v2.md Outdated Show resolved Hide resolved

## Authentication

Like the v1 API, the v2 API uses basic auth with the same secret. This token is usually in `grin/main/.api_secret`.

This comment has been minimized.

Copy link
@hashmap

hashmap Nov 14, 2019

Member

Perhaps this model is too simple, we clearly have more than one API here, at least 2 - data consumer and node management (perhaps also data producer - push_tx), which may require different tokens or more granular auth model.

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Nov 14, 2019

Author Member

Clearly we could separate them in 2 or 3 difference endpoints. Do we want to add such complexity though. For such use case someone can just build a middleware on top of it to filter the requests?

This comment has been minimized.

Copy link
@hashmap

hashmap Nov 15, 2019

Member

Logically we have 2 APIs here, let's call them Blockchain API and Node API. The former is not node-specific (at least eventually), the later is. There are 2 different use cases and sets of clients. Security-wise exposing node api to internet is very risky. It's not that easy to restrict access to different JSON-RPC calls as for REST API (simple URL filtering is enough).
Token per endpoint model doesn't seem too hard to me, 1-2 lines of code to lookup a config key eg api_token_{endpoint_name}

This comment has been minimized.

Copy link
@quentinlesceller

quentinlesceller Nov 15, 2019

Author Member

I see what you mean. However I'm still not sure it's worth the additional complexity here. It's also possible to do filtering with JSON RPC method. Not against but not convinced :)

This comment has been minimized.

Copy link
@jaspervdm

jaspervdm Nov 15, 2019

Member

I agree with this concern, it might be worthwhile to split them. A wallet only needs very few of these endpoints to fully function, so anyone exposing their node publically would prefer to only have those few methods available on the outside (or without api secret token).

@lehnberg

This comment has been minimized.

Copy link
Contributor

lehnberg commented Nov 16, 2019

In accordance with our RFC process, as of today this can be considered in Final Comment Period with a disposition to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.