Add event lookup for wildcard indexing - #135

JonoPrest · 2024-08-21T16:13:48Z

A new way of looking up events.

EventLookup supports:

Looking up by topic0 only if there is a single registration for the event
Looking up wildcard events without the address in contractAddress mapping
Looking up events when the address does exist in contractAddress mapping
Safe API for adding registrations with no collisions
Safe API for setting wildcard values after the fact without collisions

DZakh · 2024-08-22T08:31:28Z

-  let events = Js.Dict.empty()
+  let events: eventModLookup = EventLookup.empty()


When I was refactoring the chain workers, I wanted to change this place to pass eventModLookup chain worker functor factories - separately for each chain.

The reasoning is:

Remove the dependency of chain workers from Config - currently the events is the last thing left

The EventLookup structure is simpler and more optimised, since it's unique for each chain

It'll allow to have a different implementation of EventLookup for Fuel

DZakh · 2024-08-22T09:16:02Z

+  let rec find = (arr, fn, ~index=0) => {
+    open Belt
+    switch arr[index] {
+    | Some(item) => item->fn ? item->Some : arr->find(fn, ~index=index + 1)
+    | None => None
+    }
+  }


There should be Js.Array2.find or Belt.Array.keepMap

keepMap will not exit early and return a whole array of matching items though.

I'm not sure of Js.Array2.find, that probably does exit early. If it's native implementation it's probably best.

Yes, it was added in es6 and exits early

DZakh

Looks good. Just the change doesn't look very reliable. Also, I think the refactoring to have a separate event lookup for each chain should simplify the code and make the lookup operation more optimal.

  let config = Config.getGenerated()
  let config = RegisterHandlers.registerAllHandlers()

Also, I'd say we can merge it as it is for now and do the refactoring later.

DZakh · 2024-08-22T09:40:34Z

+  chains: array<ChainMap.Chain.t>,
+}
+type registeredEvent<'a> = {contract: contract, event: 'a, mutable isWildcard: bool}
+type eventsAtTopic0<'a> = Single(registeredEvent<'a>) | Multiple(dict<registeredEvent<'a>>)


Instead of the variant I think it's more convenient to work with the following data structure:

{ ids: array<string>, entities: dict<registeredEvent<'a>> }

What would IDs contain?

The keys of entities. Maybe worth calling keys

It's useful for:

Saving the Object.keys call

Cheaply get the number entities

Ok I understand. I think you mean events not entities right?

Also we can then just rather save an int for number of events. That can work. If it's just for checking single or multiple. That sounds good to me 👍🏼

Hmmm thinking about this:

switch eventsAtTopic0 { | {ids: [], events} => //None case | {ids: [singleId], events} => //Some single case | {ids: multipleIds, events} =>//Some multiple case }

I think you end up with the same cases with worse semantics. Not sure if it's an improvement.

Yes, something like this. Also, since there are no delitions doing it like this might be more convenient:

{ events: array<registeredEvent<'a>>, eventByContractName: dict<registeredEvent<'a>> }

And your example will be:

switch eventsAtTopic0 { | {events: []} => //None case | {events: [event]} => //Some single case | {events} =>//Some multiple case }

I think you end up with the same cases with worse semantics. Not sure if it's an improvement.
I agree that for the example above the semantics a little bit worse, even though I personally find it fine. There are some benefits coming along:

For some cases you don't need to have separate logic for Single and Multiple events, since now it's possible to do eventsAtTopic0. eventByContractName->Js.Dict.get without additional checks (also works for eventsAtTopic0.events)

No Js.Dict.keys and Js.Dict.values calls and additional object allocations for Single and Multiple variants. In our case this is not very important, since the code runs only on events registration, but still I think it's good to do our best with performance

Nice, cool that sounds like a good 👌🏼 will do.

DZakh · 2024-08-22T09:44:56Z

+      events
+      ->Js.Dict.values
+      ->Utils.Array.find(event =>
+        event.isWildcard && event.contract.chains->Utils.Array.includes(chain)


After refactoring, we discussed splitting the event lookup by each chain. We can keep the wildcard event in a separate field and get it without function calls and iterations. It might be worth doing the part of refactoring already since it feels like it'll simplify quite some code.

DZakh · 2024-08-22T10:06:09Z


 let populateChainQueuesWithRandomEvents = (~runTime=1000, ~maxBlockTime=15, ()) => {
-  let config = Config.getGenerated()
+  let config = RegisterHandlers.registerAllHandlers()


Why is this change? It reruns the EventHandlers code, which I don't think current with the current testing implementation.

Got you...

Basically the config can only be set after handlers registered. So I should change it rather to a function that if the ref is None, it runs registerAllHandlers and then returns the value else it can return the cached value.

I'm not too worried about the performance on a test though TBH.

Did you get a case when it's missing?

Yes, all the cases were missing.

Hmmm, weird how it worked before

No it's not weird, it's because I moved the Config.setGenerated into the register handlers function. That way you can be sure you have all the wildcard validation and entries on the config.

Ah, missed the change

DZakh · 2024-08-22T10:08:02Z

+  {{chain_id}},
+{{/each}}
+    ]->Belt.Array.map(chainId => ChainMap.Chain.makeUnsafe(~chainId))


Suggested change

{{chain_id}},

{{/each}}

]->Belt.Array.map(chainId => ChainMap.Chain.makeUnsafe(~chainId))

ChainMap.Chain.makeUnsafe(~chainId={{chain_id}}),

{{/each}}

]

Happy to take the suggestion but why?

This would expand into more boilerplate than before?

I don't have a strong opinion about it

Rename topic0 to sighash

JonoPrest requested a review from DZakh August 21, 2024 16:13

DZakh reviewed Aug 22, 2024

View reviewed changes

DZakh approved these changes Aug 22, 2024

View reviewed changes

JonoPrest force-pushed the jp/event-routing branch 3 times, most recently from 79978bc to 04dccd4 Compare August 28, 2024 10:42

JonoPrest added 2 commits August 30, 2024 10:09

Add event lookup for wildcard indexing

916bda2

Rename topic0 to sighash

Remove custom array find util

a1e2351

JonoPrest force-pushed the jp/event-routing branch from 04dccd4 to a1e2351 Compare August 30, 2024 08:11

JonoPrest enabled auto-merge (squash) August 30, 2024 08:11

JonoPrest merged commit af620a9 into main Aug 30, 2024

JonoPrest deleted the jp/event-routing branch August 30, 2024 08:14

		let events = Js.Dict.empty()
		let events: eventModLookup = EventLookup.empty()

Uh oh!

Conversation

JonoPrest commented Aug 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DZakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonoPrest Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonoPrest Aug 22, 2024 •

edited

Loading