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.

Sign up for GitHub

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

Auto web3 #344

Merged
merged 8 commits into from
Nov 15, 2017
Merged

Auto web3 #344

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
facd755
Added auto web3 configuration
Oct 10, 2017
52e9151
Fixed lint tests
Oct 10, 2017
51d5abb
Changed web3 instance variable name to w3
Oct 10, 2017
7726190
Merge remote-tracking branch 'origin' into issue-302-auto-web3
Oct 18, 2017
49e0d10
Added some sugestions
Oct 18, 2017
12c23d8
Added importlib and connector functionality
Nov 15, 2017
26de091
Added back testnet variable
Nov 15, 2017
daed896
Changed Windows pipe path string to os.path.join
Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions web3/auto/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from web3.auto import ipc
from web3.auto import http

w3 = ipc.ipc()

if not w3:
w3 = http.http()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now, this is opening more connections than it needs to:

  1. It connects to http even if ipc is available (on module import of http)
  2. It re-attempts the connection that was already tested during module import

You could get around both of those issues by switching to a dynamic import and using the w3 already defined, looking something like:

for connector in ('ipc', 'http'):
    connection = importlib.import_module('web3.auto.%s' % connector)
    if connection.w3:
        ...

It could be nice to give these modules a consistent interface, something like ipc.connect() and http.connect(). Not critical, though.

Copy link
Author

Choose a reason for hiding this comment

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

I like the ipc.connect() and http.connect(), makes it clearer.

16 changes: 16 additions & 0 deletions web3/auto/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from web3 import (
HTTPProvider,
Web3
)


def http():

w3 = Web3(HTTPProvider('http://localhost:8545'))
Copy link
Member

Choose a reason for hiding this comment

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

It's nice to be able to configure this type of thing using environment variables. This can be done something like the following.

w3 = Web3(HTTPProvider(
    os.environ.get('WEB3_HTTP_PROVIDER_URI', 'http://localhost:8545')
))

This allows for this functionality to be used beyond just localhost.

if w3.isConnected():
return w3
else:
return False


w3 = http()
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that it may not be good to instantiate this here since it's done in web3.auto

Copy link
Collaborator

Choose a reason for hiding this comment

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

This supports the API from the issue:

from web3.auto.http import w3

Which would give you an autocreated http connection even if an IPC one exists. I suppose we could revisit whether that's necessary. I don't have a strong use case for it, it just seemed like a good idea in the abstract.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this comment should address the double-instantiation issue: https://github.com/pipermerriam/web3.py/pull/344#discussion_r143795348

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I will work on the recommendations.

^.^

Copy link
Author

@sbc64 sbc64 Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

My two cents on the issue of whether to use from web3.auto.http import w3 and from web3.auto.ipc import w3.

I don't think people will be running three different ethereum clients, maybe me to test this implementation. They will likely be running a single client. I might be wrong on this but it is usually testrpc or parity or geth.

Using from web3.auto import w3 is enough to find the current client running.

Copy link
Collaborator

@carver carver Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm happy with going that direction: either you're using a simple, single-client setup and can use from web3.auto import w3 or you have multiple clients and you need to specify the provider explicitly. That doesn't seem like an unreasonable burden.

28 changes: 28 additions & 0 deletions web3/auto/ipc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from os import path

from web3 import (
IPCProvider,
Web3
)


def ipc():

home = path.expanduser('~')

# The following 2 lines are default paths
parity_path = home + '/.local/share/io.parity.ethereum/jsonrpc.ipc'
geth_path = home + '/.ethereum/geth.ipc'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this will cover the major operating systems, for geth. Take a look at how the IPC module does it: https://github.com/pipermerriam/web3.py/blob/master/web3/providers/ipc.py#L38-L65

In fact, it might be better to just initialize IPCProvider() without any args, which will automatically look for the geth file.

Nice that it looks for the parity path! Can you add in support for parity on other OS's, too?

Copy link
Member

Choose a reason for hiding this comment

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

I would be supportive of just adding the parity paths to the web3.providers.ipc.get_default_ipc_path function. In fact, I think it would be good to modify that function so that rather than just returning a path, it enumerates a number of different paths and checks to see if there is an IPC socket in place that it can connect to and returns the first one.

Order of operations I would like to see on that is:

  1. go-ethereum (mainnet)
  2. go-ethereum (rinkeby)
  3. go-ethereum (ropsten)
  4. parity (mainnet)
  5. parity (ropsten)
  6. parity (kovan)


w3 = Web3(IPCProvider(parity_path))
if w3.isConnected():
return w3

w3 = Web3(IPCProvider(geth_path))
if w3.isConnected():
return w3

return False


w3 = ipc()
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Maybe remove this instantiation since it's done independently in web3.auto?