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

[refactor] #1981, #4195: Refactor the event filters #4240

Merged
merged 13 commits into from
Mar 18, 2024

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented Jan 29, 2024

Description

  • rename the Filter trait to EventFilter
  • "manually" implement all the event filters without macros
  • remove all the filter combinators to provide a nicer and less nested API
  • derive *EventSet types with a derive macro
    • a bitmask that represents a set of event types
    • serialized to json as an array of strings
  • change the filter types naming scheme to be more logical:
    • AccountFilter -> AccountEventFilter (filters the event as a whole: by origin id and event type)
        - AccountEventFilter -> AccountEventSet (matches over a type of event)
  • flatten the event filter hierarchy (account event is a kind of domain event, but account event filter is not)
  • provide builder-like API for setting event filter fields

Linked issue

Closes #1981, #4195

Benefits

Checklist

  • write code documentation about using the new API
  • make ci pass

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jan 29, 2024
@DCNick3 DCNick3 changed the title Refactor the event filters [refactor] #1981, #4195: Refactor the event filters Jan 29, 2024
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from 4f83035 to 1686240 Compare January 29, 2024 07:48
@DCNick3
Copy link
Contributor Author

DCNick3 commented Feb 1, 2024

This new API doesn't allow to represent the filter "account created events within domain wonderland", which the previous system allowed.

This still can be implemented with the flattened approach by introducing typed id matchers. For example:

pub enum AccountIdMatcher {
  /// match any account id
  ByAny,
  /// match account id within a specific domain
  ByDomain(DomainId),
  /// match only a specific account id
  ByAccount(AccountId),
}

This functionality is somewhat related to the ValuePredicate::Identifiable predicate, though it treats ids as strings, which is unacceptable for the events IMO.

Anyways, I would prefer this to be implemented as a separate PR though

@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from c27784e to a1cdde5 Compare February 2, 2024 07:43
@DCNick3 DCNick3 marked this pull request as ready for review February 2, 2024 07:58
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from a1cdde5 to 2ceed8e Compare February 2, 2024 11:14
@mversic mversic self-assigned this Feb 5, 2024
tools/parity_scale_decoder/src/main.rs Outdated Show resolved Hide resolved
data_model/src/events/mod.rs Outdated Show resolved Hide resolved
data_model/Cargo.toml Outdated Show resolved Hide resolved
client/tests/integration/events/data.rs Outdated Show resolved Hide resolved
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch 2 times, most recently from 8f4a4de to 15ab511 Compare February 6, 2024 14:16
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from 15ab511 to b2cc3e0 Compare February 6, 2024 16:55
data_model/src/events/data/filters.rs Outdated Show resolved Hide resolved
data_model/src/events/notification.rs Outdated Show resolved Hide resolved
data_model/src/events/data/filters.rs Outdated Show resolved Hide resolved
client/tests/integration/triggers/data_trigger.rs Outdated Show resolved Hide resolved
data_model/src/events/data/filters.rs Outdated Show resolved Hide resolved
@DCNick3 DCNick3 self-assigned this Feb 7, 2024
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Should we consider case for filtering multiple events?

Like someone wants to count number of accounts, it would be possible by filtering events registered or unregistered (and increment or decrement counter respectively).

Current workaround would be to create multiple event streams.

data_model/src/events/data/filters.rs Show resolved Hide resolved
data_model/src/events/data/filters.rs Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor Author

DCNick3 commented Feb 16, 2024

Should we consider case for filtering multiple events?

I did initially dismiss this idea (because the previous design did not allow this), but I think it's actually pretty doable by using a derive macro like enumset or bitmask.

I'll try implementing this

@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from b2cc3e0 to 90d66ec Compare February 20, 2024 07:36
mversic
mversic previously approved these changes Mar 18, 2024
Erigara
Erigara previously approved these changes Mar 18, 2024
DCNick3 added 6 commits March 18, 2024 15:44
…he event filters

- rename the `Filter` trait to `EventFilter`
- "manually" implement all the event filters circumventing the macro
  - as a consequence, move all the filters to a separate module
- change the filter types naming scheme to be more logical:
  - AccountFilter -> AccountEventFilter (filters the event as a whole: by origin id and event type)
  - AccountEventFilter -> AccountEventMatcher (matches over a type of event)
- flattens the event filter hierarchy (account event is a kind of domain event, but account event filter is not)

Signed-off-by: Nikita Strygin <[email protected]>
… event filter derive macro

Signed-off-by: Nikita Strygin <[email protected]>
…he event filters (even further)

- Remove the FilterOpt combinator, fuse it with DataEventFilter
- Rename DataEntityFilter to DataEventFilter
- Add `Any` suffix to nested events matchers
- Add doc-comments to event filters & their variants

Signed-off-by: Nikita Strygin <[email protected]>
…terBox and TriggeringFilterBox to EventFilterBox and TriggeringEventFilterBox

To make it consistent with the renamed EventFilter trait

Signed-off-by: Nikita Strygin <[email protected]>
…n_for_events and friends implicitly convert event filter types

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Unify various event filter APIs, introduce a fluent builder API

- All event filters implement Debug, Clone, Eq, Ord
- All event filters (except TimeEventFilter) have a similar fluent builder API
    Event filter starts by accepting anything, with each method call limiting the accepted events
- Structs with hidden fields provide getters & builder APIs to create them
- Data event matchers are no longer prefixed with `By` to make them read better inside the `only_events` method call

Signed-off-by: Nikita Strygin <[email protected]>
@DCNick3 DCNick3 dismissed stale reviews from Erigara and mversic via 21f2f9d March 18, 2024 12:44
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from eaf54e7 to 21f2f9d Compare March 18, 2024 12:44
Erigara
Erigara previously approved these changes Mar 18, 2024
mversic
mversic previously approved these changes Mar 18, 2024
DCNick3 added 7 commits March 18, 2024 16:00
…r-iroha#3068: Add an event set proc macro

This proc macro generates a wrapper around a bit mask to specifying a set of events to match

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Use the auto-generated event sets instead of handwritten matchers

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: match more exhaustively over events in DataEventFilter

This will produce a compile error in case a new top-level data event is introduced. It will not catch new Domain event types (like Account, Asset, AssetDefinition). Achieving that will require listing all (even non-hierarchical) event types, which is probably too much boilerplate

Signed-off-by: Nikita Strygin <[email protected]>

[refactor] hyperledger-iroha#1981, hyperledger-iroha#4195, hyperledger-iroha#3068: match more exhaustively over events in DataEventFilter

This will produce a compile error in case a new top-level data event is introduced. It will not catch new Domain event types (like Account, Asset, AssetDefinition). Achieving that will require listing all (even non-hierarchical) event types, which is probably too much boilerplate

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Add event filters for PermissionTokenSchemaUpdate, Configuration and Executor events

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Remove some more redundancy when constructing event filters

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Update event filter builder methods naming

Follow the pattern of [event filter] for_X everywhere

Signed-off-by: Nikita Strygin <[email protected]>
…r-iroha#3068: Remove NotificationEvent

Instead of it, expose TriggerCompletedEvent as a top-level event, same as ExecuteTriggerEvent

Signed-off-by: Nikita Strygin <[email protected]>
@DCNick3 DCNick3 dismissed stale reviews from mversic and Erigara via 60065fe March 18, 2024 13:00
@DCNick3 DCNick3 force-pushed the event-filters-refactor branch from 21f2f9d to 60065fe Compare March 18, 2024 13:00
@DCNick3 DCNick3 merged commit c380c8b into hyperledger-iroha:iroha2-dev Mar 18, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants