-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow block identifier for Contract.estimateGas #1639
Conversation
66ad31d
to
900c23b
Compare
This evens the API with eth.estimateGas.
900c23b
to
190a6ac
Compare
Thanks for the PR @palango. Can we get one or more tests documenting this change? |
Sure, I'll add some tests, but let me know if you want something particular. |
@marcgarreau I wonder how #1046 actually works, the Do I need to add that there? |
Coming up to speed on the block identifier... just curious, if you're testing this locally, are you using parity/openethereum? Looks like the feature is available on that client, but not geth. If we follow the precedent set by #1046, then we'd skip the implemention in eth-tester, and document that in the tests similar to this xfail. |
Yes, openethereum supports it, but geth not. See ethereum/go-ethereum#2586
I remember vaguely that I read somewhere that py-evm supports a block identifier on this call. I f so, it would be nice to support it. |
Had a quick chat with @carver who confirmed py-evm does have support for the block identifier, and that eth-tester would need to be updated to that effect. Are you interested in giving that a shot? |
I don't have time for that at the moment. I'll come back if that gets better. |
(started work on this today) |
There's a PR up in eth-tester which satisfies these failing tests (and hopefully doesn't create any new ones). |
We can try to get new eth-tester released on Monday. In the meantime, it is possible to pin a requirement to a github-hosted commit, if you want to try it out. @marcgarreau |
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.
Great! (to merge after the eth-tester dependency can be pulled from pypi again)
What was wrong?
Related to Issue #1011 and PR #1046
How was it fixed?
Adds
block_identifier
toContract.estimateGas(...)
Todo:
Cute Animal Picture