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

eth_call formatter breaks when supplying third argument #1921

Closed
banteg opened this issue Mar 21, 2021 · 11 comments
Closed

eth_call formatter breaks when supplying third argument #1921

banteg opened this issue Mar 21, 2021 · 11 comments

Comments

@banteg
Copy link
Contributor

banteg commented Mar 21, 2021

What was wrong?

eth_call supports a third optional argument which overrides state prior to a call. web3.py makes an incorrect assumption there should only be two arguments.

  File "web3/middleware/formatting.py", line 74, in apply_formatters
    response = make_request(method, formatted_params)
  File "web3/middleware/attrdict.py", line 33, in middleware
    response = make_request(method, params)
  File "cytoolz/functoolz.pyx", line 250, in cytoolz.functoolz.curry.__call__
  File "web3/middleware/formatting.py", line 73, in apply_formatters
    formatted_params = formatter(params)
  File "cytoolz/functoolz.pyx", line 250, in cytoolz.functoolz.curry.__call__
  File "eth_utils/decorators.py", line 91, in wrapper
    return ReturnType(result)
  File "eth_utils/applicators.py", line 58, in apply_formatters_to_sequence
    raise IndexError(
IndexError: Too few formatters for sequence: 2 formatters for […]

How can it be fixed?

Make formatter aware of the third option argument for eth_call.

@wolovim
Copy link
Member

wolovim commented Mar 22, 2021

Thanks for raising the issue. We'll dig in next opportunity. PRs welcome; this might be a straightforward one for interested contributors.

@edd34
Copy link
Contributor

edd34 commented Apr 12, 2021

@marcgarreau @banteg I've created a PR, still in draft because I am not sure about it, will happy to get feedback about it 😃
#1942

@edd34
Copy link
Contributor

edd34 commented Apr 12, 2021

@banteg can you give me a snippet which reproduce the bug ? So I can test against it, thank you.
@marcgarreau Can you explain me a little more what should I do here ? I mean, how and where can I test it ? What's the workflow you usually use when working on issue. (I've tried something on #1942 )

@kclowes
Copy link
Collaborator

kclowes commented Apr 12, 2021

@edd34 thanks for the PR! To get the current tests passing, it looks like you'll need to add a new request formatter here. You'll want to follow the apply_formatter_to_dict pattern similar to line 345.

Once those are passing, I'd add a new test to tests/core/contracts/test_contract_call_interface.py that's similar to the Override example in the geth docs. For completeness, I'd also add a similar test in web3/_utils/module_testing/eth_module.py. I'm not sure if Parity/OE supports the third argument, so you may need to add the function with an xfail decorator to the ParityEthModuleTest class in tests/integration/parity/common.py and do the same thing in the TestEthereumTesterEthModule class tests/integration/test_eth_tester.py. Let me know if that doesn't make sense or if you need some more clarification!

@edd34
Copy link
Contributor

edd34 commented Apr 14, 2021

Hi @kclowes , it's not clear for me, I think I need first to understand what are formatter for and how/where are they applied.
Can you send me resource where I can grasp more understanding of it ? I'll do my own research from my side.
I'd like also to see one formater where it is all used.

@wolovim
Copy link
Member

wolovim commented Apr 15, 2021

@edd34 thanks for giving this a shot initially. there was more complexity hiding in there than my first guess. i'll pick it up from your PR.

@edd34
Copy link
Contributor

edd34 commented Apr 15, 2021

Ok I'll read the pr afterward

@wolovim
Copy link
Member

wolovim commented Apr 19, 2021

welp, this turned into something more exciting. possibly user error, but i'm getting unexpected results from Geth in my testing. i pulled in @kclowes, who reproduced them. we'll get an issue up in the geth repo to verify if its us or them.

@kclowes
Copy link
Collaborator

kclowes commented Apr 20, 2021

I just raised a geth issue here!

@wolovim
Copy link
Member

wolovim commented Apr 22, 2021

yep, user error 🤦 - used compilation vs. runtime bytecode when testing. finished up @edd34's PR (#1942), which will close this issue once merged. side note: looks like this'll be one of those rare features that we implement before web3.js and ethers.js 😅🏆

@edd34
Copy link
Contributor

edd34 commented Apr 23, 2021

Nice effort here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants