Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(events): handle subscribe and get/fill cases separately #12586

Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 11, 2024

After reviewing #12585 I realised the remaining problem was this:

  • subscribe-type calls always wants future reverts but only historical reverts when a tipset is specified
  • get-type calls only want reverts when a tipset is specified

But we only have one "excludeReverted" argument to Install(). My first go at this was changing that argument to subscribe and using that to differentiate the behaviour and doing the tipsetKeyCid == cid.Undef check just for prefillFilter. But then I realised that for the non-subscribe calls we only ever install+remove the filter immediately, so the logic is kind of unnecessary if we just introduce a second method to deal with that case.

So now we have a Fill() in addition to an Install(), with the former not installing the filter, it just fills it if there's anything to fill and applies the tipsetKeyCid == cid.Undef directly itself. Then Install() can use Fill() for its historic stuff and then install the filter and ignore the reversion exclusion entirely because you always want those reverts in that case.

  • eth_subscribe can't get historic events, so it doesn't touch prefillFilter
  • SubscribeActorEventsRaw() can get historic events, but you can also specify a tipset (!) which is kind of silly, but perhaps you want to subscribe to the next tipset, which you have, but there aren't events for it yet .. that should work now, including reversions of it.
  • eth_newFilter is the interesting case because it's a hybrid API that can be used a bit like eth_subscribe but for polling, but it can also be used for just getting historic events like eth_getLogs so it needs this dual functionality supplied by Install().

@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Oct 11, 2024
@aarshkshah1992
Copy link
Contributor

@rvagg Just need to fix the lint error.

* subscribe-type calls always wants future reverts but only historical reverts
  when a tipset is specified
* get-type calls only want reverts when a tipset is specified

Add a new Fill() method to avoid the unnecessary install + remove step for get-
type calls.
@rvagg rvagg force-pushed the fix/fix/get-logs-reverted-events branch from b055a43 to 616bf37 Compare October 11, 2024 09:39
@rvagg rvagg merged commit d93e246 into fix/get-logs-reverted-events Oct 11, 2024
73 of 76 checks passed
@rvagg rvagg deleted the fix/fix/get-logs-reverted-events branch October 11, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

2 participants