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

Implement Gaia-lite LCD Spec #2113

Closed
51 tasks done
jackzampolin opened this issue Aug 21, 2018 · 25 comments
Closed
51 tasks done

Implement Gaia-lite LCD Spec #2113

jackzampolin opened this issue Aug 21, 2018 · 25 comments
Assignees

Comments

@jackzampolin
Copy link
Member

jackzampolin commented Aug 21, 2018

This issue is to track implementation of the newly accepted LCD API spec (#1979). Each of the interchain standards needs to be checked to ensure that it is brought up to spec. The input, return and error values in the spec also need to be updated for each endpoint. Those updates should accompany code changes. Below is a list of the ICS and their endpoints to track progress.

cc @fedekunze

@gamarin2
Copy link
Contributor

Also needed prelaunch, but not in the current spec: signing endpoint in key API.

cc @adrianbrink

@jackzampolin
Copy link
Member Author

Can we open a PR on the spec for that @gamarin2 ?

@gamarin2
Copy link
Contributor

@adrianbrink is supposed to do it

@faboweb
Copy link
Contributor

faboweb commented Aug 22, 2018

GET /bank/balance/{account} should be GET /bank/balances/{account}?
GET /slashing/validator/{validatorAddr}/signing-info should be GET /slashing/validators/{validatorAddr}/signing-info?

@jackzampolin
Copy link
Member Author

Thank you for checking my work there @faboweb

@jackzampolin
Copy link
Member Author

I just added one here: POST /keys/{name}/sign to sign an unsigned transaction. To accompany this and mirror the CLI, each of the routes that produce transactions need to have a ?generate= param that defaults to false, which when set to true returns only the sign bytes for the transaction.

@siman
Copy link

siman commented Aug 29, 2018

Sorry for off-topic, but what does an LCD abbreviation mean?

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 29, 2018

@siman iirc, light (lite?) client daemon 👍

@fedekunze
Copy link
Collaborator

fedekunze commented Aug 29, 2018

Just realized we missed GET /stake/delegators/{delegatorAddr}/redelegations/{validatorAddr} to the list that still needs to be implemented and added to the documentation

@fedekunze
Copy link
Collaborator

Also adding GET stake/validators/{validatorAddr}/delegators and GET stake/validators/{validatorAddr}/delegations as discussed in #2027

@fedekunze
Copy link
Collaborator

Proposal for splitting the POST delegation endpoint: #2191 . CC: @jackzampolin

@fedekunze fedekunze added the lite label Sep 1, 2018
@fedekunze fedekunze changed the title Implement LCD Spec Implement Gaia-lite LCD Spec Sep 1, 2018
@fedekunze
Copy link
Collaborator

I just added one here: POST /keys/{name}/sign to sign an unsigned transaction. To accompany this and mirror the CLI, each of the routes that produce transactions need to have a ?generate= param that defaults to false, which when set to true returns only the sign bytes for the transaction.

@jackzampolin it was renamed to tx/sign. Should rename it ? Personally I'm in favour of it

@abelliumnt
Copy link

abelliumnt commented Sep 23, 2018

[ ] ICS0

  • POST /txs

[ ] ICS1

  • GET /keys
  • POST /keys
  • GET /keys/seeds Do we really need this endpoint?
  • GET /keys/{name}
  • PUT /keys/{name}
  • DELETE /keys/{name}
  • POST /keys/{name}/recover
  • GET /auth/accounts/{address}
  • POST /keys/{name}/sign ***
  • POST /tx/sign
  • POST /tx/broadcast

[ ] ICS20

  • GET /bank/balances/{account}
  • POST /bank/transfers

@jackzampolin
I have some thoughts above the following APIs:

  • POST /txs and POST /tx/broadcast: They are duplicate.
  • POST /tx/sign and POST /keys/{name}/sign : I think the only thing need to be signed is transaction. So only POST /tx/sign is enough.
  • GET /bank/balances/{account} and GET /auth/accounts/{address}: They are duplicate.
  • POST /keys/{name}/recover and POST /keys: POST /keys can add new key. The post body is:
type NewKeyBody struct {
	Name     string `json:"name"`
	Password string `json:"password"`
	Seed     string `json:"seed"`
}

If seed field is not empty, then it will be used to recover a key. So POST /keys/{name}/recover is not necessary.

  • POST /accounts/{address}/send and POST /bank/transfers: Should we rename POST /accounts/{address}/send to POST /bank/transfers?

@abelliumnt
Copy link

abelliumnt commented Sep 23, 2018

@jackzampolin @cwgoes
Why we need both gaiacli rest-server and Gaia-lite LCD? I think their functionalities are duplicated. Could you clarify this?

@faboweb
Copy link
Contributor

faboweb commented Sep 25, 2018

As far as I know: gaiacli rest-server is the command Gaia-lite LCD or better just Gaia-lite is the name for the product/feature.

@faboweb
Copy link
Contributor

faboweb commented Sep 25, 2018

POST /txs and POST /tx/broadcast: They are duplicate.

Let's keep both. POST /txs is more restful, POST /tx/broadcast feels more familiar to the blockchain domain.

POST /tx/sign and POST /keys/{name}/sign

You are correct. Let's stick with just POST /tx/sign

GET /bank/balances/{account} and GET /auth/accounts/{address}

Let's go with both where GET /bank/balances/{account} returns only balances (not sequence etc.) for the sake of consistency with the token transfer POST and to show the bank module is used.

POST /keys/{name}/recover and POST /keys

Let's use the explicit POST /keys/{name}/recover endpoint for recovery and keep POST /keys for key creation which fails if a seed is provided.

POST /accounts/{address}/send and POST /bank/transfers

I propose to name this endpoint POST /bank/accounts/{address} to stay consistent with the GET request and to make it clear the bank module is used for both requests.

@fedekunze
Copy link
Collaborator

fedekunze commented Sep 25, 2018

cc: @faboweb @HaoyangLiu

Let's keep both. POST /txs is more restful, POST /tx/broadcast feels more familiar to the blockchain domain.

The original idea @jackzampolin and I discussed (I don't recall the exact date) is that we should would leave it as POST /txs and have a return parameter in the request body with the desired output: return=<sync|async|block>. But since we are also keeping with POST /tx/sign let's keep the 3 of them

Let's go with both where GET /bank/balances/{account} returns only balances (not sequence etc.) for the sake of consistency with the token transfer POST and to show the bank module is used.

Agree, but lets change GET /bank/balances/{account} to use an /{address} instead

Let's use the explicit POST /keys/{name}/recover endpoint for recovery and keep POST /keys for key creation which fails if a seed is provided.

👍

I propose to name this endpoint POST /bank/accounts/{address} to stay consistent with the GET request and to make it clear the bank module is used for both requests.

From just looking at the endpoint POST /bank/accounts/{address} someone could think that you're creating a new bank account to tied to your {address} (?). What about POST /bank/accounts/{address}/transfers

@faboweb
Copy link
Contributor

faboweb commented Sep 25, 2018

return parameter in the request body with the desired output: return=<sync|async|block>

I would put them into the request URL and not into the body, so the data stays the same but the handling changes. Plus: Let's do that later.

POST /bank/accounts/{address}/transfers

👍

Agree, but lets change GET /bank/balances/{account} to use an /{address} instead

👍

@fedekunze
Copy link
Collaborator

Updated list of endpoints according to the most recent discussion

@jackzampolin
Copy link
Member Author

Update list.

@faboweb
Copy link
Contributor

faboweb commented Oct 12, 2018

Follow up of #2336

I propose that we don't bloat the API and remove:

  • GET /stake/delegators/{delegatorAddr}/redelegations/validator_from/{validatorSrcAddr}/validator_to/{validatorDstAddr}

I don't see a use case for this. I see a usecase making this more general purpose:

  • GET /stake/delegators/{delegatorAddr}/redelegations/{validatorSrcAddr}

@fedekunze
Copy link
Collaborator

fedekunze commented Oct 12, 2018

@faboweb I get your point and I think that would be actually more useful. I even thought initially of the same idea but with GET /stake/delegators/{delegatorAddr}/redelegations/{validatorAddr} (where validatorAddr could be either src or dst).

The thing is that that functionality is not implemented since there's no key for it. The one that's similar is getting the redelegations from a validatorSrcAddr, i.e something like the following:

GET /stake/validators/{validatorSrcAddr}/redelegations

Here's the permalink to the function:

// return all redelegations from a particular validator
func (k Keeper) GetRedelegationsFromValidator(ctx sdk.Context, valAddr sdk.ValAddress) (reds []types.Redelegation) {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, GetREDsFromValSrcIndexKey(valAddr))
defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
key := GetREDKeyFromValSrcIndexKey(iterator.Key())
value := store.Get(key)
red := types.MustUnmarshalRED(k.cdc, key, value)
reds = append(reds, red)
}
return reds
}

Am I right @cwgoes ?

@fedekunze
Copy link
Collaborator

I think this is done, @jackzampolin can we close this ?

@jackzampolin
Copy link
Member Author

@fedekunze Can you do the honor as this has been your baby!?

@fedekunze
Copy link
Collaborator

@fedekunze Can you do the honor as this has been your baby!?

😆 closing this ! 🎊

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this issue Mar 1, 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

7 participants