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.

Sign up for GitHub

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

Jump to bottom

Add async eth.sign #2833

Merged
merged 1 commit into from
Feb 24, 2023
Merged

Add async eth.sign #2833

merged 1 commit into from
Feb 24, 2023

Conversation

e3243eric
Copy link
Contributor

@e3243eric e3243eric commented Feb 21, 2023 •
edited
Loading

What was wrong?

Related to Issue #2826. The sign method is unavailable on the AsyncEth class.

How was it fixed?

Add the sign method to the AsyncEth class, and add corresponding tests.

Todo:

Cute Animal Picture

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

@e3243eric e3243eric force-pushed the async_sign branch 7 times, most recently from a4b5a77 to 8d3bdf3 Compare February 22, 2023 13:14
kclowes
kclowes reviewed Feb 22, 2023
View reviewed changes
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.

Thanks for the contribution! I left a comment on the line where the tests are failing. It should be a quick fix, but I'm happy to do it if you don't want to/have time. Just let me know. Looks good otherwise!

web3/_utils/module_testing/eth_module.py Outdated Show resolved Hide resolved
@e3243eric e3243eric force-pushed the async_sign branch 7 times, most recently from b3f6bf4 to ffd6d44 Compare February 23, 2023 19:30
@e3243eric e3243eric requested a review from kclowes February 23, 2023 19:44
kclowes
kclowes reviewed Feb 23, 2023
View reviewed changes
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 looks good, thank you! I left an inline comment saying this same thing, but the sign_munger can get moved into the BaseEth class and then just called like: mungers=[BaseEth.sign_munger] in the Eth and AsyncEth classes. Once that's updated, we should be good to merge!

web3/eth/async_eth.py Outdated Show resolved Hide resolved
@kclowes kclowes mentioned this pull request Feb 23, 2023
Closed
@e3243eric
Copy link
Contributor Author

Ok, I moved the sign_munger to the BaseEth class and updated it in the Eth and AsyncEth classes.

kclowes
kclowes approved these changes Feb 24, 2023
View reviewed changes
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.

Looks great! Thanks @e3243eric!

@kclowes kclowes merged commit a2ecdc3 into ethereum:master Feb 24, 2023
@e3243eric e3243eric deleted the async_sign branch February 25, 2023 07:23
@e3243eric e3243eric mentioned this pull request Mar 2, 2023
Merged
1 task
@fselmo fselmo mentioned this pull request Mar 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kclowes kclowes kclowes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@e3243eric @kclowes