test: support eth_*Filter RPCs #136
test: support eth_*Filter RPCs #136JonathanOppenheimer wants to merge 12 commits intoalarso16/get-logsfrom
eth_*Filter RPCs #136Conversation
JonathanOppenheimer
requested review from
ARR4N and
StephenButtolph
as code owners
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
JonathanOppenheimer
added
the
DO NOT MERGE
JonathanOppenheimer
changed the title
eth_filters RPCs
JonathanOppenheimer
changed the title
eth_filters RPCs eth_filter RPCs
JonathanOppenheimer
changed the title
eth_filter RPCs eth_*filter RPCs
JonathanOppenheimer
changed the title
eth_*filter RPCs eth_*Filter RPCs
| t.Helper() | ||
| var filterID string | ||
| require.NoError(t, sut.CallContext(ctx, &filterID, method, args...)) | ||
| require.NotEmpty(t, filterID) |
There was a problem hiding this comment.
| require.NotEmpty(t, filterID) | |
| require.NotEmpty(t, filterID) | |
| t.Cleanup(func() { | |
| uinstallFilter(t, filterID) | |
| }) |
I think this would be fine, since it matches the exact functionality you implemented, but it doesn't give you the chance to forget the defer statement
There was a problem hiding this comment.
While this is a good change, we can't call t.Run (which uninstall filter uses), inside of t.cleanup. I'm going to leave as is, unless you have another suggestion (I would like to use testRPC in uninstallFilter)
Co-authored-by: Austin Larson <78000745+alarso16@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
JonathanOppenheimer
removed
the
DO NOT MERGE
| createFilter := func(t *testing.T, ctx context.Context, sut *SUT, method string, args ...any) string { | ||
| t.Helper() | ||
| var filterID string | ||
| require.NoError(t, sut.CallContext(ctx, &filterID, method, args...)) |
There was a problem hiding this comment.
@StephenButtolph -- the CallContext usage here feels unavoidable due to the need to capture the filterID. Is it acceptable here?
| verifyFilterDeleted := func(t *testing.T, ctx context.Context, sut *SUT, filterID string) { | ||
| t.Helper() | ||
| var changes []common.Hash | ||
| require.ErrorContains(t, sut.CallContext(ctx, &changes, "eth_getFilterChanges", filterID), "filter not found") |
There was a problem hiding this comment.
Similarly, I want to verify and error. we could add a new RPC helper that does error checking rather than success cases, but that would be an additional indirection.
| verifyFilterDeleted(t, ctx, sut, filterID) | ||
| }) | ||
|
|
||
| t.Run("newPendingTransactionFilter", func(t *testing.T) { |
There was a problem hiding this comment.
I think this is appropriately set up. I could turn it into a big test table, but then one of the test table entries would be a function which is a smell I think.
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
| // - eth_getRawTransactionByBlockNumberAndIndex | ||
| // - eth_getRawTransactionByHash | ||
| // - eth_pendingTransactions | ||
| // Standard Ethereum node APIS: |
There was a problem hiding this comment.
This is incorrect placed
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
2 participants
Adds support for the standard Ethereum JSON-RPC filter APIs:
eth_newFilter- Create log filter with optional criteria (address, topics)eth_newBlockFilter- Create filter for new blockseth_newPendingTransactionFilter- Create filter for pending transactionseth_getFilterChanges- Poll for new events since last polleth_getFilterLogs- Get all logs matching a filtereth_uninstallFilter- Remove a filterThis should not be merged until after #120 is merged.