Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fixed #895 #898

Merged
merged 1 commit into from
Apr 9, 2016
Merged

fixed #895 #898

merged 1 commit into from
Apr 9, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Apr 6, 2016

According to spec there are now two methods eth_getFitlerChanges and eth_getFilterLogs. But both of them are implemented in one method in parity.

docs

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Apr 6, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 6, 2016
@tomusdrw
Copy link
Collaborator

tomusdrw commented Apr 7, 2016

My understanding of docs is that eth_getFilterLogs should not reset polling state.

So shouldn't scenario like that be possible?

  1. eth_getFilterChanges (resets polling)
  2. eth_getFilterLogs (returns changes happened since 1.)
  3. eth_getFilterLogs (again returns changes happened since 1.)
  4. eth_getFilterChanges (returns changes happened since 1. and resets polling)
  5. eth_getFilterLogs (returns changes happened since 4.)

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Apr 7, 2016
@arkpar
Copy link
Collaborator

arkpar commented Apr 7, 2016

As far as I understand eth_getFilterLogs returns all logs since genesis,
eth_getFilterChanges returns logs since last call to eth_getFilterChanges

@tomusdrw
Copy link
Collaborator

tomusdrw commented Apr 7, 2016

But it is parametrized with filter id which doesn't make much sense if it always returns all logs.

@arkpar
Copy link
Collaborator

arkpar commented Apr 7, 2016

All logs matching a filter

@tomusdrw
Copy link
Collaborator

tomusdrw commented Apr 7, 2016

True. It makes sense now.

@gavofyork
Copy link
Contributor

@tomusdrw grumble gone?

@tomusdrw
Copy link
Collaborator

tomusdrw commented Apr 8, 2016

Mine is, but @arkpar has got on too. We can merge it as is, but cross check if this is really what was intended in docs.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 8, 2016
@gavofyork gavofyork merged commit 215973c into master Apr 9, 2016
@gavofyork gavofyork deleted the fixed_895 branch April 9, 2016 00:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants