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

Validator UX docs for baklava network #1688

Merged
merged 56 commits into from
Nov 25, 2019

Conversation

aaitor
Copy link
Contributor

@aaitor aaitor commented Nov 13, 2019

Description

As a validator I expected to have a valid documentation page allowing to me to run a validator in baklava network including the validator, proxy and attestation service.

Tested

An explanation of how the changes were tested or an explanation as to why they don't need to be.

Other changes

Describe any minor or "drive-by" changes here.

Related issues

  • Fixes #[issue number here]

Backwards compatibility

Brief explanation of why these changes are/are not backwards compatible.

@codecov
Copy link

codecov bot commented Nov 13, 2019

Codecov Report

Merging #1688 into asaj/aaitor-docs will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           asaj/aaitor-docs   #1688    +/-   ##
=================================================
  Coverage              74.2%   74.2%            
=================================================
  Files                   278     278            
  Lines                  7653    7653            
  Branches                956     672   -284     
=================================================
  Hits                   5679    5679            
  Misses                 1857    1857            
  Partials                117     117
Flag Coverage Δ
#mobile 74.2% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7d2899...175d25d. Read the comment docs.

@aaitor
Copy link
Contributor Author

aaitor commented Nov 14, 2019

This PR shouldn't be merged till the official baklava images are publicly available and all the commands are tested again using those images.

@timmoreton timmoreton added the Priority: P0 Blocker label Nov 20, 2019
Needs to be tested with a publicly available attestation-service docker image as soon as this as published.
@aaitor aaitor changed the title WIP: Validator UX docs for baklava network Validator UX docs for baklava network Nov 22, 2019
Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

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

Great stuff! There are some inaccuracies, but I'm happy to merge and improve in follow-on PRs

@@ -19,6 +19,7 @@
"prepublishOnly": "yarn build:gen && yarn build",
"start-ts": "TS_NODE_FILES=true ts-node src/index.ts",
"start": "node lib/index.js",
"db:create": "mkdir -p db && touch db/dev.db",
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 think this should be here? I.e. it should not be the concern of this application to create databases other than for development and testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that command was to make it consistent with the documentation found in the README.md of the attestation-service and the comment made about not including the dev suffix.

What do you suggest? Add the command or keep the yarn run db:create:dev instruction in the README?

docker pull $CELO_IMAGE
```

### Create accounts
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this section should cover the creation of the attestation key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid duplicate this documentation in two pages, I added the text clarifying are necessary 4 accounts (Validator, Validator Group, Proxy and Attestation Service) and added a link to the attestation service documentation page.

| DATABASE_URL | The URL under which your database is accessible, currently supported are `postgres://`, `mysql://` and `sqlite://` | |
| CELO_PROVIDER | The URL under which a celo blockchain node is reachable, i.e. something like `https://integration-forno.celo-testnet.org` | |
| ACCOUNT_ADDRESS | The address of the validator account | |
| ATTESTATION_KEY | The private key with which attestations should be signed. You could use your account key for attestations, but really you should authorize a dedicated attestation key | |
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just tell people to delegate an attestation signer, it's actually quite hard to retrieve the account key for use anyways

celocli account:new
```

We copy the account details and assign the Private Key to the `ATTESTATION_KEY` environment variable:
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to authorize the key on the Accounts smart contract

You can find the migration scripts for creating the schema at the `celo-monorepo`, `packages/attestation-service` folder. From there, after setting up the `DATABASE_URL` env variable you can run the following commands:

```bash
yarn run db:create
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work other than the sqlite driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing that db:create line and adding reference that depending on the database technology the users need to create the database with the proper user access.

@asaj asaj changed the base branch from master to asaj/aaitor-docs November 25, 2019 04:39
@asaj asaj merged commit 61a552c into celo-org:asaj/aaitor-docs Nov 25, 2019
asaj pushed a commit that referenced this pull request Nov 25, 2019
@aaitor aaitor deleted the feature/baklava_validator_doc branch November 25, 2019 08:29
aaronmgdr added a commit that referenced this pull request Nov 26, 2019
* master: (61 commits)
  [Wallet] Handle `/v/<code` deep links for phone verifications (#1776)
  Patch tslint auto fix for macOS Catalina until tslint#6.x is out (#1802)
  Fix typos (#1855)
  Update documentation sidebar (#1861)
  [Wallet] Update forno dev documentation (#1818)
  [Docs] Correct typo
  Validator UX docs for baklava network (#1688) (#1849)
  Various improvements to the CLI, allow voters to revote for a group (#1840)
  Serialize to hex string (#1848)
  Update faucet to pull from the reserve when possible (#1844)
  Fix elect validators migration, deploy integration (#1847)
  Configurable genesis balances (#1838)
  Deploy new version of Celostats and minnor change from celo-blockchain (#1714)
  Make default node URL consistent in celocli (#1805)
  Onboarding feedback (#1811)
  return at least true, when the function is returning bool (#1825)
  Minor Contractkit changes (#1819)
  Point end-to-end tests back to master (#1824)
  Update migration config to correct protocol parameters (#1822)
  Update with new istanbul lookback window size flag (#1820)
  ...

# Conflicts:
#	yarn.lock
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.

7 participants