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

Async sign and send raw middleware #3025

Merged
merged 4 commits into from Jul 6, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 6, 2023

What was wrong?

closes #2773
closes #2936
closes #2937

How was it fixed?

  • Add async support for sign-and-send raw transaction middleware
  • Use the hex value directly, instead of relying on formatters, since the JSON-RPC specs expect the hex
  • Add tests

Todo:

Cute Animal Picture

20230705_082223

@fselmo fselmo force-pushed the async-sign-and-send-raw-middleware branch from 5a98a83 to 83b3a8a Compare July 6, 2023 15:33
- Copy the tests for sync over to async
- Don't assume some conversion is going to happen later down the line. The JSON-RPC asks for hexstr value, give it the hexstr value if we can guarantee that it is a hexstr. We already have the type as HexBytes so call ``.hex()`` on the value and send that as the param.
- closes ethereum#2936
@fselmo fselmo force-pushed the async-sign-and-send-raw-middleware branch from 83b3a8a to de2f61a Compare July 6, 2023 15:35
@fselmo fselmo marked this pull request as ready for review July 6, 2023 15:35
@fselmo fselmo requested review from kclowes, reedsa and pacrob July 6, 2023 15:42
"value": 1,
"nonce": 0,
}
if isinstance(expected, type) and issubclass(expected, Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

Exceptions could be covered in a separate parameterized test, but really your preference here.

Copy link
Collaborator Author

@fselmo fselmo Jul 6, 2023

Choose a reason for hiding this comment

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

Agreed, this is not ideal for new tests. I wanted to just copy the older sync cases so they can share the same test params but revamping both would be a good idea at some point.

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

This LGTM! :shipit:

@fselmo fselmo merged commit 896fd77 into ethereum:main Jul 6, 2023
74 checks passed
@fselmo fselmo deleted the async-sign-and-send-raw-middleware branch April 3, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants