[R4R]: add swagger-ui for gaiacli lite-server #2215

HaoyangLiu · 2018-09-02T12:51:47Z

This is another prototype of swagger-ui implementation for gaia-lite server. Comparing with previous one #2199, this implementation brings in much less code change and easy to maintain.

The following files under client/lcd/swaggerui are copied from the swagger-api

favicon-16x16.png
favicon-32x32.png
index.html
oauth2-redirect.html
swagger-ui-bundle.js
swagger-ui-bundle.js.map
swagger-ui.css
swagger-ui.css.map
swagger-ui.js
swagger-ui.js.map
swagger-ui-standalone-preset.js
swagger-ui-standalone-preset.js.map

Besides, client/lcd/statik/statik.go is generated by the tool statik.

If we want to update API docs, we just need to follow the steps edit client/lcd/swaggerui/swagger.json.
Run the following command to start gaia-lite:

gaiacli lite-server --chain-id=<chain-id>

Then you can visit this url to check your modified API docs:

http://localhost:1317/swaggerui/

@jackzampolin @fedekunze
Do you think this is acceptable? If not please give me some suggestions. Thanks very much.

…date API docs

fedekunze · 2018-09-02T13:08:13Z

client/keys/add.go

+}
+
+// RecoverResuestHandler is the handler of creating seed in swagger rest server
+func RecoverResuestHandler(w http.ResponseWriter, r *http.Request) {


Not sure if you're refering to Rest or Request with Resuest, please change it accordingly

fedekunze · 2018-09-02T13:09:23Z

client/keys/add.go

+	decoder := json.NewDecoder(r.Body)
+	err := decoder.Decode(&m)
+	if err != nil {
+		w.WriteHeader(400)


use http library with the corresponding status codes constants

fedekunze · 2018-09-02T13:11:08Z

client/keys/add.go

+	name := vars["name"]
+	var m RecoverKeyBody
+
+	decoder := json.NewDecoder(r.Body)


use wire Codec to decode the body

fedekunze · 2018-09-02T13:12:47Z

client/lcd/README.md

+```
+go get github.com/rakyll/statik
+```
+* Directly Edit API docs: client/lcd/swaggerui/swagger.json


Add link to file from develop branch

fedekunze · 2018-09-02T13:13:56Z

client/lcd/README.md

+```
+* Directly Edit API docs: client/lcd/swaggerui/swagger.json
+
+* Edit API docs within this [website](https://app.swaggerhub.com). Please refer to this [link](https://app.swaggerhub.com/help/index) for how to use the about website to edit API docs.


within ~~this [website]~~ [SwaggerHub]

fedekunze · 2018-09-02T13:15:20Z

client/lcd/README.md

+
+* Edit API docs within this [website](https://app.swaggerhub.com). Please refer to this [link](https://app.swaggerhub.com/help/index) for how to use the about website to edit API docs.
+
+* Download swagger.json and replace the old swagger.json under client/lcd/swaggerui folds


Download swagger.json and replace the old swagger.json under client/lcd/swaggerui folder

fedekunze · 2018-09-02T13:15:36Z

client/lcd/README.md

+
+* Download swagger.json and replace the old swagger.json under client/lcd/swaggerui folds
+
+* Regenerate statik.go file


Generate a new statik.go file with the following command:

fedekunze · 2018-09-02T13:18:38Z

client/lcd/lcd_test.go

-	// TODO Do we really need this endpoint?
-	res, body := Request(t, port, "GET", "/keys/seed", nil)
+	// recover key
+	recoverKeyURL := fmt.Sprintf("/keys/%s/recover", "test_recover")


Can you move /keys/{name}/recover logic to a separate function ? Same way as getAccount, doSend, etc. are implemented

fedekunze · 2018-09-02T13:20:48Z

cmd/gaia/cmd/gaiacli/main.go

@@ -18,6 +18,7 @@ import (
 	slashingcmd "github.com/cosmos/cosmos-sdk/x/slashing/client/cli"
 	stakecmd "github.com/cosmos/cosmos-sdk/x/stake/client/cli"

+	_ "github.com/cosmos/cosmos-sdk/client/lcd/statik"


I'm curious why is this being imported if it's not used ?

This import will trgger side effects: call the init function of the package. We can also call the init function explicitly.

Let's call it explicitly, makes the code more obvious

I'm afraid that I can't do that.

func init() { }

The init function is private, so I can't call it explicitly from outside. Besides, this file is generated by statik, so we should avoid to edit it maunally.

fedekunze · 2018-09-02T13:24:36Z

client/lcd/README.md

+* Regenerate statik.go file
+```
+statik -src=client/lcd/swaggerui -dest=client/lcd
+```


What happens after the new statik.go file is generated ? Just run gaiacli lite-server and it's done ? Maybe add that section here as well

(My question is mainly how do I see the generated docs afterwards)

You can start lite-server with this command:

gaiacli lite-server --chain-id=<chain_id>

Then visit the following url with your explorer:

http://localhost:1317/swaggerui/

fedekunze · 2018-09-02T13:26:48Z

client/lcd/README.md

+
+## Steps
+
+* Install the command line tool first.


replace * for numbers (e.g 1. , 2. , ... ) since they are steps

fedekunze

Overall looks good, I left a couple of comments. Also please make sure to update PENDING.md since this are breaking changes (specially endpoint renaming).

Also please update Gaia-lite section of the docs with a link to how to update the API (in this case it would be something like https://cosmos.network/docs/sdk/client/lcd/README.html). I remember you already made some comments in that section in one of your other PRs, maybe merge both of them within the Gaia-lite section ?

fedekunze · 2018-09-02T13:38:13Z

@HaoyangLiu client/lcd/README.md is not within the docs folder, can you add them there as well ?

fedekunze · 2018-09-02T13:39:13Z

also Golint is failing in CircleCI, make sure that's fixed as well. Thanks !

codecov · 2018-09-02T13:42:30Z

Codecov Report

Merging #2215 into develop will decrease coverage by 0.44%.
The diff coverage is 4%.

@@            Coverage Diff             @@
##           develop   #2215      +/-   ##
==========================================
- Coverage    61.15%   60.7%   -0.45%     
==========================================
  Files          123     125       +2     
  Lines         7391    7569     +178     
==========================================
+ Hits          4520    4595      +75     
- Misses        2558    2660     +102     
- Partials       313     314       +1

…hardcode http status

fedekunze · 2018-09-02T22:24:17Z

client/keys/add.go

-// add new key REST handler
+func paramCheck(m NewKeyBody, kb keys.Keybase) (int, string) {
+	if m.Name == "" {
+		return http.StatusBadRequest, fmt.Sprintf("You have to specify a name for the locally stored account.")


no need for fmt.Sprintf here

Just want to keep the code style consistent. Removing it looks better.

fedekunze · 2018-09-02T22:24:32Z

client/keys/add.go

+		return http.StatusBadRequest, fmt.Sprintf("You have to specify a name for the locally stored account.")
+	}
+	if m.Password == "" {
+		return http.StatusBadRequest, fmt.Sprintf("You have to specify a password for the locally stored account.")


HaoyangLiu · 2018-09-03T02:21:46Z

@jackzampolin @cwgoes
Here is prototype of gaia-lite. I have some questions:

If gaia-lite is done. Do we need to keep subcommand rest-server.
I noticed that almost all APIs of gaia-lite are also included in rest-server. So how about merge them? Here I just renamed rest-server to lite-server and added swagger-ui.
Now the API docs locates in client/lcd/swaggerui/swagger.json. Based on this json file, statik can generate client/lcd/statik/statik.go. So once client/lcd/statik/statik.go is generated, the supported modules and swagger host IP are solidified. This will result in some problems:
1. We can't implement modular APIs
2. If gaia-lite is running on remote server or --laddr option is assigned to other value, the swagger-ui will not work well. Because swagger-ui only send http request to localhost:1317

@fedekunze
Do you have some suggestion about the above two problems?

…ing in indent to make response more readable

zramsay · 2018-10-02T15:19:54Z

if there's a simple build process to generate to bunch of files required to serve the RPC docs, the website Makefile can handle this (like it does for the regular docs, it's basically a cp then the build commands). would be good to have some instructions about building & serving the RPC docs added to the docs/DOCS_READM.md

zramsay

requires instructions for building/serving the RPC docs added to docs/DOCS_README.md

zramsay · 2018-10-02T15:22:15Z

docs/light/update_API_docs.md

@@ -0,0 +1,18 @@
+# How to update API docs


let's move this whole file to a section in docs/DOCS_README.md

zramsay · 2018-10-02T15:23:44Z

docs/light/update_API_docs.md

+
+1. Install the swagger-ui generate tool first.
+    ```
+    make get_tools


which Makefile is this meant to target? should specify that this command is to be run from ... ? (root of repo i think)

HaoyangLiu · 2018-10-02T15:57:35Z

@zramsay
The API docs are generated by during building gaiad and gaiacli. When you execute make install or make build, the make target update_gaia_lite_docs will be executed too. It will generate a golang file : client/lcd/statik/statik.go which contains the API docs. So the docs building process is transient.
Besides, I have move the update_API_docs.md to docs/DOCS_README.md

HaoyangLiu · 2018-10-02T16:00:39Z

@cwgoes
The JS files are static and they won't be changed later. As for the map files, do you think I should unminify them? I'm sorry I failed to get your point here.

cwgoes · 2018-10-02T18:09:33Z

@HaoyangLiu The JS files are OK, it's not a big deal. Only remaining issue is investigating why localnet CI is failing - looks like something in the static file handling package?

HaoyangLiu · 2018-10-02T23:38:30Z

@cwgoes
I have investigated the issue, please refer to #2331
The docker image circleci/golang:1.10.3 actually has golang1.9 installed. However, the swagger-ui tool statik requires golang version large than 1.10.

HaoyangLiu · 2018-10-02T23:52:07Z

@cwgoes
Due to some change in gaiacli config, my previous change in creating certifier doesn't work well. For getting status command, viper.IsSet(client.FlagTrustNode) also returns true though client.FlagTrustNode is not predefined.
So I removed the change and add another change in get status

cwgoes · 2018-10-03T16:52:09Z

Due to some change in gaiacli config, my previous change in creating certifier doesn't work well. For getting status command, viper.IsSet(client.FlagTrustNode) also returns true though client.FlagTrustNode is not predefined.
So I removed the change and add another change in get status

We've temporarily disabled light client verification in the LCD tests due to an upstream Tendermint issue, so I think you don't need to also do it in getStatus.

This needs to be rebased against develop since the TM 0.24 PR was merged, then we should be good to merge it.

HaoyangLiu · 2018-10-04T00:22:06Z

@cwgoes
Currently the gaiacli status command can't be executed. Please check my execution log:

lhy@lhy-ubuntu:~$ gaiacli status
Must specify these options: --chain-id  when --trust-node is false
lhy@lhy-ubuntu:~$ gaiacli status --chain-id test-chain-dNvAWi
ERROR: unknown flag: --chain-id

Now when executing get status, it will create certifier which require --chain-id. However, certifier should not be created in this command. So I added viper.Set(client.FlagTrustNode, true):

func printNodeStatus(cmd *cobra.Command, args []string) error {
	// No need to verify proof in getting node status
	viper.Set(client.FlagTrustNode, true)
	cliCtx := context.NewCLIContext()
	status, err := getNodeStatus(cliCtx)
	if err != nil {
		return err
	}

jackzampolin · 2018-10-04T01:04:55Z

Excited to see this merged!

cwgoes · 2018-10-04T06:37:24Z

Now when executing get status, it will create certifier which require --chain-id. However, certifier should not be created in this command. So I added viper.Set(client.FlagTrustNode, true)

I see, OK - instead, let's use cliContext.WithTrustNode, viper.Set is a bit of an anti-pattern (hard to track global variable changes).

I made a PR - https://github.com/HaoyangLiu/cosmos-sdk/pull/8.

Minor cleanup

HaoyangLiu · 2018-10-04T10:55:01Z

@cwgoes
the golang version of docker image circleci/golang:1.11.1 is 1.9 too which is very strange.

go version go1.9.1 linux/amd64

Please check the CI failure: https://circleci.com/gh/cosmos/cosmos-sdk/34860#config/containers/0

cwgoes · 2018-10-04T10:59:09Z

the golang version of docker image circleci/golang:1.11.1 is 1.9 too which is very strange.

That's strange. I'll try another image in a separate PR (easier to test with PRs against this repo).

alessio · 2018-10-15T19:48:10Z

client/utils/rest.go

@@ -1,5 +1,6 @@
 package utils

+import "C"


This seems to have broken cross compilation between Darwin and Linux. Can we safely remove this line or is there any reason why we shouldn't do so?

If we want this cross-compilation ability can we test for it in CI? Then we'll know in the future if we break it.

I'd second the idea, though we have to figure out how go about it - golang/go#19470

It was introduced in #2215, it's presently unnecessary and it is clearly breaking cross compilation.

HaoyangLiu added 2 commits September 2, 2018 20:09

Add swagger-ui to gaiacli rest-server

79c0bc4

Rename rest-server to lite-server and add README to clarify how to up…

8e032f8

…date API docs

HaoyangLiu requested review from cwgoes, ebuchman and rigelrozanski as code owners September 2, 2018 12:51

fedekunze reviewed Sep 2, 2018

View reviewed changes

fedekunze suggested changes Sep 2, 2018

View reviewed changes

Refactor code according to code review

9a1dac2

HaoyangLiu added 2 commits September 2, 2018 22:00

Refactor code for key management: use cdc instead of json and remove …

e742b02

…hardcode http status

Update pending and skip statik in test_lint

e496bbd

HaoyangLiu requested a review from zramsay as a code owner September 2, 2018 14:30

Reset makefile

6ecec39

HaoyangLiu mentioned this pull request Sep 2, 2018

WIP: Split LCD implementation PR, part two #2199

Closed

fedekunze reviewed Sep 2, 2018

View reviewed changes

remove useless fmt.Sprintf and add paramCheck to recover key

093c4ad

Add post respones process to add json to header and decide whether br…

5d259b1

…ing in indent to make response more readable

zramsay suggested changes Oct 2, 2018

View reviewed changes

zramsay reviewed Oct 2, 2018

View reviewed changes

HaoyangLiu added 2 commits October 2, 2018 23:38

Add indent option to context and fix a bug in node status command

9830acc

Move update_API_docs.md to docs/DOCS_README.md

e1bb6fc

zramsay approved these changes Oct 2, 2018

View reviewed changes

Merge with develop

1d4cfe8

WithTrustNode, Makefile fixes, 'make format'

c676eab

cwgoes and others added 4 commits October 4, 2018 08:39

Update CircleCI Golang container

bf3e276

Revert viper.Set change

e72c19a

Merge pull request #8 from cosmos/cwgoes/patch-fix

d46b562

Minor cleanup

Refactor code in set response header

31b26e7

cwgoes approved these changes Oct 4, 2018

View reviewed changes

cwgoes merged commit 9f67e8a into cosmos:develop Oct 4, 2018

alessio reviewed Oct 15, 2018

View reviewed changes

alessio pushed a commit that referenced this pull request Oct 17, 2018

Remove 'import "C"' from client/utils package

7211613

It was introduced in #2215, it's presently unnecessary and it is clearly breaking cross compilation.

alessio mentioned this pull request Oct 17, 2018

Remove 'import "C"' from client/utils package #2523

Merged

5 tasks


		* Edit API docs within this [website](https://app.swaggerhub.com). Please refer to this [link](https://app.swaggerhub.com/help/index) for how to use the about website to edit API docs.

		* Download swagger.json and replace the old swagger.json under client/lcd/swaggerui folds


		* Download swagger.json and replace the old swagger.json under client/lcd/swaggerui folds

		* Regenerate statik.go file

[R4R]: add swagger-ui for gaiacli lite-server #2215

[R4R]: add swagger-ui for gaiacli lite-server #2215

Conversation

HaoyangLiu commented Sep 2, 2018 • edited Loading

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

fedekunze Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze Sep 2, 2018 • edited Loading

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

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze commented Sep 2, 2018

fedekunze commented Sep 2, 2018

codecov bot commented Sep 2, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoyangLiu commented Sep 3, 2018 • edited Loading

zramsay commented Oct 2, 2018

zramsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoyangLiu commented Oct 2, 2018

HaoyangLiu commented Oct 2, 2018

cwgoes commented Oct 2, 2018

HaoyangLiu commented Oct 2, 2018 • edited Loading

HaoyangLiu commented Oct 2, 2018

cwgoes commented Oct 3, 2018

HaoyangLiu commented Oct 4, 2018

jackzampolin commented Oct 4, 2018

cwgoes commented Oct 4, 2018

HaoyangLiu commented Oct 4, 2018

cwgoes commented Oct 4, 2018

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

HaoyangLiu commented Sep 2, 2018 •

edited

Loading

fedekunze Sep 2, 2018 •

edited

Loading

fedekunze Sep 2, 2018 •

edited

Loading

codecov bot commented Sep 2, 2018 •

edited

Loading

HaoyangLiu commented Sep 3, 2018 •

edited

Loading

HaoyangLiu commented Oct 2, 2018 •

edited

Loading