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

LCD REST-server API #383

Closed
faboweb opened this issue Jan 24, 2018 · 20 comments
Closed

LCD REST-server API #383

faboweb opened this issue Jan 24, 2018 · 20 comments
Assignees

Comments

@faboweb
Copy link
Contributor

faboweb commented Jan 24, 2018

How should the LCD REST-server be structured? This is a first proposal. I will add a full OpenAPI definition later.

Resources:

Version - Version of local code
Status - Status of the connected node
Keys - Locally stored keys
Accounts - Nonce and balances on the blockchain
Blocks - All blocks in the blockchain
ValidatorSets - Sets of validators at a block-height
TXs - All TXs in the blockchain

Online spec viewer:
https://app.swaggerhub.com/apis/faboweb1/Cosmos-LCD/1.0.0-oas3

@faboweb faboweb changed the title LCD REST-server structure LCD REST-server API Jan 24, 2018
@jbibla
Copy link
Contributor

jbibla commented Feb 6, 2018

please consider error handling luniehq/lunie#445

@mappum
Copy link
Contributor

mappum commented Feb 6, 2018

please consider error handling luniehq/lunie#445

Specifically, we need 404 errors to actually get returned as 200 but with a body that specifies the result is empty. Many 400-level errors happen during normal operation (e.g. querying our account which has no balance yet), but Electron doesn't let us hide these errors so we get spammed.

@jbibla
Copy link
Contributor

jbibla commented Feb 6, 2018

thanks for the clarifcation @mappum !

@faboweb
Copy link
Contributor Author

faboweb commented Feb 21, 2018

https://app.swaggerhub.com/apis/faboweb1/Cosmos-LCD/1.0.0-oas3

A first version of the API spec is done. Feedback (this week) would be highly appreciated.

@ebuchman
Copy link
Member

Thanks Fabo! Some thoughts:

  • GET /keys/seed feels a bit weird since its creating a key - dont we need to give it a name ?

  • POST /balances/{address} … this is basically just a shortcut to a SendTx ?

  • what is GET /tx ?

  • I think we need GET /account or GET /nonce, no? It's needed if we end up building transactions in multiple steps (create, sign, send). But it might make sense to just replace /balances with /account ?

  • Question about naming - when should we use plural? GET /blocks/latest returns one single block, always. Same with /validatorsets. Maybe those shouldn't be plural? /balances is ok because an account can have many different coins ... but it might be simpler as just /account

Otherwise this looks like a great start!

@faboweb
Copy link
Contributor Author

faboweb commented Feb 21, 2018

GET /keys/seed feels a bit weird since its creating a key - dont we need to give it a name ?

This shouldn't create a key. This should just generate a seed phrase like mentioned here #301 .

POST /balances/{address} … this is basically just a shortcut to a SendTx ?

yeah, makes things sooo much easier

/balances and /account

I renamed /account to /balances so POST /balances/address feels more natural. I will change this to /accounts so this fits more in the rest of the company naming.

Plurals

In REST you usually use plurals for resource naming. As the top resource is a collection of several entities. I.e.: /blocks is the resource of all blocks even if we will never query all of them. We send select one entity out of this resource with the second argument => /blocks/latest

/nonce

I forgot this one. Will add it ASAP!

Finally: Thank you for the incredibly fast feedback! 🚀 🎆

@faboweb
Copy link
Contributor Author

faboweb commented Feb 21, 2018

  • Renamed /balances to /accounts
  • Renamed /tx to /txs
  • Added GET /accounts/{address}/nonce

@ebuchman
Copy link
Member

This shouldn't create a key. This should just generate a seed phrase like mentioned here #301 .

Right. Can you walk me through what happens? User clicks create account, is given a seed, told to write down that seed. Then it can enter the seed (and a password) into the UI to create an account key on disk and use it for signing ?

POST /balances/address

Maybe we should make a set of tx endpoints so this is clearer - eg. POST /tx/send. Then we can do tx/delegate, tx/vote, etc. They're still one-stop-shop for sending txs but the naming makes it clear which endpoints actually create txs on the chain. Thoughts ?

In REST you usually use plurals for resource naming. As the top resource is a collection of several entities. I.e.: /blocks is the resource of all blocks even if we will never query all of them. We send select one entity out of this resource with the second argument => /blocks/latest

Ok!

