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

fix: validate Ape has connected to the right network and client #1038

Merged
merged 10 commits into from Sep 15, 2022

Conversation

fubuloubu
Copy link
Member

What I did

Reorder how Web3Provider determines chain ID so that a fault is detected with mismatch of chain ID

fixes: #1030
fixes: #1032

How I did it

We have some validation already in place for chain ID, and this solves a corner case in the logic for validating that the correct network and client is connecting when a connection already exists.

How to verify it

  1. Start anvil
  2. ape console --network :mainnet (assumes geth is default)

Prior to change, (2) will work just fine, and fails after this change

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@fubuloubu fubuloubu marked this pull request as draft September 7, 2022 03:33
src/ape/api/providers.py Outdated Show resolved Hide resolved
@antazoey antazoey marked this pull request as ready for review September 7, 2022 18:39
src/ape/api/providers.py Outdated Show resolved Hide resolved
@banteg
Copy link
Contributor

banteg commented Sep 10, 2022

i think this doesn't address the problem, we need to check that the associated chain id in the network spec matches the chain id from the provider we connected to. this needs to be done on connection to prevent user error of accidentally connecting to the wrong endpoint. such error could have dire results such as sending test transactions on live network or caching the wrong abis.

@antazoey
Copy link
Contributor

i think this doesn't address the problem, we need to check that the associated chain id in the network spec matches the chain id from the provider we connected to. this needs to be done on connection to prevent user error of accidentally connecting to the wrong endpoint. such error could have dire results such as sending test transactions on live network or caching the wrong abis.

If you look at the connect method in ape-geth, you will see it already has logic for this, but the chain ID here was causing it not to work.

With these changes, if you do something like:

ape console --network ethereum:rinkeby:geth

and have a URI for mainnet in your config file, you will get this output (including the error):

INFO: Connecting to existing Erigon node at 'https://erigon.yearn.science/'.
ERROR: (ProviderError) HTTP Connection does not match expected chain ID. Are you connected to 'rinkeby'?

However, if you switch to main branch, this bug is still present. Wen you try the same thing, it allow you to connect to the console.

Thus, I think this PR fixes the bug. Or no??

banteg
banteg previously approved these changes Sep 14, 2022
Copy link
Contributor

@banteg banteg left a comment

Choose a reason for hiding this comment

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

checked this pr and confirmed it works for me

@banteg
Copy link
Contributor

banteg commented Sep 14, 2022

confirmed it works. maybe one suggestion is to also add numeric chain ids to the error.

INFO: Connecting to existing Erigon node at 'http://localhost:8545'.
ERROR: (ProviderError) HTTP Connection does not match expected chain ID. Are you connected to 'goerli'?

becomes

ERROR: (ProviderError) HTTP Connection chain ID (1) does not match expected chain ID (5). Are you connected to 'goerli'?

@antazoey antazoey force-pushed the fix/geth-connection-validation branch from 145247c to cd63644 Compare September 14, 2022 16:02
ghost
ghost previously approved these changes Sep 14, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

src/ape/api/providers.py Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

🚀

@fubuloubu fubuloubu requested review from antazoey and removed request for NotPeopling2day and johnson2427 September 14, 2022 19:36
@antazoey antazoey merged commit d496cee into ApeWorX:main Sep 15, 2022
@fubuloubu fubuloubu deleted the fix/geth-connection-validation branch February 21, 2023 22:01
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.

check ape has connected to the intended network check ape has connected to the intended client
3 participants