move contract.eventFilter to contract.events.*.createFilter #702

dylanjw · 2018-03-17T00:04:25Z

What was wrong?

The contract.events.* api doesnt do very much yet.

How was it fixed?

Add deprecation warning to contract.eventFilter, and create a new method contract.events.<event_name>.createFilter()

Cute Animal Picture

pipermerriam

Leaving this one comment here about changing the API.

pipermerriam · 2018-03-17T01:25:42Z

docs/contracts.rst

@@ -129,7 +129,7 @@ Each Contract Factory exposes the following methods.

    Returns the transaction hash for the deploy transaction.

-.. py:classmethod:: Contract.eventFilter(event_name, filter_params=None)
+.. py:classmethod:: Contract.events.<event name>.createFilter(filter_params=None)


Just realizing we're using a legacy API that was never really re-evaluated for whether we could improve it.

What if instead of filter_params as a single argument you could pass in all of the individual values allowed in filter params, probably as keyword arguments.

c.events.Transfer.createFilter(topics=..., args=..., _from=..., fromBlock=..., toBlock=...)

Note that _from kind of awkward due to the collision with the from keyword in python so maybe we could even change that one to address=...?

What if we kept the filter param dict as an option to give raw arguments, and had the new kwargs pass through any argument processing. For example, if providing topics through the keyword arg, you wouldnt need to include the event signature as the first arg and could enter the other topics as strings or integers values, rather than hex string representations.

Also, the AND or OR syntax for topics is a little odd:

from the json-rpc doc:

A note on specifying topic filters:

Topics are order-dependent. A transaction with a log with topics [A, B] will be matched by the following topic filters:

[] "anything"
[A] "A in first position (and anything after)"
[null, B] "anything in first position AND B in second position (and anything after)"
[A, B] "A in first position AND B in second position (and anything after)"
[[A, B], [A, B]] "(A OR B) in first position AND (A OR B) in second position (and anything after)"

Could this be better? Maybe a single string of topics with "," for AND (e.g, followed by...) "|" for OR and "*" for ANY? The above examples would look like:

"*"
"A"
"*, B"
"A, B"
"A|B, A|B"

Im not sure that this is an improvement. It would be cool if the "and anything after" matching could be controlled with the syntax, but I dont think there is a way to have "and nothing after".

Maybe rather than a DLS (which is what the string based version you proposed) we could do this with a custom helper type.

>>> t(A) & t(B)) , t(A) | t(B) [[A, B], [A, B]]

This fancy new t class would have custom written __and__ and __or__ methods which evaluated to the list form for the topic.

I just looked up the spec and I don't think my fancy new type idea will work.

So far, when I've seen people get stuck on events, it's not because they are confused by the topic filters. I think we should keep transparency to the json-rpc layer where we're not sure we have a solution that's a lot better (and fixing something that really needs to be fixed).

Some examples of things that I have seen trip people up: 1) default fromBlock value is latest and the event they're looking for already happened, 2) disappearing filters on the client.

Some crazy options for the first:

deviate from the fromBlock default in json-rpc, so that it is 0

require that people specify a fromBlock when creating a filter?

always return a result with log queries, with a value like {fromBlock: 1234, toBlock:1237, events: [], allEvents: [{event-that-did-not-match}]}, so that it's clear what blocks are being inspected. There are negative performance implications, but maybe you could opt out of that behavior for production.

#551 is our crazy idea for 2) - which I'm on board with

I take it back: #188 is an example of confusion on filter topics. Although I'd still argue it's less common that the others.

I like the idea of requiring a fromBlock, and including some metadata about the search in the results. Im less keen on adding the non-matching events to the filter results.

Forcing users to think about a choice for the fromBlock parameter should be enough. I would prefer keeping the default as latest than changing to 0. Typically you need to set up an event listener before catching events. It seems like a more unique situation to listen for events from the past. But I guess thats irrelevant if we make it a mandatory field.

I added an explanation of how the topic matching works to the doc, so maybe thats enough for #188

pipermerriam · 2018-03-26T17:20:39Z

docs/contracts.rst

@@ -129,7 +129,7 @@ Each Contract Factory exposes the following methods.

    Returns the transaction hash for the deploy transaction.

-.. py:classmethod:: Contract.eventFilter(event_name, filter_params=None)


Given that this method is still present, we should leave it in the documentation and just mark it as having been deprecated.

.. warning:: The `Contract.eventFilter` method has been deprecated in favor of .......

pipermerriam · 2018-03-26T17:24:03Z

web3/providers/eth_tester/middleware.py

@@ -35,7 +36,11 @@ def is_named_block(value):
    return value in {"latest", "earliest", "pending"}


-to_integer_if_hex = apply_formatter_if(is_string, hex_to_integer)
+def is_str_is_hex(value):


Would naming this is_hexstr be more consistent with our existing naming convention?

carver · 2018-03-27T19:43:46Z

I've seen at least one person who wants to catch all events coming out of a contract, so I'm not so sure we want to drop contract.eventFilter(). But I don't feel strongly about it, especially since we're not removing the feature for a while. We can always un-deprecate it later.

