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

Auto web3 #344

Merged
merged 8 commits into from
Nov 15, 2017
Merged

Auto web3 #344

merged 8 commits into from
Nov 15, 2017

Conversation

sbc64
Copy link

@sbc64 sbc64 commented Oct 10, 2017

What was wrong?

This request provides a solution for: https://github.com/pipermerriam/web3.py/issues/302

How was it fixed?

The auto module configures a web3 instance instance for you. If you don't specify which provider you want it will default on IPC first, then HTTP.

Cute Animal Picture

Cute animal picture
(I'm starting to hoard cute animal pictures on my phone)

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Looks great! Excited to have this in. Just a couple pieces of feedback.

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.

web3/auto/ipc.py Outdated

# 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)

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

A few suggested changes.


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.

web3/auto/ipc.py Outdated

# 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
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)

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

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

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.

web3/auto/ipc.py Outdated
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?

@pipermerriam pipermerriam mentioned this pull request Oct 10, 2017
Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Cool, I'd love to get this into the upcoming beta. I included a few "nice-to-have" suggestions, but I don't think they should block merging.

if connection.w3:
w3 = connection.w3

if w3.isConnected():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check looks redundant: if w3 is set, then isConnected() is True

if os.path.exists("\\\\.\\pipe\\geth.ipc"):
return "\\\\.\\pipe\\geth.ipc"
elif os.path.exists("\\\\.\\pipe\\jsonrpc.ipc"):
return "\\\\.\\pipe\\jsonrpc.ipc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

A utility function like first_available_path(paths) could maybe remove some of the repetition here.

@carver carver added this to the Version 4 Stable milestone Nov 15, 2017
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

One change requested assuming my remembered knowledge of constructing paths on various operating systems is accurate.

elif sys.platform == 'win32':
return "\\\\.\\pipe\\geth.ipc"
if os.path.exists("\\\\.\\pipe\\geth.ipc"):
Copy link
Member

Choose a reason for hiding this comment

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

I think these should still use os.path.join to construct these paths as it will use the proper path separators for the os.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, I've been out of windows for so long that I don't remember what \\.\pipe\geth.ipc even means.

@Sebohe because of the '\' start, it wasn't intuitive to me how to use join here. This was the first way I could find to do the join: os.path.join('\\\\', '.', 'pipe', 'geth')

It looks like this is the last change needed to get merged.

elif sys.platform == 'win32':
return "\\\\.\\pipe\\geth.ipc"
ipc_path = os.path.join(
"\\\\",
Copy link
Member

Choose a reason for hiding this comment

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

I so wish we had windows CI to know that this works.... 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagging #166 for motivation.

Copy link
Author

@sbc64 sbc64 Nov 15, 2017

Choose a reason for hiding this comment

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

I tested it on my windows machine if it is of some consolation to you. I couldn' get seperated "\\", "\\" to work. It would merge them to one \. That is why I used 4 backslashes.

Copy link
Author

Choose a reason for hiding this comment

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

I also had geth and parity running while doing my in house testing, but I agree a more thourough test would be comforting.

@pipermerriam
Copy link
Member

:shipit:

@carver carver merged commit 3f3b869 into ethereum:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants