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

Integration get started page #566

Merged
merged 18 commits into from
Sep 30, 2021
Merged

Integration get started page #566

merged 18 commits into from
Sep 30, 2021

Conversation

zhyatt
Copy link
Contributor

@zhyatt zhyatt commented Sep 1, 2021

Review Integration Guides index page

docker pull ghcr.io/nanocurrency/nano-docs:pr-566
docker run --rm -it -p 8000:8000 ghcr.io/nanocurrency/nano-docs:pr-566

@filipesmedeiros
Copy link

I'll take a look as soon as I have a free slot!

@qwahzi
Copy link
Collaborator

qwahzi commented Sep 15, 2021

Some initial thoughts. Will edit with more notes when I get more time:

"What is Nano? Overview" page

  • "Pending", "Proof-of-Work", "Representative", "Contributing", "running a node", "review integration options", "RPC", and "CLI" links didn't work because of missing trailing slash [Confirmed Docker artifact after testing on live docs.nano.org]

  • Living Whitepaper link is broken because it's looking for /living-whitepaper/index.md vs /living-whitepaper/ [Confirmed issue, not just Docker artifact]

  • Pending should be changed to "Receivable / Ready to Receive" per Replacing pending with receivable/ready to receive nano-node#3411

  • The receiver can be offline and safely leave the funds in this state until they are ready to publish a matching block receiving the funds to their account, though best practice is to publish receive blocks as soon as possible so that the balance participates in consensus, and so that optional pruning is more effective(?) [Possible suggestion, but might be overkill for an overview page]

"Integration Guides" page

  • Nano could be capitalized in the "How transactions work" paragraph?

  • "Production integration node should be non-voting" section should mention: "Voting is configured in the config-node.toml file."

"Integration Guides: The Basics" page

  • The pending reference should be replaced with "Receivable / Ready to Receive"

  • Links to "Distribution and Units" page that still has the old unit structure

"Integration Guides: Key Management" page

  • 10/12 "pending blocks" references (2 are RPC examples) should be changed to "Receivable / Ready to Receive"

Living Whitepaper: Distribution and Units

  • Unit dividers section should be fixed so that only Nano/Nano/nano == 10^30 and raw == 1 == 10^0 exist

"Glossary" page

@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 16, 2021

Awesome, thanks for the initial feedback @qwahzi ⭐ Will review and reply once you have a chance to provide additional notes.

@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 16, 2021

Thanks for helping @filipesmedeiros !

@satoshizzle
Copy link

I might be wrong but In https://github.com/nanocurrency/nano-docs/blob/master/docs/what-is-nano/overview.md

"Once a block sending funds is confirmed by the network, the transaction goes into a pending state and cannot be reversed"

Shouldn't that be replaced with "ready to receive" state?

https://www.reddit.com/r/nanocurrency/comments/ov3fro/term_pending_to_be_replaced_with_ready_to_receive/

@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 16, 2021

@satoshizzle Thanks, I will include this when I review all feedback shortly. We have a separate issue related to this to update throughout the docs soon: #554

@mistakia
Copy link

The Integration Guides link on the homepage should probably link to the index page instead of The Basics page.

@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 17, 2021

@qwahzi I think I covered your items, some notes:

@satoshizzle @mistakia Your comments should also be resolved now.

@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 20, 2021

I am looking to merge this soon so if there are any remaining points of feedback, please drop them in. Otherwise, marking "Approved" on the review is much appreciated!

@otonashixav
Copy link

  1. I might suggest using strictly IPv4 for the docker setup, possibly moving IPv6 configuration to an additional setup step. I quickly ran through the setup process and ran into issues querying the RPC since IPv6 is not enabled by default (I recall some users on discord having this issue) and not supported at all on windows hosts (me while testing). Using [::1] in the curl command did not work, whereas specifying -p 127.0.0.1:17076:17076 allowed the RPC to be queried.
  2. Since docker is the suggested method of running a node, it would be nice if commands such as nano_node --generate_config node in the Node Configuration section showed how to run them with docker i.e. docker exec container_name nano_node --generate_config node.

@zhyatt zhyatt mentioned this pull request Sep 21, 2021
@zhyatt
Copy link
Contributor Author

zhyatt commented Sep 21, 2021

  1. I might suggest using strictly IPv4 for the docker setup, possibly moving IPv6 configuration to an additional setup step. I quickly ran through the setup process and ran into issues querying the RPC since IPv6 is not enabled by default (I recall some users on discord having this issue) and not supported at all on windows hosts (me while testing). Using [::1] in the curl command did not work, whereas specifying -p 127.0.0.1:17076:17076 allowed the RPC to be queried.

Added to new issue for consideration #572

  1. Since docker is the suggested method of running a node, it would be nice if commands such as nano_node --generate_config node in the Node Configuration section showed how to run them with docker i.e. docker exec container_name nano_node --generate_config node.

Added to new issue as well #574

Thanks @otonashixav

@zhyatt zhyatt merged commit b238647 into master Sep 30, 2021
@zhyatt zhyatt deleted the integration-get-started branch September 30, 2021 22:46
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

6 participants