Edit: (although, I realize now that the event name was required in contract.eventFilter() anyway)

pipermerriam · 2018-04-05T23:06:14Z

web3/contract.py

+    @combomethod
+    def createFilter(
+            self, *,  # PEP 3102
+            argument_filters=dict(),


This should get fixed. Mutable values as defaults for function arguments is an antipattern:

http://docs.python-guide.org/en/latest/writing/gotchas/

This specific case, I don't think it causes any problems, but still something we should avoid.

pipermerriam · 2018-04-05T23:07:11Z

web3/contract.py

+            fromBlock=None,
+            toBlock="latest",
+            address=None,
+            topics=list()):


Same here, should be changed to None and then defaulted to an empty list conditionally if it is None

ti6oti6o · 2018-04-06T03:07:17Z

I think I posted the below in the wrong spot (apologies for that):
createFilter does not yield the same result as eventFilter...For example, the below gives no results with createFilter and some results with eventFilter.... Why is that? Below is rinkeby and web3(4.0.0)

myfilter_new=contract_instance.events.Deposit.createFilter(fromBlock=2058615)
myfilter_old=contract_instance.eventFilter('Deposit',{'fromBlock':2058615,'toBlock':'latest'})
print("     filterDeposit (OLD METHOD)--",myfilter_old.get_all_entries())
print("     filterDeposit (NEW METHOD)--",myfilter_new.get_all_entries())

dylanjw · 2018-04-06T03:33:17Z

@ti6oti6o Can you have a look at this comment, in a thread discussing issues with the createFilter API. Can you post enough information to reproduce the issue you are experiencing with createFilter?

pipermerriam · 2018-04-06T16:33:35Z

I've seen at least one person who wants to catch all events coming out of a contract

This strikes me as something that deserves it's own dedicated API:

contract.all_events.createFilter(...)

move contract.eventFilter to contract.events.*.createFilter

698cf6e

dylanjw force-pushed the add-to-contract-events-api branch from 704a254 to 698cf6e Compare March 17, 2018 00:07

pipermerriam reviewed Mar 17, 2018

View reviewed changes

dylanjw added 2 commits March 20, 2018 15:23

Update arguments for createFilter

a2765e9

Update tests with createFilter argument changes

f82a1b9

dylanjw force-pushed the add-to-contract-events-api branch from 3e8cec9 to f82a1b9 Compare March 21, 2018 20:15

Make lint

5f4977c

dylanjw force-pushed the add-to-contract-events-api branch from e0dba10 to 5f4977c Compare March 23, 2018 21:30

pipermerriam approved these changes Mar 26, 2018

View reviewed changes

dylanjw and others added 4 commits March 26, 2018 14:58

Update createFilter doc

17e23ce

Make contract address default for createEvent

24b6b47

Merge branch 'master' into add-to-contract-events-api

9afca16

Merge branch 'master' into add-to-contract-events-api

3eddd2a

carver merged commit 0b6d737 into ethereum:master Mar 27, 2018

carver mentioned this pull request Apr 4, 2018

Empty event log #730

Closed

pipermerriam reviewed Apr 5, 2018

View reviewed changes

ethereum deleted a comment from ti6oti6o Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move contract.eventFilter to contract.events.*.createFilter #702

move contract.eventFilter to contract.events.*.createFilter #702

dylanjw commented Mar 17, 2018

pipermerriam left a comment

pipermerriam Mar 17, 2018

dylanjw Mar 17, 2018 •

edited

Loading

pipermerriam Mar 18, 2018

pipermerriam Mar 18, 2018

carver Mar 20, 2018 •

edited

Loading

carver Mar 20, 2018

dylanjw Mar 20, 2018 •

edited

Loading

dylanjw Mar 20, 2018

pipermerriam Mar 26, 2018

pipermerriam Mar 26, 2018

carver commented Mar 27, 2018 •

edited

Loading

pipermerriam Apr 5, 2018

pipermerriam Apr 5, 2018

ti6oti6o commented Apr 6, 2018

dylanjw commented Apr 6, 2018 •

edited

Loading

pipermerriam commented Apr 6, 2018

		@@ -129,7 +129,7 @@ Each Contract Factory exposes the following methods.

		Returns the transaction hash for the deploy transaction.

		.. py:classmethod:: Contract.eventFilter(event_name, filter_params=None)

move contract.eventFilter to contract.events.*.createFilter #702

move contract.eventFilter to contract.events.*.createFilter #702

Conversation

dylanjw commented Mar 17, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjw Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjw Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti6oti6o commented Apr 6, 2018

dylanjw commented Apr 6, 2018 • edited Loading

pipermerriam commented Apr 6, 2018

dylanjw Mar 17, 2018 •

edited

Loading

carver Mar 20, 2018 •

edited

Loading

dylanjw Mar 20, 2018 •

edited

Loading

carver commented Mar 27, 2018 •

edited

Loading

dylanjw commented Apr 6, 2018 •

edited

Loading