-
-
Notifications
You must be signed in to change notification settings - Fork 15
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: properly detect, specify, and handle connecting forked network to non-fork [APE-1393] #156
Conversation
Partially addresses: ApeWorX/ape#1467 |
if not self.metadata.forked_network: | ||
# This will fail when trying to connect hardhat-fork to | ||
# a non-forked network. | ||
raise HardhatProviderError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we had an issue with the genesis block on certain networks, which is why we had to switch to logging as an error instead, with hopes to someday having a better way. Now, hardhat has a metadata RPC that we can use to deduce whether the network is a fork, so we can put back this error.
See this comment for historical reference: #32 (comment)
"Network is no a fork. " | ||
"Hardhat is likely already running on the local network. " | ||
"Try using config:\n\n(ape-config.yaml)\n```\nhardhat:\n " | ||
"host: auto\n```\n\nso that multiple processes can automatically " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this message should prevent the user from being misled so much, and even has a suggestion for how to fix.
this is the part that fixes ApeWorX/ape#1467
53f37eb
to
13f4712
Compare
@@ -176,26 +175,6 @@ def test_contract_revert_custom_exception(owner, get_contract_type, accounts): | |||
assert err.value.inputs == {"addr": accounts[7].address, "counter": 123} | |||
|
|||
|
|||
def test_contract_dev_message(owner, get_contract_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, we can longer run this test without installing ape-vyper
, maybe we will find a way to have a less integration-y test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid
What I did
fixes: ApeWorX/ape#1467
How I did it
How to verify it
Checklist