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

Add RPCAddr option for the JSON-RPC API #444

Closed

Conversation

@nfeignon
Copy link
Contributor

nfeignon commented Oct 7, 2019

Allows choosing the JSON-RPC API listening interface.
Set it to localhost as default for security considerations.
It currently listens on any interface (0.0.0.0).

@albrow

This comment has been minimized.

Copy link
Member

albrow commented Oct 15, 2019

Thanks @nfeignon! Sorry for the late reply, we were busy at Ethereum Devcon last week.

This is a good change, but if we're going to allow users to specify a custom listen address, I would rather combine RPC_ADDR and RPC_PORT into a single environment variable for convenience. Can we change the RPC_ADDR env var to expect an IP address as well as a port? Then we can deprecate RPC_PORT and eventually remove it.

@fabioberger fabioberger force-pushed the 0xProject:development branch from 11e0fa4 to cdece2c Oct 18, 2019
@fabioberger

This comment has been minimized.

Copy link
Contributor

fabioberger commented Oct 18, 2019

@nfeignon we unfortunately had to force-push to development under exceptional circumstances. Could you please rebase your PR off of development?

The steps assuming you've updated your forks development to be identical to upstream would be:

git checkout feature/add-rpc-addr-option
git checkout -b feature/add-rpc-addr-option-old
git checkout development
git branch -D feature/add-rpc-addr-option
git checkout -b feature/add-rpc-addr-option
git cherry-pick c11591e14238808a7cfd34ce849375546b45c0b1
git push origin feature/add-rpc-addr-option -f
@nfeignon nfeignon force-pushed the nfeignon:feature/add-rpc-addr-option branch from c11591e to fd04061 Oct 18, 2019
@nfeignon

This comment has been minimized.

Copy link
Contributor Author

nfeignon commented Oct 18, 2019

@fabioberger No problem, I've rebased my PR.

@albrow I chose to separate RPC_ADDR and RPC_PORT because that's what Parity (--jsonrpc-port, --jsonrpc-interface) and Geth (--rpcaddr, --rpcport) do but I don't mind editing my PR :)
Should I remove RPC_PORT completely or ignore it if RPC_ADDR is defined? What about the default interface (any or localhost) if only RPC_PORT is defined?

@albrow

This comment has been minimized.

Copy link
Member

albrow commented Oct 22, 2019

Should I remove RPC_PORT completely or ignore it if RPC_ADDR is defined? What about the default interface (any or localhost) if only RPC_PORT is defined?

Here's what I'm thinking:

  • Let's leave RPC_PORT for now, in order to maintain backwards compatibility.
  • If RPC_ADDR is defined, we should ignore RPC_PORT.
  • RPC_ADDR should be expected to always include the address and port.
  • If RPC_PORT is defined and RPC_ADDR is not defined, we should use the current default bind address: 0.0.0.0 to maintain backwards compatibility.

Then, in a future release, we can remove the RPC_PORT environment variable.

@albrow

This comment has been minimized.

Copy link
Member

albrow commented Oct 28, 2019

@nfeignon talked it over with @fabioberger and since we are nearing the 6.0.0 release at this point, we would like to go ahead and break backwards compatibility here. (It's better to do it now than later since we are still in beta at the moment).

Instead of what I suggested in my previous comment, let's simply remove RPC_PORT. RPC_ADDR should be expected to always include the address and port.

@nfeignon nfeignon force-pushed the nfeignon:feature/add-rpc-addr-option branch from fd04061 to d6b2046 Oct 29, 2019
@nfeignon

This comment has been minimized.

Copy link
Contributor Author

nfeignon commented Oct 29, 2019

Instead of what I suggested in my previous comment, let's simply remove RPC_PORT. RPC_ADDR should be expected to always include the address and port.

Great, I updated my commit to remove RPC_PORT.

@albrow

This comment has been minimized.

Copy link
Member

albrow commented Nov 1, 2019

@nfeignon in the interest of time, I'm moving this over to #487 so I can fix the merge conflicts for you. You will still get credited for your commit on GitHub.

Thanks again for your contribution and thank you for bearing with us!

@albrow albrow closed this Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.