-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
R4R: Bianjie/lcd implementation #2147
Conversation
…to haoyang/lcd-implementation
Haoyang/lcd implementation new
// Which is much friendly for further development | ||
func ServeSwaggerCommand(cdc *wire.Codec) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "lite-server", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaoyangLiu Could you add the cmd to the Gaia-lite
section of clients.md docs ?
@@ -66,6 +67,7 @@ func main() { | |||
tendermintCmd, | |||
ibcCmd, | |||
lcd.ServeCommand(cdc), | |||
lcd.ServeSwaggerCommand(cdc), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add to Basecoin's basecli file as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK. Thanks so much for the work here @HaoyangLiu. There are a couple things left related to docs
and examples
only. Please address those before merging
docs/light/getting_started.md
Outdated
### Transfer a token | ||
When the connected full node is trusted, then the proof is not necessary, so you can run gaia-lite node with trust-node option: | ||
``` | ||
gaiacli lite-server --chain-id {your chain id} --trust-node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
docs/light/getting_started.md
Outdated
![transfer](pics/transfer-tokens.png) | ||
If you have want to run gaia-lite node in a remote server, then you must specify the public ip to swagger-host-ip | ||
``` | ||
gaiacli lite-server --chain-id {your chain id} --swagger-host-ip {remove server public ip} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gaiacli lite-server --chain-id=<chain_id> --swagger-host-ip=<remote_server_ip>
. Typo in remove ?
::: tip Note | ||
🚧 We are actively working on documentation for Gaia-lite. | ||
::: | ||
The Gaia-Lite is a light gaiad node. Unlike gaiad full node, it doesn't participate consensus and execute transactions, so it only require very little storage and calculation resource. However, it can provide the same security as a gaia full node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very concise and well written explanation about Gaia-lite, thanks !
docs/light/getting_started.md
Outdated
operation: [build transaction](api.md#create_transfer---post) | ||
The gaia-lite node can connect to multiple full nodes. Then gaia-lite will do load balancing for full nodes which is helpful to improve reliability and TPS. You can do this by this command: | ||
``` | ||
gaiacli lite-server --chain-id {your chain id} --node-list tcp://10.10.10.10:26657,tcp://20.20.20.20:26657 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gaiacli lite-server --chain-id=<chain_id> --node-list=tcp://10.10.10.10:26657,tcp://20.20.20.20:26657
. Although not sure if the node list needs to be in between " ". Will double check that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have double checked that. The node list doesn't need to be in between " "
docs/sdk/clients.md
Outdated
|
||
You can start a gaia-lite node with the following command: | ||
``` | ||
gaiacli lite-server --chain-id {your chain id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gaiacli lite-server --chain-id-<chain_id>
I have change the code according to your comment. Please check it. |
@HaoyangLiu Pulling this down today and will get started testing. Can you make sure all the routes and docs here match this issue: #2113 Looking forward to getting this merged! |
docs/light/getting_started.md
Outdated
|
||
Then sign the returned transaction byte array with users' private key. Finally broadcast the signed | ||
transaction. Refer to this link for how to broadcast the signed transaction: [broadcast transaction](api.md#create_transfer---post) | ||
The gaia-lite support modular rest APIs. Now it supports four modules: general, key, token and stake. If you need all of them, just start it with this command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gaia-lite is built in a modular format. Each Cosmos module defines it's own RPC API. Currently the following modules are supported: transaction, key, bank (token), staking, governance, and slashing.
Hi @jackzampolin, now ICS0, ICS22, ICS23 are still not supported. The following APIs in ICS1 are not supported too.
Do you expect full implementation of all APIs in #2113 in this PR? If so, I will continus to implement the unsupported APIs and append the code to this PR. |
@HaoyangLiu I expect full implementation of of |
@jackzampolin |
@jackzampolin
However, currently, the response of /keys (get) in gaiacli rest-server is like this:
I wonder if the wrapper is necessary. Because the wrapper may bring in some additional work for user who want to switch from "gaiacli rest-server" to "gaiacli lite-server" |
@HaoyangLiu Lets leave off the wrapper and update the documentation accordingly. You can add the docs update to this PR. |
So far, I have implemented all rest APIs in ICS0, ICS1, ICS20 and ICS21. As for ICS22 and ICS23, I will implement them in later PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big PR, so I left some initial comments on the client stuff first. Will continue reviewing 👍
Thanks for the work!
client/context/context.go
Outdated
@@ -10,6 +10,7 @@ import ( | |||
"github.com/spf13/viper" | |||
|
|||
rpcclient "github.com/tendermint/tendermint/rpc/client" | |||
tendermintLite"github.com/tendermint/tendermint/lite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting error
func (ctx CLIContext) WithClientManager(clientManager *ClientManager) CLIContext { | ||
ctx.ClientManager = clientManager | ||
return ctx | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline.
client/context/loadbalancing.go
Outdated
@@ -0,0 +1,46 @@ | |||
package context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this entire needs to be formatted correctly. Also, it should probably be renamed to client_manager.go
client/context/loadbalancing.go
Outdated
type ClientManager struct { | ||
clients []rpcclient.Client | ||
currentIndex int | ||
mutex sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll suffice to just have a write only mutex here (I don't see any logic that obtains a read-lock).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean use sync.Mutex instead?
client/context/loadbalancing.go
Outdated
mgr.currentIndex = 0 | ||
} | ||
return client | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new line.
client/context/query.go
Outdated
@@ -281,6 +305,50 @@ func (ctx CLIContext) ensureBroadcastTx(txBytes []byte) error { | |||
return nil | |||
} | |||
|
|||
// proofVerify perform response proof verification | |||
func (ctx CLIContext) proofVerify(path string, resp abci.ResponseQuery) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verifyProof
sounds better I think.
client/context/query.go
Outdated
} | ||
|
||
// Validate the substore commit hash against trusted appHash | ||
substoreCommitHash, err := store.VerifyMultiStoreCommitInfo(multiStoreProof.StoreName, multiStoreProof.CommitIDList, commit.Header.AppHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can break this call into multiple lines for legibility.
client/context/query.go
Outdated
@@ -300,10 +368,15 @@ func (ctx CLIContext) query(path string, key cmn.HexBytes) (res []byte, err erro | |||
} | |||
|
|||
resp := result.Response | |||
if !resp.IsOK() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is resp.IsOK()
invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Maybe it is a mistake in code merging. I have changed it to resp.IsOK which seems more elegant
client/context/query.go
Outdated
// proofVerify perform response proof verification | ||
func (ctx CLIContext) proofVerify(path string, resp abci.ResponseQuery) error { | ||
// Data from trusted node or subspace query doesn't need verification | ||
if ctx.TrustNode || !isQueryStoreWithProof(path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the !isQueryStoreWithProof(path)
should be moved outside to the caller. This provides a better abstraction.
client/context/query.go
Outdated
if len(paths) != 3 { | ||
return false | ||
} | ||
// WARNING This should be consistent with query method in iavlstore.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on this warning and why we need it? What needs to be done about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In iavlstore.go, proof will be included in response only when the query path is /store or /key. I concern that this may be changed in iavlstore.go. If there are some changes about proof building in iavlstore.go, then we must change code here to be consistent with the changes.
Please move to #2177. |
Closing in favor of #2177 (as intended @HaoyangLiu?) |
proceed from: #2118