feat: db event registry repository #42
Conversation
| registriesRepositories: { | ||
| eventRegistryRepository: IEventRegistryRepository; | ||
| strategyRegistryRepository: IStrategyRegistryRepository; |
There was a problem hiding this comment.
i think the previous way was ok. I...Registry should be de dependency not I...RegistryRepository. Lmk if iam missing something
There was a problem hiding this comment.
i've changed the way it works by creating an instance per chain but using the same repository, since it's not really necessary to load into memory the lastProcessedEvent of another chain (if we deploy a container per chain, this is more obvious to see).
if this makes sense, the same change applies for strategies (we create an instance per chain) but didn't want to mix and make the change for StrategyRegistry too. if we go for this approach, i'll make the change in another PR
if smth it's not clear, we can discuss it offline 😄
| const strategyRegistry = await InMemoryCachedStrategyRegistry.initialize( | ||
| new Logger({ className: "InMemoryCachedStrategyRegistry" }), | ||
| new DatabaseStrategyRegistry( | ||
| new Logger({ className: "DatabaseStrategyRegistry" }), | ||
| strategyRegistryRepository, | ||
| ), | ||
| ); | ||
| const eventsRegistry = new DatabaseEventRegistry( | ||
| new Logger({ className: "DatabaseEventRegistry" }), | ||
| eventRegistryRepository, | ||
| ); |
There was a problem hiding this comment.
is there a reason why you are doing this outside the SharedDependenciesService ?
| this.cache.set(chainId, { ...event, chainId }); | ||
| } | ||
|
|
||
| /** |
| private constructor( | ||
| private readonly logger: ILogger, | ||
| private readonly eventRegistry: IEventsRegistry, | ||
| cache: Map<ChainId, ProcessedEvent>, |
There was a problem hiding this comment.
Extremely minor, but maybe renaming it to initialEntries or initialCache and making it optional would make this a little bit more explanatory?
| cache: Map<ChainId, ProcessedEvent>, | |
| initialCache?: Map<ChainId, ProcessedEvent>, |
Nothing serious though, feel free to dismiss this comment if you prefer it the way it is.
There was a problem hiding this comment.
the goal of this class this is a proxied cache so it always is initialized with a pre-filled cache that fallbacks to the main registry implementation and given that constructor is private i think it's the same to have it optional or not
| const cachedEvent = this.cache.get(chainId); | ||
| if (cachedEvent) { | ||
| return cachedEvent; | ||
| } |
There was a problem hiding this comment.
I've got no idea on the business logic but worth asking; should you include some kind of TTL for this cache?
There was a problem hiding this comment.
is not really necessary since we're processing one event at time and saving on each iteration, tbh here we just save a really few calls to the DB
| // Verify no additional calls to underlying registry | ||
| expect(mockEventRegistry.getLastProcessedEvent).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
What if the other registry fails while saving?
There was a problem hiding this comment.
that's a really good question sir
cc @0xkenj1 we can discuss it offline if this is is a retriable error or not
| const CHAIN_ID_TYPE = "integer"; | ||
|
|
||
| await db.schema | ||
| .createTable("events") |
There was a problem hiding this comment.
Does using last_events as the table name fit better to this use case? It feels weird having a table named events with a chainId primary key.
| .createTable("events") | |
| .createTable("last_events") |
🤖 Linear
Closes GIT-188
Description
We want to persist the last event processed for each chain so we can safely stop and resume the indexer from where we left off.
EventRegistryRepositoryto store the last processed event per chainDBEventsRegistrythat interfaces with the repoDBEventRegistry(InMemoryCachedEventRegistry) and loads into memory the data stored on DB on initeventsforDBEventsRegistryrepoChecklist before requesting a review