Skip to content

Conversation

@kclowes
Copy link
Collaborator

@kclowes kclowes commented Oct 21, 2020

What was wrong?

Filter is the last method in Eth that wasn't using Method. This PR fixes that.

Todo:

  • Add entry to the release notes
  • Make sure tests are using the correct middleware
  • Better solution than passing around a string in place of a method
  • Lint
  • Get rid of TODOs
  • Manually test

Cute Animal Picture

image

@kclowes kclowes force-pushed the filter-to-method-ii branch 2 times, most recently from ba906e7 to cc2dd32 Compare October 26, 2020 21:22
@kclowes kclowes changed the title [WIP] Basic filter tests passing [WIP] Move filter method to use Method class Oct 26, 2020
@kclowes kclowes force-pushed the filter-to-method-ii branch from 07af185 to 2d42385 Compare October 29, 2020 21:15
@kclowes kclowes force-pushed the filter-to-method-ii branch from 135e28b to 3e51053 Compare November 12, 2020 20:56
@kclowes kclowes changed the title [WIP] Move filter method to use Method class Move filter method to use Method class Nov 16, 2020
try:
(method_str, params), response_formatters = method.process_params(module, *args, **kwargs) # noqa: E501
except _UseExistingFilter as err:
return LogFilter(eth_module=module, filter_id=err.filter_id)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pipermerriam I can't figure out how to pull this out without going back to passing around a string method name, since all of the formatters are chosen based on the method name. Let me know if you see another way!

Copy link
Member

Choose a reason for hiding this comment

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

If we can't get this out, lets consider making it generic. I'd suggest doing that as a follow up to this PR.

Roughly speaking, it looks like we need an official way for a Method to short-circut the JSON-RPC request and to return a value that is generated locally. That could be using something like an exception like we did here, but I think that's probably not the best choice here because it feels like a heavy way to implement what is effectively an if/else.

We might need to return something more structured from process_params that we can then switch on.

request = method.preprocess_request(module, *args, **kwargs)
if request.is_json_rpc:
    ...  # do the actual request
elif request.has_static_response:
    ...  # directly return the response value.
else:
    ...  # dunno, maybe just raise Exception("Invariant")

I don't know if this is going to work, or be clean. Just my stab at what the API could look like. I think you'll be doing yourself and everyone on the team a favor to spend some time working on these APIs, especially in making them less cryptic in how they work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, agreed. I'll do some mucking around with it in another PR.

@kclowes
Copy link
Collaborator Author

kclowes commented Nov 16, 2020

I think this is ready for a final look @marcgarreau and @pipermerriam!

@kclowes kclowes requested a review from tmckenzie51 November 16, 2020 23:08
try:
(method_str, params), response_formatters = method.process_params(module, *args, **kwargs) # noqa: E501
except _UseExistingFilter as err:
return LogFilter(eth_module=module, filter_id=err.filter_id)
Copy link
Member

Choose a reason for hiding this comment

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

If we can't get this out, lets consider making it generic. I'd suggest doing that as a follow up to this PR.

Roughly speaking, it looks like we need an official way for a Method to short-circut the JSON-RPC request and to return a value that is generated locally. That could be using something like an exception like we did here, but I think that's probably not the best choice here because it feels like a heavy way to implement what is effectively an if/else.

We might need to return something more structured from process_params that we can then switch on.

request = method.preprocess_request(module, *args, **kwargs)
if request.is_json_rpc:
    ...  # do the actual request
elif request.has_static_response:
    ...  # directly return the response value.
else:
    ...  # dunno, maybe just raise Exception("Invariant")

I don't know if this is going to work, or be clean. Just my stab at what the API could look like. I think you'll be doing yourself and everyone on the team a favor to spend some time working on these APIs, especially in making them less cryptic in how they work.

@kclowes kclowes force-pushed the filter-to-method-ii branch 3 times, most recently from b357c90 to e6e4f9f Compare November 18, 2020 21:32
@kclowes kclowes force-pushed the filter-to-method-ii branch from e6e4f9f to 017f24d Compare November 18, 2020 22:15
@kclowes kclowes force-pushed the filter-to-method-ii branch from 81a1249 to 0c0b40a Compare November 19, 2020 20:04
@kclowes kclowes merged commit e176ce0 into ethereum:master Nov 19, 2020
@kclowes kclowes deleted the filter-to-method-ii branch November 19, 2020 22:01
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