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

event enums: Rename to conform with oasis-core #253

Merged
merged 4 commits into from
Dec 19, 2022
Merged

Conversation

mitjat
Copy link
Contributor

@mitjat mitjat commented Dec 15, 2022

This came up in https://github.com/oasisprotocol/oasis-indexer/pull/246/files#r1048033749

Unlike the indexer, oasis-core does not have an enum of all event types. Instead, events are represented in a nested struct that is used as a union type. So to represent a Transfer event, for example, oasis-core use an object like Event { Staking: { Transfer: { ... } } }. There's no explicit enumeration/names for event types. The indexer, OTOH, needs an enum so that clients can somehow say "give me only events of type X".

Enums were named somewhat arbitrarily. They were named after the last element of the "path" in the above composite object. In addition, the first element of the path (in practice, the originating tendermint app) was saved as a separate backend column in the DB, but it only ever makes sense to expose both backend and type at the same time, or neither.

This PR changes the enum value scheme as follows:

  • backend and type are merged into a single field, simplifying handling and understanding
  • JSON/CBOR names of fields are used, rather than Go-internal struct field names
  • When the "path" from the top-level Event to the leaf event struct has more than two elements, all elements are included in the enum name (e.g. staking.escrow.take)

analyzer/consensus/consensus.go Show resolved Hide resolved
analyzer/api.go Show resolved Hide resolved
Copy link
Collaborator

@Andrew7234 Andrew7234 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

k

analyzer/api.go Show resolved Hide resolved
Copy link
Contributor

@aefhm aefhm left a comment

Choose a reason for hiding this comment

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

(non-blocking) We don't foresee searching by backend?

@mitjat mitjat force-pushed the mitjat/event-type-enum branch from 3eaa525 to a86943a Compare December 16, 2022 18:17
@mitjat
Copy link
Contributor Author

mitjat commented Dec 16, 2022

(non-blocking) We don't foresee searching by backend?

I can't think of a use case, no. And if we need to for some niche reason, we can still do prefix matching on type.

@mitjat mitjat force-pushed the mitjat/api-events branch 3 times, most recently from dec98ed to f9d0b2d Compare December 17, 2022 08:46
Base automatically changed from mitjat/api-events to main December 17, 2022 08:51
@mitjat mitjat force-pushed the mitjat/event-type-enum branch from a86943a to 0384ad7 Compare December 19, 2022 13:01
@mitjat mitjat enabled auto-merge December 19, 2022 13:03
@mitjat mitjat disabled auto-merge December 19, 2022 13:17
@mitjat mitjat enabled auto-merge December 19, 2022 13:17
CI wouldn't trigger at the old HEAD
@mitjat mitjat merged commit 3cf110c into main Dec 19, 2022
@mitjat mitjat deleted the mitjat/event-type-enum branch December 19, 2022 15:57
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