LCD and API Specification / Documentation #1617

adrianbrink · 2018-07-10T11:50:53Z

Supercedes #1314 . All past discussion happened on that branch.

This branch squashes all commits into a single commit without force-pushing over public branches.

ebuchman

Just some initial review. Still going through it. Don't think it belongs in the spec folder. We can merge and mark it as a WIP and move it to the docs - will keep reviewing from there

ebuchman · 2018-07-10T17:06:51Z

docs/spec/light/api.md

+
+url: /broadcast_tx_sync
+
+Functionality: Submit a signed transaction and wait for it to be committed.


doesn't wait for it to be committed. that's broadcast_tx_commit. Should we have all three endpoints here? We certainly shouldn't use the same terms to refer to different things ...

ebuchman · 2018-07-10T17:07:35Z

docs/spec/light/api.md

+
+Functionality: Submit a signed transaction and wait for it to be committed.
+
+Parameters:


these dont seem to be rendering properly in https://github.com/cosmos/cosmos-sdk/blob/adrian/lcd_spec_final/docs/spec/light/api.md#broadcast_tx_sync---post

This is the case for all these params tables ..

ebuchman · 2018-07-10T17:08:25Z

docs/spec/light/api.md

+    "code":200,
+    "error": "",
+    "result": {
+        "address":BD607C37147656A507A5A521AA9446EB72B2C907


I think the address is missing quotes

ebuchman · 2018-07-10T17:08:39Z

docs/spec/light/api.md

+    "code":200,
+    "error": "",
+    "result": {
+        "seed":crime carpet recycle erase simple prepare moral dentist fee cause pitch trigger when velvet animal abandon


missing quotes ?

ebuchman · 2018-07-10T17:09:55Z

docs/spec/light/api.md

+    "code":200,
+    "error": "",
+    "result": {
+        "atom": 1000,


Amino v0.10 follows proto3 where integers are quoted as strings

Does this apply to all integers? Will the error codes change as well?

I think we should stick with integers here though, since it seems to be the standard for json. This API doesn't necessarily have to comply with amino encoding.

ebuchman · 2018-07-10T17:10:09Z

docs/spec/light/api.md

+    "code":200,
+    "error": "",
+    "result": {
+     "transaction": "TODO:<JSON sign bytes for the transaction>"


ebuchman · 2018-07-10T17:11:19Z

docs/spec/light/getting_started.md

+
+![deposit](https://github.com/irisnet/cosmos-sdk/raw/bianjie/lcd_spec/docs/spec/lcd/pics/create-account.png)
+
+First you need to get a new seed phrase :[get-seed](https://github.com/irisnet/cosmos-sdk/blob/bianjie/lcd_spec/docs/spec/lcd/api.md#keysseed---get)


these links dont seem to be working

ebuchman · 2018-07-10T18:21:55Z

docs/spec/light/load_balancer.md

@@ -0,0 +1,203 @@
+# Load Balancing Module
+
+The LCD will be an important bridge between service providers and cosmos blockchain network. Suppose


Not sure about this section. I don't think we want to build a load balancer in to the LCD.

More likely we need to work on streaming events out of Tendermint and into something like Postgres and load-balancing the access to postgres.

I think it could be useful that a single light-client distributes its requests over many full-nodes. That's what this section is mostly describing. I am not sure whether we want Gaia Light to include postgres (or some other database) as a dependency.

ebuchman · 2018-07-10T18:23:10Z

docs/spec/light/overview.md

@@ -0,0 +1,55 @@
+# Overview


isnt this just a copy of what's already in the readme?

I've removed the overview.md

ebuchman · 2018-07-10T18:24:15Z

docs/spec/light/api.md

+Error messages my change and should be only used for display purposes. Error messages should not be
+used for determining the error type.
+
+## ICS0 - TendermintAPI - not yet implemented


Do we want other endpoints to be part of this ? /status, /net_info, etc. ?

Yes, eventually we'll add those in as well.

* Move from specs to docs folder * Attempt to fix markdown links * Address general comments from the PR * Remove overview.md due to duplication with readme.md

codecov · 2018-07-11T11:51:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e984b73). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1617   +/-   ##
=========================================
  Coverage          ?   63.71%           
=========================================
  Files             ?      122           
  Lines             ?     6774           
  Branches          ?        0           
=========================================
  Hits              ?     4316           
  Misses            ?     2211           
  Partials          ?      247

ebuchman

Really great start. I think we should move it to lite though to be consistent with Tendermint

cwgoes

Agreed on switching from light to lite.

Left several comments, nothing that needs to block this PR unless you want to make changes, but I don't think we should consider this API finalized yet (mostly because of HD keys).

cwgoes · 2018-07-18T17:52:31Z

docs/light/api.md

+}
+```
+
+### /broadcast_tx_commit - POST


We'll need either simulation options for these endpoints or a separate simulation endpoint, see #1246 (not yet implemented).

Doesn't need to block this PR.

cwgoes · 2018-07-18T17:56:39Z

docs/light/load_balancer.md

@@ -0,0 +1,203 @@
+# Load Balancing Module - WIP


While we can include this, I'm not convinced client-side load balancing is all that useful.

It doesn't protect servers from malicious or misconfigured clients, and can't ever balance that effectively since it has no information about what requests other clients are making to the same full nodes. I think implementing a way for full nodes to provide a single endpoint (fullnode.myvalidator.com) which balances to multiple Tendermint/SDK instances, fronted by a cache layer (don't need to write this, e.g. Varnish) will be more likely to work.

What we probably do want is client-side retry logic - switch to a different full node if one is down.

cwgoes · 2018-07-18T17:58:21Z

docs/light/readme.md

+| Storage resource|Huge|Little|Full node will save all blocks and ABCI states. LCD just saves validator sets and some checkpoints|
+| Power consume|Huge|Little|Full nodes have to be deployed on machines which have high performance and will be running all the time. So power consume will be huge. LCD can be deployed on the same machines as users' applications, or on independent machines but with poor performance. Besides, LCD can be shutdown anytime when necessary. So LCD only consume very little power, even mobile devices can meet the power requirement|
+| Provide APIs|All cosmos APIs|Modular APIs|Full node supports all cosmos APIs. LCD provides modular APIs according to users' configuration|
+| Secuity level| High|High|Full node will verify all transactions and blocks by itself. LCD can't do this, but it can query any data from other full nodes and verify the data independently. So both full node and LCD don't need to trust any third nodes, they all can achieve high security|


Worth mentioning weak subjectivity here. I would rate LCD security "Medium" personally (but note that it's way, way higher than e.g. Metamask).

cwgoes · 2018-07-18T17:59:59Z

docs/light/api.md

+}
+```
+
+### /keys/recover - POST


This doesn't seem to reflect HD derivation?

cwgoes · 2018-07-18T18:00:33Z

docs/light/api.md

+| Parameter | Type   | Default | Required | Description      |
+| --------- | ------ | ------- | -------- | ---------------- |
+| name      | string | null    | true     | name of key      |
+| password  | string | null    | true     | password of key  |


Should add a way to specify an algorithm (ed25519/secp256k1).

cwgoes · 2018-07-18T18:01:33Z

docs/light/api.md

+}
+```
+
+## ICS1 - KeyAPI


This API needs to be refactored quite a bit for HD keys, which we probably want to encourage use of (do we want to support anything other than HD keys?)

Also, we support using the Ledger through the LCD API if you want to include that.

cwgoes · 2018-07-18T18:02:26Z

docs/light/api.md

+    "code":200,
+    "error":"",
+    "result": {
+        "atom":1000,


This isn't how Amino serializes numbers now, they're strings instead.

If we want the API to return numbers, we need to check that we won't lose precision.

cwgoes · 2018-07-18T18:03:05Z

docs/light/getting_started.md

+| chain-id    | string    | null                    | true     | chain id of the full node to connect                 |
+| node        | URL       | "tcp://localhost:46657" | true     | address of the full node to connect                  |
+| laddr       | URL       | "tcp://localhost:1317"  | true     | address to run the rest server on                    |
+| trust-node  | bool      | "false"                 | true     | Whether this LCD is connected to a trusted full node |


What does this flag do? Enable/disable proof verification?

cwgoes · 2018-07-18T18:05:42Z

docs/light/readme.md

+
+![validator-set-change](pics/validatorSetChange.png)
+
+### Trust propagation


Discuss bisection update algorithm here? Also related to slashing for Byzantine signatures even by unbonded validators (informal discussion with @sunnya97 and @jaekwon, not sure if it was written up anywhere).

Or that could go in a separate "Gaia lite formal spec" document.

ebuchman · 2018-07-19T15:45:23Z

Merged - follow up in #1758

Thanks for the review @cwgoes - noted we should follow up on it from #1758

Finalise LCD spec

9cdbe49

adrianbrink requested review from jbibla and zramsay as code owners July 10, 2018 11:50

adrianbrink added the ready-for-review label Jul 10, 2018

ebuchman reviewed Jul 10, 2018

View reviewed changes

ebuchman changed the base branch from develop to master July 11, 2018 00:29

Address comments

42c2712

* Move from specs to docs folder * Attempt to fix markdown links * Address general comments from the PR * Remove overview.md due to duplication with readme.md

ebuchman changed the base branch from master to develop July 16, 2018 15:53

ebuchman approved these changes Jul 18, 2018

View reviewed changes

cwgoes reviewed Jul 18, 2018

View reviewed changes

cwgoes mentioned this pull request Jul 18, 2018

LCD and API Specification / Documentation #1314

Closed

2 tasks

ebuchman merged commit 27840ab into develop Jul 19, 2018

ebuchman mentioned this pull request Jul 19, 2018

Rename LCD/ gaia-light ––> Gaia-lite #1758

Closed

ValarDragon deleted the adrian/lcd_spec_final branch September 23, 2018 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LCD and API Specification / Documentation #1617

LCD and API Specification / Documentation #1617

adrianbrink commented Jul 10, 2018

ebuchman left a comment

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

adrianbrink Jul 11, 2018

adrianbrink Jul 11, 2018

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

ebuchman Jul 10, 2018

adrianbrink Jul 11, 2018

ebuchman Jul 10, 2018

adrianbrink Jul 11, 2018

ebuchman Jul 10, 2018

adrianbrink Jul 11, 2018

codecov bot commented Jul 11, 2018 •

edited

Loading

ebuchman left a comment

cwgoes left a comment

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

cwgoes Jul 18, 2018

ebuchman commented Jul 19, 2018


		url: /broadcast_tx_sync

		Functionality: Submit a signed transaction and wait for it to be committed.


		Functionality: Submit a signed transaction and wait for it to be committed.

		Parameters:


		![deposit](https://github.com/irisnet/cosmos-sdk/raw/bianjie/lcd_spec/docs/spec/lcd/pics/create-account.png)

		First you need to get a new seed phrase :[get-seed](https://github.com/irisnet/cosmos-sdk/blob/bianjie/lcd_spec/docs/spec/lcd/api.md#keysseed---get)

		@@ -0,0 +1,203 @@
		# Load Balancing Module

		The LCD will be an important bridge between service providers and cosmos blockchain network. Suppose


		![validator-set-change](pics/validatorSetChange.png)

		### Trust propagation

LCD and API Specification / Documentation #1617

LCD and API Specification / Documentation #1617

Conversation

adrianbrink commented Jul 10, 2018

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2018 • edited Loading

Codecov Report

ebuchman left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Jul 19, 2018

codecov bot commented Jul 11, 2018 •

edited

Loading