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

Migrate tester provider results from middleware to defaults #1188

Merged
merged 1 commit into from
Jan 12, 2019

Conversation

dylanjw
Copy link
Contributor

@dylanjw dylanjw commented Jan 7, 2019

What was wrong?

Many of default results for the EthereumTesterProvider come from a set of special middlewares. Eliminating these middlewares will reduce the number of middlewares that need to be duplicated for async support. Having these middlewares eliminated will also help with testing the early stages of the async api, which implements no middlewares.

Before I get too far with this, does this seem like an acceptable way forward? Should I continue eliminating the rest of the eth_tester middlewares? is there any reason that these results and formatters need the middleware pattern? Removing them does not preclude using custom middlewares to alter the EthereumTesterProvider defaults.

Cute Animal Picture

image

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I think 👍

@dylanjw
Copy link
Contributor Author

dylanjw commented Jan 7, 2019

Great. I'll continue with the rest.

Not ready to merge until the following are complete:

  • Eliminate ethereum_tester_fixture_middleware
    - [ ] Eliminate default_transaction_fields_middleware
    - [ ] Eliminate ethereum_tester_middleware

Copy link
Member

@pipermerriam pipermerriam 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 👍 to merge assuming it is complete.

@dylanjw
Copy link
Contributor Author

dylanjw commented Jan 10, 2019

This is ready to merge. I am going to work on the request parameter formatters and the transaction fields middleware in separate pull requests.

@pipermerriam
Copy link
Member

@dylanjw you should not have merge rights.

@dylanjw dylanjw merged commit f88d22e into ethereum:master Jan 12, 2019
@dylanjw
Copy link
Contributor Author

dylanjw commented Jan 12, 2019

Assuming piper meant 'now' instead of 'not', in case anyone reading this PR thinks Im out of control.

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.

2 participants