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

Moar infura #1150

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Moar infura #1150

merged 1 commit into from
Dec 5, 2018

Conversation

fubuloubu
Copy link
Contributor

What was wrong?

There was only two options for infura networks, and one (mainnet) was default without any suitable warnings.

How was it fixed?

I added modules for the missing networks, rinkeby and kovan. I added a module for mainnet that warned you that you were on mainnet, and loaded that as the default w3 instance for the infura package.

Both seem to work fine with my dev account.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fubuloubu
Copy link
Contributor Author

Not sure why the doctest failure...

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.

Removing default import is probably something that would have to go in v5 since it'd be a breaking change.

web3/auto/infura/__init__.py Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Contributor Author

I'll remove the warning so the tests pass, and open a different issue for removing the default.

@carver
Copy link
Collaborator

carver commented Dec 5, 2018

FWIW, all our other auto imports also default to mainnet. (eg~ looking where geth puts the IPC for mainnet)

I'm absolutely in favor of being explicit, broadly across the repo. But this auto-import feature is specifically intended for quick access at the command line, so I'm not wild about giving up the quick way of connecting to prod. I especially don't want to give up from web3.auto import w3, which seems like the obvious next step after removing the infura mainnet default. (If we are going to maintain consistency across auto-imports, which I think we should)

It also make sense to backport this to v4 after this one gets merged, probably.

web3/auto/infura/__init__.py Outdated Show resolved Hide resolved
web3/auto/infura/mainnet.py Show resolved Hide resolved
@fubuloubu
Copy link
Contributor Author

@carver Well, from web3.auto import w3 is inherently explicit, because you actually have to run the endpoint for it to work lol. Infura is more implicit though, if someone were to say use their MetaMask dev account with all four networks having funds on it (albeit a small amount), it would be a little devious to forget that Infura's auto is mainnet.

P.S. hello from San Fran!

@carver
Copy link
Collaborator

carver commented Dec 5, 2018

Well, from web3.auto import w3 is inherently explicit, because you actually have to run the endpoint for it to work lol. Infura is more implicit

Yeah, I suppose. It sounds like you and Piper agree on that one, and I don't feel strongly enough about the quick infura import (or the auto import consistency) to give a 👎 -- as long as we get to keep from web3.auto import w3. 😁

Feel free to ping again when tests pass, for a merge.

hello from San Fran!

Ohai! Let's catch up off thread. (See if we can make lunch happen, or something).

@fubuloubu
Copy link
Contributor Author

Note: doctest failure is the 2040 thing in docs/examples.rst

@carver
Copy link
Collaborator

carver commented Dec 5, 2018

Note: doctest failure is the 2040 thing in docs/examples.rst

Yup, already broken in master, and addressed in another PR IIRC. Not a blocker.

@carver carver merged commit 3b93236 into ethereum:master Dec 5, 2018
@fubuloubu fubuloubu deleted the moar-infura branch December 5, 2018 23:11
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