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

cmd: Remove default values for -ethUrl #1448

Merged
merged 4 commits into from
Apr 15, 2020
Merged

Conversation

yondonfu
Copy link
Member

@yondonfu yondonfu commented Apr 14, 2020

What does this pull request do? Explain your changes. (required)

This PR removes the default values for the -ethUrl flag. After this PR, whenever the node connects to an on-chain network, an Ethereum node JSON-RPC URL will need to be explicitly provided using the -ethUrl flag.

Specific updates (required)

  • Remove the default values for the -ethUrl flag
  • Only run mainnet tests in test_args.sh if the MAINNET_ETH_URL env variable is set
  • Only run Rinkeby tests in test_args.sh if the RINKEBY_ETH_URL env variable is set
  • Use -ethUrl in README commands

Values for MAINNET_ETH_URL and RINKEBY_ETH_URL are set via CircleCI.

How did you test each of these updates (required)

Updated tests in test_args.sh.

Does this pull request close any open issues?

Fixes #1434

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

@j0sh
Copy link
Contributor

j0sh commented Apr 14, 2020

Curious, will users have a convenient way to discover or acquire an Ethereum JSON-RPC endpoint? Should that be documented somewhere? This is going to require that mainnet users upgrading to the next release make changes to their CLI flags, right?

@yondonfu
Copy link
Member Author

yondonfu commented Apr 14, 2020

Users will still be able to use a hosted node service like Infura or Alchemy if they don't want to run their own node they'll just have to sign up for an account themselves.

livepeer/docs#58 updates RTD with a section on connecting either to hosted node service or a self-hosted node. Side note: I was thinking about whether this information should go into this repo or in the RTD and decided to update RTD since some of that information was already there. Would be interested in just rendering go-livepeer specific docs from this repo at some point and having RTD linking to the "go-livepeer guide".

This is going to require that mainnet users upgrading to the next release make changes to their CLI flags, right?

Yeah. We're removing the default Infura endpoints because we're going to only allow requests from the explorer soon for those endpoints (I believe this is in response to updates to Infura's pricing plan).

@yondonfu
Copy link
Member Author

70c2315: Add development docs with notes on running tests and specifying mainnet/Rinkeby Ethereum node JSON-RPC API URLs in order to run certain tests.

@j0sh
Copy link
Contributor

j0sh commented Apr 14, 2020

Side note: I was thinking about whether this information should go into this repo or in the RTD and decided to update RTD

Maybe also include a link to the RTD docs for those trying to figure out how to obtain an Ethereum JSON-RPC URL?

I know this has been forewarned a few times on Discord, the forums, etc, but I'd also make sure this change is prominently called out in the release notes and announcement as something that requires user action in order to complete the upgrade. Also doesn't hurt to explain the reason for it, eg Infura pricing, so users feel a little more informed and it seems a little less arbitrary, because registering with a RPC service or setting up a node is going to be a point of friction.

Copy link
Contributor

@kyriediculous kyriediculous left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@yondonfu
Copy link
Member Author

yondonfu commented Apr 14, 2020

Maybe also include a link to the RTD docs for those trying to figure out how to obtain an Ethereum JSON-RPC URL?

Added in c808bb4. The link to RTD doesn't actually go to the relevant section, but this will be fixed once livepeer/docs#58 is merged.

I'd also make sure this change is prominently called out in the release notes and announcement as something that requires user action in order to complete the upgrade. Also doesn't hurt to explain the reason for it, eg Infura pricing, so users feel a little more informed and it seems a little less arbitrary, because registering with a RPC service or setting up a node is going to be a point of friction.

👍 Definitely. Planning on calling this out in the release notes + Discord announcement.

doc/ethereum.md Outdated Show resolved Hide resolved
Copy link
Contributor

@j0sh j0sh left a comment

Choose a reason for hiding this comment

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

LGTM after a fixup

@kyriediculous
Copy link
Contributor

Looks good, let's squash the fixup commit 👍

@yondonfu yondonfu merged commit e654cfe into master Apr 15, 2020
@yondonfu yondonfu deleted the yf/remove-default-ethurl branch April 15, 2020 01:00
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.

Remove default ethUrl values
3 participants