Added GET /accounts/{address}/nonce

Note if you get the full account, the nonce is included in it, so you shouldnt need the extra /nonce

@faboweb
Copy link
Contributor Author

faboweb commented Feb 21, 2018

Can you walk me through what happens?

Left a comment in #301.

tx/delegate

I am open to tx/x for sending. The idea was to make the transaction logic completely transparent to the developer. The developer will only want to "send coins to an account" or "vote on a proposal", he probably doesn't care about how this happens. I don't feel to strongly about this. Let's wait on more feedback and in doubt take your approach.

so you shouldnt need the extra /nonce

I probably do it wrong, but the current output doesn't show me the nonce:
image
How do I query for the full account?

@faboweb
Copy link
Contributor Author

faboweb commented Feb 21, 2018

Changed /candidates to /delegates recommended by @jolesbi

@ebuchman
Copy link
Member

Currently I dont think the full account is returned. We'd need to change that to actually return the whole thing, and then itd include the nonce too

@mappum
Copy link
Contributor

mappum commented Feb 21, 2018

Nice work @faboweb

👍 I like the idea of combining the tx building/signing/sending for convenience.

I'd like to be able to atomically do multiple bonds and unbonds in one operation, and I'm not sure how the API would work for that (BTW this was not possible in the previous API, it's just something that would be nice to have).

I prefer "verb" actions like SendTx to be less RESTful and be named something like /send, but I'm open to doing it either way.

Overall I approve, it's more organized/consistent than the previous endpoints.

@faboweb
Copy link
Contributor Author

faboweb commented Feb 22, 2018

As recommended by Matt I:

  • changed POST /txs to POST /txs/send
  • changed POST /accounts/{address} to POST /accounts/{address}/send
  • added endpoints for bulk operations POST /delegates/bond, POST /delegates/unbond, POST /accounts/send

And I:

  • removed pubkey from nested send endpoints for /delegates/{pubkey}/bond and /delegates/{pubkey}/unbond

@faboweb faboweb self-assigned this Feb 22, 2018
@ebuchman
Copy link
Member

What's the difference between txs/send and /accounts/{address}/send ? the latter is just a convenicence function for the former, where we only send to one account?

@faboweb
Copy link
Contributor Author

faboweb commented Feb 23, 2018

I would designed txs/send as the endpoint to send arbitrary signed Txs. Where /accounts/{address}/send is the described (build -> sign -> send) convenience endpoint only for sending coins.

@ebuchman
Copy link
Member

ebuchman commented Feb 23, 2018

/txs/send is confusing as it suggests a SendTx. A SendTx can send funds from many people to many people. So /accounts/{address}/send is a special case of that, presumably sending from one person to one person.

If the intention of /tx/send is to submit an already created and signed transaciton (whether SendTx or otherwise), it should be /tx/broadcast or similar, so as not to overload the word "send".

If the intention is just for SendTx, then /tx/send is fine, and we can also have eg. /tx/delegate, /tx/vote etc. But possibly those are deserving of better endpoints within the domain of what theyre doing (eg. /delegates/{pubkey}/bond to delegate to that pubkey)

@faboweb
Copy link
Contributor Author

faboweb commented Feb 26, 2018

If the intention of /tx/send is to submit an already created and signed transaciton (whether SendTx or otherwise), it should be /tx/broadcast or similar, so as not to overload the word "send".

Good point. Changed the endpoint to /tx/broadcast.

@ebuchman
Copy link
Member

@faboweb how does the swagger hub link work ? can we have real-time links for master and develop ?

@faboweb
Copy link
Contributor Author

faboweb commented Feb 28, 2018

This is apparently possible, but I would need to downgrade the spec to version 2 of OpenAPI. Version 3 is not available for all the plugins. I will create an issue.

@ebuchman
Copy link
Member

ebuchman commented Mar 1, 2018

Thanks. Let's close this. Changes to spec are new issues/PRs.

@ebuchman ebuchman closed this as completed Mar 1, 2018
@faboweb faboweb mentioned this issue Mar 1, 2018
9 tasks
ParthDesai pushed a commit to ChorusOne/cosmos-sdk that referenced this issue Apr 19, 2021
gibson042 pushed a commit to gibson042/cosmos-sdk that referenced this issue Apr 12, 2024
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

No branches or pull requests

4 participants