Compatibility fixes/improvements for JSON/RPC filter polling

[peterbroadhurst]

Description

A set of changes that improve consistency of the Edge JSON/RPC surface area for block/log filters, with other Ethereum JSON/RPC client implementations. These proposed changes vary from clear fixes, to other items that are more subjective. So I'm leaving this in draft while I work through a set of tests with event streaming framework ethconnect - which is part of the Hyperledger FireFly project.

• Fix for eth_getFilterChanges returns a string containing serialized JSON array (not the JSON array) #640
• Consistent filter not found error string
• Allowing filters to exist for more than one minute, when being actively used

Changes include

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

If any users were depending on the invalid behavior of the JSON/RPC eth_getFilterChanges returning a string result (rather than an array).

This needs to stay in draft until a discussion happens on how to handle https://github.com/umbracle/ethgo, which has code that depends on the incorrect behavior. Resulting in this test failure:

Now resolved via umbracle/ethgo#209

--- FAIL: TestNewFilter_Logs (5.17s)
    logs_test.go:84: 
                Error Trace:    /Users/pbroadhurst/dev/polygon/polygon-edge/e2e/logs_test.go:84
                Error:          Received unexpected error:
                                json: cannot unmarshal array into Go value of type string
                Test:           TestNewFilter_Logs

Checklist

• I have assigned this PR to myself
• I have added at least 1 reviewer
• I have added the relevant labels
• I have updated the official documentation
• I have added sufficient documentation in code

Testing

• I have tested this code with the official test suite
• I have tested this code manually

Manual tests

Please complete this section if you ran manual tests for this functionality, otherwise delete it

Documentation update

Please link the documentation update PR in this section if it's present, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[peterbroadhurst]

I've found a follow-on issue, when a block filter returns an empty result set.
In this case we get [""] (rather than [].
Will extend the PR to cover this case

[zivkovicmilos]

Hey @peterbroadhurst,

Thank you for opening up a PR.

Can you please explain what kind of functionality is expected from the filter response, that Edge is not providing?
I just saw the opened issue where you detailed the behavior in Edge.

Also, I'd like to know what kind of problem exists with the ethgo package regarding this functionality.
We are moving towards dropping this package completely from Edge in the future as it's been more of a burden to maintain than our actual JSON-RPC interface.

If you can describe the problem here with ethgo, I'd be able to get a PR out and a fix merged very quickly as I'm one of the collaborators on that repo.

[peterbroadhurst]

Thanks you @zivkovicmilos - I'm sorry for the slow reply here.

The problem in the ethgo package is here: https://github.com/umbracle/ethgo/blob/8326bf607acf2ae4893128cacf205c67261cc411/jsonrpc/eth.go#L74-L86

This is actually working around the behavior currently on the JSON/RPC interface for eth_getFilterChanges, by unmarhsaling the string response into []*ethgo.Log within the client code.

So when the fix in this PR changes the JSON/RPC response to an array (rather than a string), the error occurs.

So the PR needed in ethgo is just to pass the []*ethgo.Log interface directly to e.c.Call() (rather than the string).

I had mean to submit this as a PR to ethgo, and got distracted by other priorities in the past couple of days. Very sorry about that 🙇 . I will do that now.

[peterbroadhurst]

I've raised umbracle/ethgo#209 - I'm going to check in a change while this PR is in draft, that updates the go.mod to redirect to that branch so I can check it allows the PR tests to pass.

[peterbroadhurst]

Sadly my attempt to test with my fork of ethgo failed, as the test run against the vendor directory (and go mod vendor didn't respect the replace directive it seems). So it might be necessary to work through umbracle/ethgo#209 before I can re-test here.

[peterbroadhurst]

Ok - with umbracle/ethgo#209 merged, I've updated the PR to pull in that commit of ethgo.

[peterbroadhurst]

I have read the CLA Document and I hereby sign the CLA

[zivkovicmilos]

Hey @peterbroadhurst,

Can you please pull in the latest develop branch into this PR so we can go ahead and merge it? 🙏

We've made some minor fixes in the meantime that you'll need to resolve.

[peterbroadhurst]

Thanks @zivkovicmilos - I've completed the merge, and added the change suggested by @Kourin1996

[zivkovicmilos]

Looks good 💯

Thank you for the contribution 🙏

[Kourin1996]

Looks good to me. Thank you for the PR!

[codecov]

Codecov Report

Merging #641 (6ffdbd4) into develop (8502125) will increase coverage by 0.05%.
The diff coverage is 89.28%.

@@             Coverage Diff             @@
##           develop     #641      +/-   ##
===========================================
+ Coverage    51.22%   51.27%   +0.05%     
===========================================
  Files          109      109              
  Lines        15816    15825       +9     
===========================================
+ Hits          8101     8115      +14     
+ Misses        7032     7028       -4     
+ Partials       683      682       -1     

Impacted Files	Coverage Δ
jsonrpc/filter_manager.go	76.96% <89.28%> (+1.45%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more