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

Update all examples - major overhaul #87

Merged
merged 7 commits into from
May 25, 2018

Conversation

pinheadmz
Copy link
Member

This PR represents a week or so of testing all examples in the docs, trying every function for cURL, CLI and JS. Most changes were in the JS examples since the structure of the system has changed so much. I am going to continue to add commits to this branch to clean up typos, and add a few more examples for functions that were not listed before. I'm not totally happy with the style of all my fixes, (some examples are written for testnet and others for regtest, for example). So I intend to keep working on the docs and make every single example copy-paste-executable from top to bottom in all three interfaces. I figured this was a good point to squash commits and submit for review while I clean up the rest.

@pinheadmz pinheadmz requested a review from bucko13 May 22, 2018 20:40
Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Awesome stuff! This is looking fantastic.

@@ -3,12 +3,12 @@
## Default Listeners
```shell--visible
# With curl you just send HTTP Requests based on further docs
# Only thing to have in mind is Authentication, which is described in Auth section.
# Only thing to keep in mind is authentication, which is described in the "Authentication" section.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a link to the Authentication section?


```shell--visible
# You can use config file
Copy link
Contributor

Choose a reason for hiding this comment

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

It should still be possible to use a conf file and will be looked for by bclient. We should also include some information about how the wallet will look for a separate conf file (this should be documented in the changelog)

export BCOIN_NETWORK=regtest
export BCOIN_API_KEY=yoursecret
export BCOIN_API_KEY=api-key
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe preface with a $ to denote a variable and show that this is just a place holder

There are two objects: `bcoin.http.Client` for general API and `bcoin.http.Wallet` for wallet API.
You can also use the API with a Javascript library (used by `bcoin-cli`).
There are two objects: `NodeClient` for general API and `WalletClient` for wallet API.
`bcoin` also provides an object `Network` and its method `get` which will return the appropriate configuration paramaters for a specified network.
Copy link
Contributor

Choose a reason for hiding this comment

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

"default configuration" might be better than "appropriate" since you can actually use different ports for example than what is set by default.


`bcoin.http.Client` options:
`NodeClient` and `WalletClient` options:
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! You're really digging deep :)

@@ -107,12 +113,10 @@ let blockhash;
```

```shell--vars
blockhash='0000000000000dca8da883af9515dd90443d59139adbda3f9eeac1d18397fec3';
```
blockhash='1896e628f8011b77ea80f4582c29c21b3376183683f587ee863050376add3891'```
Copy link
Contributor

Choose a reason for hiding this comment

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

for readability and consistency I think we should keep the triple back tick ``` on a new line

```

```shell--cli
bwallet-cli --id=$id tx $hash
```

```shell--curl
curl $url/wallet/$id/tx/$hash
curl $walleturl/$id/tx/$hash
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 these still have to have /wallet/ in the path?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm cheating a bit here: at the top of the docs I add a new environment variable:
in _wallet.md L#61:

# examples in these docs will use an environment variable:
walleturl=http://x:api-key@127.0.0.1:48334/wallet/
curl $walleturl/$id

}

const walletClient = new WalletClient(walletOptions);
const wallet = walletClient.wallet(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

You may have already had this somewhere and I missed it, but I think we should document the difference between walletClient.wallet(id) version of the client and the client where you pass in the id as an argument like: walletClient.getInfo('id'). This is new and it tripped me up at first so it's a little confusing but also super useful to have both available (so you can create different clients for different wallets, or just use one walletClient to access different wallets depending on the id you pass it). it also makes the function signature slightly different depending on if you have the walletClient.wallet client or the normal walletClient client.

Copy link
Member Author

Choose a reason for hiding this comment

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

added to #the-walletdb-and-object


(async () => {
const response = await wallet.getRange(account, {start: start, end: end});
console.log(response);
const result = await wallet.getRange(account, {start: start, end: end});
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this work for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I got the current UTC timestamp date +%s on my system, subtracted 2000 and 1000 seconds respectively for start and end, got a few transactions I'd mined over the past half hour or so


# Authentication
## Auth

```shell--curl
curl http://x:[api-key]@127.0.0.1:8332/
# port determines network type (48332 for regtest), API key required in URL
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessarily true. As mentioned in another comment, you can set custom ports when starting up your node (I believe there is a port argument) so these are just the default, not necessarily what determines your network type.

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.

None yet

2 participants