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

WIP: REST server docs #1139

Closed
wants to merge 3 commits into from
Closed

WIP: REST server docs #1139

wants to merge 3 commits into from

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Jun 5, 2018

No description provided.

@mossid mossid requested a review from ebuchman as a code owner June 5, 2018 04:57
@codecov
Copy link

codecov bot commented Jun 5, 2018

Codecov Report

Merging #1139 into develop will decrease coverage by 0.21%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #1139      +/-   ##
===========================================
- Coverage    60.87%   60.65%   -0.22%     
===========================================
  Files           89       89              
  Lines         4368     4369       +1     
===========================================
- Hits          2659     2650       -9     
- Misses        1539     1547       +8     
- Partials       170      172       +2

client/README.md Outdated

```bash
> go get -u github.com/cosmos/cosmos-sdk
package github.com/cosmos/cosmos-sdk: no Go files in /home/mossid/go/src/github.com/cosmos/cosmos-sdk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line an error you encountered? Why is it on the tuto?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client/README.md Outdated
package github.com/cosmos/cosmos-sdk: no Go files in /home/mossid/go/src/github.com/cosmos/cosmos-sdk
> cd $GOPATH/src/github.com/cosmos/cosmos-sdk
> git checkout develop
Branch develop set up to track remote branch develop from origin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove console output from tuto

client/README.md Outdated
Switched to a new branch 'develop'
> dep ensure
> make install
go install -ldflags "-X github.com/cosmos/cosmos-sdk/version.GitCommit=be7ec5b" ./cmd/gaia/cmd/gaiad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a command to run? Or is this a log? If latter, remove

client/README.md Outdated
### Run node and server

```bash
# If your are going to connect to an external node(e.g. testnet), skip gaiad init and gaiad start
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this out of bash window, put it above. Rephrase to If your are going to connect to an external node(e.g. testnet), skip this part

### Test getting account

```bash
> cat $HOME/.gaiad/config/genesis.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unclear. Can you clarify the different steps, and comment? Maybe use the GET method /balance from #1092 as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test getting account section is for testing the lcd is working correctly. To do this, we first get balance from genesis file(as the "expected' value) and compare it with lcd query result(as the "actual" value). Interchain standard describes the lcd endpoints, which could be wrong if the lcd is malfunctioning, so we cannot use it as the expected value I think?

```

## Notable Flags

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add trust-node flag


* `--chain-id <STRING>`: chain id of the full node to connect
* `--node <URL>`: address of the full node to connect
* `--laddr <URL>`: address to run the rest server on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this flag. Don't you specify the address of the rest server when you ssh into it. To me the steps are:

  1. SSH into the serv
  2. Install
  3. Run the Rest server

In this case, why would you need a flag to specify the address of the server you're already ssh'd in? Maybe a flag for the port would be useful, but why specify the address?

Copy link
Contributor Author

@mossid mossid Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--laddr is mainly used for specifying tcp server port. I haven't seen any use case using it for specifying the address. Simply put localhost as the address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why not change it to --lport?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tendermint codebase and the other part of the sdk codebase uses --laddr so we are using the same one for compatibility.

@gamarin2 gamarin2 mentioned this pull request Jun 19, 2018
6 tasks
@adrianbrink
Copy link
Contributor

I spoke to @mossid today and we decided to close this one in favour of #1314 to avoid confusion around the LCD.

@ValarDragon ValarDragon deleted the joon/rest-docs branch September 23, 2018 04:02
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

Successfully merging this pull request may close these issues.

5 participants