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 raw transaction signing middleware with byte addresses #1067

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

koirikivi
Copy link
Contributor

What was wrong?

Using web3.middleware.construct_sign_and_send_raw_middleware with bytes typed addresses caused improper behaviour in at least the following cases:

  1. Passing a bytes typed address as the from address, in which case the middleware got never executed because the check transaction.get('from') not in accounts always evaluated as True
  2. Passing a bytes typed address as the to address (for an instance, when doing web3.eth.contract(address=bytes(...), ...)), which resulted in an exception.

How was it fixed?

The transaction is now formatted according to the transaction abi types in sign_and_send_raw_middleware (similar to what's done with abi_middleware).

I'm not totally sure about the fix, since I don't know web3.py internals that well. I discussed this issue with @voith and he encouraged me to submit a fix though. Any feedback is appreciated :) .

Cute Animal Picture

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

@koirikivi koirikivi force-pushed the signing-middleware-bytes branch 3 times, most recently from 8e136eb to 8ab7413 Compare September 20, 2018 20:19
@koirikivi koirikivi force-pushed the signing-middleware-bytes branch from 8ab7413 to 442b50a Compare September 20, 2018 20:49
Copy link
Contributor

@dylanjw dylanjw left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for working on this.

@dylanjw dylanjw merged commit d502e25 into ethereum:master Sep 20, 2018
@koirikivi
Copy link
Contributor Author

koirikivi commented Sep 21, 2018

Thanks for the quick merge!

Is it possible to also get this to V4? This is very much something I'd like to use in our soon-to-hit production code, without forking :) .

@carver
Copy link
Collaborator

carver commented Sep 21, 2018

Yeah, by the looks of it, it would be a single bugfix release: 4.7.2 -- hopefully sometime early next week.

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