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

Allow Ethereum host to be configured by URL #13

Merged
merged 4 commits into from
Jan 23, 2018

Conversation

hayesgm
Copy link
Collaborator

@hayesgm hayesgm commented Jan 11, 2018

This patch adds the ability to configure the Ethereum host via a URL, e.g.

config :ethereumex, url: "https://rinkeby.infura.io/<token>"

Additionally, we allow http options to be passed in to the RPC calls (e.g. to configure timeouts).

…thereumex.

This patch passes through an HTTP options config from the caller. This allows us to set the configuration (such as connect and recv timeout) from the caller into EthereumEx. As there are a large number of possible configuration options, it seems easier to pass the options from HTTPoison to the caller than to expose them through our own interface.
@hayesgm hayesgm requested a review from ayrat555 January 11, 2018 23:01
config/dev.exs Outdated
@@ -1,5 +1,6 @@
use Mix.Config
config :ethereumex,
url: System.get_env("ETHEREUM_URL"),
Copy link
Member

@ayrat555 ayrat555 Jan 12, 2018

Choose a reason for hiding this comment

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

@hayesgm Instead of adding another url option we can add url_path option so the Ethereum host will be configured:
"#{scheme()}://#{host()}#{port()}/#{url_path()}"

the only thing that should be changed is Config.port method

  @spec port() :: integer()
  defp port do
    port = env_var(:port)
    if is_nil(port), do: "", else: port
  end

  @spec env_var(atom()) :: binary() | integer()| nil
  defp env_var!(var) do
    Application.fetch_env!(:ethereumex, var)
  end
end

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

or we can remove scheme, host, port options and just use url option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I personally prefer a URL option, since it's easier to pass in and it's easier to construct a URL from scheme, host and port instead of the reverse. This ends up being important because Infura gives you a full URL to access their nodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if you want me to move forward with changing to a URL-only config.

Copy link
Member

Choose a reason for hiding this comment

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

@hayesgm it's ok, let's use a URL-only config

@ayrat555
Copy link
Member

@hayesgm Can you add your changes from this PR and #12 to Changelog and README files?

@@ -17,7 +17,7 @@ defmodule Ethereumex.HttpClient do

@spec post_request(binary()) :: {:ok | :error, any()}
defp post_request(payload) do
response = rpc_url() |> HTTPoison.post(payload, [{"Content-Type", "application/json"}])
response = rpc_url() |> HTTPoison.post(payload, [{"Content-Type", "application/json"}], Ethereumex.Config.http_options())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a good idea. We should implement this to generic Client requests as well, useful for things like timeout threshold of HTTPoision. What do you think @ayrat555 ?

Copy link
Member

@ayrat555 ayrat555 Jan 21, 2018

Choose a reason for hiding this comment

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

Yes, it's nice feature. I am just waiting for @hayesgm to move to URL option in config. And it's good to merge.

@hayesgm
Copy link
Collaborator Author

hayesgm commented Jan 22, 2018

Upgraded this to exclusively use the url parameter and added relevant documentation.

Copy link
Member

@ayrat555 ayrat555 left a comment

Choose a reason for hiding this comment

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

@hayesgm thank you!

@ayrat555 ayrat555 merged commit a82c5ad into master Jan 23, 2018
@ayrat555 ayrat555 deleted the hayesgm/http-config-url branch January 23, 2018 07:09
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.

3 participants