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

feat: Events API enabled by default if EnableEthRPC is true #10077

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

geoff-vball
Copy link
Contributor

Related Issues

Proposed Changes

Sets a default path for the Events Database to be in the same directory as the tx hash database.
Enables all event API functionality if EnableEthRPC is set to true, otherwise it is always disabled.
Changes the Event Config to allow opt-out of these event APIs

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@geoff-vball geoff-vball requested a review from a team as a code owner January 19, 2023 21:59
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction LGTM.

Comment on lines 664 to 665
// EnableEthRPC enables APIs that can create and query filters for actor events as they are emitted.
// DisableRealTimeFilterAPI will disable those APIs if you want to opt-out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs

node/modules/actorevent.go Outdated Show resolved Hide resolved
@geoff-vball geoff-vball force-pushed the gstuart/eth-event-config branch 2 times, most recently from d3f0829 to 1a426ff Compare January 19, 2023 22:31
Copy link
Contributor

@travisperson travisperson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just need to update the comments on the EventConfig to correctly document the new Disable* values.

MaxFilters: 100,
MaxFilterResults: 10000,
MaxFilterHeightRange: 2880, // conservative limit of one day
Events: EventsConfig{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm @raulk - I thought we agreed to move this section under FEVM ? then the rename make sense cuz its FEVM.Events?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe this makes sense for now. While the Events subsystem is technically independent of FEVM, this is a user-facing configuration file and I think it leads to better product usability for node operators if all the new options they care about are nested within the Fevm group. We might need to rejigger this when builtin-actors are emitting events or with Wasm actors, but we can cross that bridge when we get to it, because I expect changes in the event subsystem anyway.

@jennijuju jennijuju merged commit b08b63d into release/v1.20.0 Jan 20, 2023
@jennijuju jennijuju deleted the gstuart/eth-event-config branch January 20, 2023 00:56
@jennijuju jennijuju added this to the Network v18 milestone Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants