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.

Sign up for GitHub

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

Jump to bottom

Flesh out gRPC guides, consolidate installation docs #245

Merged
merged 5 commits into from
Aug 15, 2017
Merged

Flesh out gRPC guides, consolidate installation docs #245

merged 5 commits into from
Aug 15, 2017

Conversation

MaxFangX
Copy link
Contributor

@MaxFangX MaxFangX commented Aug 3, 2017 •
edited
Loading

Python and Javascript gRPC guides

  • Now serve as complete standalone guides programmatically pulled into dev.lightning.community
  • Contains code examples for Simple, Response-streaming, and Bidirectional RPCs

Consolidate installation docs

Remove characters from the docker guide that break utf-8 encoding (you have to decode it with latin-1 codec which is too hacky to implement as special logic in the dev site rendering code

@MaxFangX MaxFangX changed the title Update Python and gRPC guides Update gRPC guides, consolidate installation docs Aug 3, 2017
@MaxFangX MaxFangX changed the title Update gRPC guides, consolidate installation docs Flesh out gRPC guides, consolidate installation docs Aug 3, 2017
bryanvu
bryanvu suggested changes Aug 9, 2017
View reviewed changes
docs/INSTALL.md Outdated

At this point, you should set your `$GOPATH` environment variable, which
represents the path to your workspace. By default, `$GOPATH` is set to
`~/go`. Be sure to set your `$GOPATH` every time you open a new terminal
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this recommend that you set the $GOPATH in .profile or .bashrc rather than setting it every time you open a new window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update in tutorial as well.

docs/INSTALL.md Outdated
`~/go`. Be sure to set your `$GOPATH` every time you open a new terminal
window.
```
export GOPATH=~/projects/lightning
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 our recommended GOPATH? Why wouldn't we just go with the default? It's not really clear whether it's necessary to use ~/projects/lightning or if this is just a semi-random suggestion.

Copy link
Contributor Author

@MaxFangX MaxFangX Aug 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I figured that people may want to keep their Go workspaces separate. Also, the tutorial is consistent with this in that it also installs everything in /projects/lightning. In any case, I implemented your above suggestion of recommending they put this step (as well as the $GOPATH/bin step below) into their .bashrc so I don't think it's an issue.

docs/INSTALL.md Outdated
export PATH=$PATH:$GOPATH/bin
```
This will ensure that your shell will be able to detect the binaries that
were just installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be "will be installed" rather than "were just installed."

docs/INSTALL.md Outdated
start both `btcd` and `lnd` in the `simnet` mode. Simnet is similar to regtest
in that you'll be able to instantly mine blocks as needed to test `lnd`
locally. In order to start either daemon in the `simnet` mode use `simnet`
instead of `testnet`, such as adding the `--bitcoin.simnet` flag instead of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need the "such as"

docs/grpc/javascript.md Outdated
@@ -18,35 +18,34 @@ at least somewhere reachable by your Javascript code).
The `rpc.proto` file is [located in the `lnrpc` directory of the `lnd`
sources](https://github.com/lightningnetwork/lnd/blob/master/lnrpc/rpc.proto).

In order to allow the auto code generated to compile the protos succucsfully,
In order to allow the auto code generated to compile the protos successfully,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should read "In order for the auto-generated code to compile successfully,"

docs/grpc/javascript.md Outdated
```

You can run the following in your shell or put it in a program and run it like
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably note that this requires that the user have a channel open or a route available between themselves and the target node.

docs/grpc/javascript.md Outdated
console.log("Current status" + status);
});
```
Now, create an invoice for your node at `localhost:10009` and send a payment to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should include some hints like "create an invoice using 'lncli addinvoice'" and send a payment from a remote node using "lncli sendpayment". This should also probably note that you must have set up two nodes with an open channel prior to running this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion - updated for Python guide as well.

docs/grpc/python.md Outdated

import grpc
Everytime you use Python gRPC, you will have to import the generated rpc modeuls
Copy link
Contributor

Choose a reason for hiding this comment

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

modeuls -> modules

docs/grpc/python.md Outdated
```

#### Response-streaming RPC
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention the same prerequisites as for the JS versions.

@bryanvu
Copy link
Contributor

bryanvu commented Aug 9, 2017

Nice work Max! Just added a few minor comments. Let me know if you have any questions.

@MaxFangX
Copy link
Contributor Author

MaxFangX commented Aug 9, 2017

Changes implemented, rebased, and ready for merge

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 73.844% when pulling 505db494710e7f78c581b524bcb959b98417f2c8 on MaxFangX:update-guides into 5c89ec6 on lightningnetwork:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 73.857% when pulling 505db494710e7f78c581b524bcb959b98417f2c8 on MaxFangX:update-guides into 5c89ec6 on lightningnetwork:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 73.858% when pulling 505db494710e7f78c581b524bcb959b98417f2c8 on MaxFangX:update-guides into 5c89ec6 on lightningnetwork:master.

bryanvu
bryanvu previously approved these changes Aug 11, 2017
View reviewed changes
Roasbeef
Roasbeef approved these changes Aug 15, 2017
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🏌🏾

@Roasbeef Roasbeef merged commit 9cd1168 into lightningnetwork:master Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@bryanvu bryanvu bryanvu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MaxFangX @bryanvu @coveralls @Roasbeef