feat: persisting strategy registry into db #40

0xnigir1 · 2024-12-11T20:19:04Z

🤖 Linear

Description

We want to persist strategyIds on PoolCreated events so we could later pre-process all of the past events when we implement in code a new Strategy Handler.

Create StrategyRepository to store seen strategyIds with their corresponding address and whether it was handled or not
DBStrategyRegistry that interfaces with the repo
a Proxied version that caches in memory and uses the DBStrategyRegistry (InMemoryCachedStrategyRegistry)
update Processing app to use the InMemoryCachedStrategyRegistry

Checklist before requesting a review

I have conducted a self-review of my code.
I have conducted a QA.
If it is a core feature, I have included comprehensive tests.

linear · 2024-12-11T20:19:07Z

GIT-185 Create DBStrategyRegistry

0xkenj1

GJ ser, this is a lot of work. Just small comments :)

0xkenj1 · 2024-12-13T00:07:56Z

apps/processing/src/services/processing.service.ts

0xkenj1 · 2024-12-13T00:08:25Z

apps/processing/src/services/sharedDependencies.service.ts

+    DatabaseStrategyRegistry,
+    IEventsRegistry,
+    InMemoryCachedStrategyRegistry,
+} from "@grants-stack-indexer/data-flow/dist/src/internal.js";


is pointing to dist

fixed: aa3ee27

0xkenj1 · 2024-12-13T00:08:53Z

apps/processing/src/services/sharedDependencies.service.ts

@@ -13,6 +18,7 @@ import {
    KyselyDonationRepository,
    KyselyProjectRepository,
    KyselyRoundRepository,
+    KyselyStrategyRepository,


StrategyRegistryRepository

0xkenj1 · 2024-12-13T00:15:58Z

apps/processing/src/services/sharedDependencies.service.ts

+            new Logger({ className: "InMemoryCachedStrategyRegistry" }),
+            new DatabaseStrategyRegistry(
+                new Logger({ className: "DatabaseStrategyRegistry" }),


I am a little concerned about using multiple logger instances. I think it adds tons of overhead, think about a logger that sends logs to a server or prints all the logs into a file, for each instance you will have a connection (1st case) or a dedicated file (2nd case) correct me if iam wrong pls.

i would love to hear @0xyaco 's opinion about having multiple logger instances across the application

mmm i see what you're pointing, we can better discuss it on the Error handling and Logging milestone but a solution could be a Context object on each call so we know which file or chain emitted a log?

0xkenj1 · 2024-12-13T00:18:32Z

packages/data-flow/README.md


 The `StrategyRegistry` stores strategy IDs to populate strategy events with them given the Strategy address.
+There are 3 implementations:
+
+-   `InMemoryStrategyRegistry`: stores map in-memory


maybe we should get rid of this one since it will be no longer used and adds noise here

okii makes sense

fixed: aa3ee27

0xkenj1 · 2024-12-13T00:30:52Z

packages/data-flow/src/registries/cachedStrategyRegistry.ts

+        private readonly strategyRegistry: IStrategyRegistry,
+        cache: Map<ChainId, Map<Address, Strategy>>,
+    ) {
+        this.cache = structuredClone(cache);


Didn't know that structuredClone exist.

Why do we need to pass cache as parameter instead of having it contained in the class ?

the cache is built in the static async constructor, for the class to be instantiated with the pre-filled cache elements, it needs to receive it in constructor

0xkenj1 · 2024-12-13T00:31:48Z

packages/data-flow/src/registries/cachedStrategyRegistry.ts

+        strategyRegistry: IStrategyRegistry,
+    ): Promise<InMemoryCachedStrategyRegistry> {
+        const strategies = await strategyRegistry.getStrategies();
+        const cache = new Map<ChainId, Map<Address, Strategy>>();


i think here is my answer xd

0xkenj1 · 2024-12-13T00:51:57Z

scripts/migrations/README.md

@@ -41,9 +37,13 @@ pnpm install
 To apply all pending migrations:

 ```bash
-pnpm script:db:migrate
+pnpm db:migrate --schema=schema_name


should we rollback this ?

nono, user can still name a different schema if they want (i mean i think flexibility is cool to have) but default is public schema

0xkenj1 · 2024-12-13T00:52:11Z

scripts/migrations/README.md

+Optional arguments:
+
+-   `--schema` or `-s`: Database schema name where migrations are applied. Defaults to `public`.
+


is no longer needed right ?

0xkenj1 · 2024-12-13T00:52:17Z

scripts/migrations/README.md

@@ -57,7 +57,7 @@ This will:
 To completely reset the database schema:

 ```bash
-pnpm script:db:reset
+pnpm db:reset --schema=schema_name


jahabeebs · 2024-12-13T01:19:41Z

apps/processing/test/unit/sharedDependencies.service.spec.ts

@@ -33,6 +38,27 @@ vi.mock("@grants-stack-indexer/indexer-client", () => ({
    EnvioIndexerClient: vi.fn(),
 }));

+// Update the mock to handle async initialization


Is this a TODO?

not a TODO, neither a needed comment. cursor wrote it my bad

jahabeebs · 2024-12-13T01:23:48Z

packages/data-flow/src/interfaces/strategyRegistry.interface.ts

     */
-    saveStrategyId(strategyAddress: Address, strategyId: Hex): Promise<void>;
+    getStrategies(params?: { handled?: boolean; chainId?: ChainId }): Promise<Strategy[]>;


Similar to what Kenji said might even consider having a separate filterStrategies

like a different method? i'm not a big fan of this approach where custom actions are mapped to specific methods. i think filters are optional conditions applied to the general case query here

jahabeebs · 2024-12-13T04:56:34Z

packages/data-flow/src/registries/cachedStrategyRegistry.ts

+        this.cache = structuredClone(cache);
+    }
+
+    async getStrategies(params?: { handled?: boolean; chainId?: ChainId }): Promise<Strategy[]> {


Maybe consider separating this out into getAllStrategies and get filteredStrategies rather than the optional params

jahabeebs · 2024-12-13T04:58:06Z

packages/data-flow/src/registries/cachedStrategyRegistry.ts

+        return strategy;
+    }
+
+    async saveStrategyId(


Not sure if you need inherit docs comment in this file?

missing yeah thanks sir

jahabeebs · 2024-12-13T04:59:16Z

packages/data-flow/src/registries/strategyRegistry.ts

+ */
+export class InMemoryStrategyRegistry implements IStrategyRegistry {
+    private strategiesMap: Map<ChainId, Map<Address, Strategy>> = new Map();
+    constructor(private logger: ILogger) {}


Should be be initializing a logger/getting the instance here? 🤔

jahabeebs · 2024-12-13T05:05:07Z

packages/repository/src/repositories/kysely/strategy.repository.ts

+    ) {}
+
+    async getStrategyByChainIdAndAddress(
+        chainId: ChainId,


Docs for this file/inherit docs?

jahabeebs · 2024-12-13T05:06:53Z

scripts/migrations/src/migrations/20241210T175001_strategy_registry.ts

+
+    const schema = getSchemaName(db.schema);
+
+    console.log("schema", schema);


Is this console log intentional?

jahabeebs · 2024-12-13T05:10:12Z

scripts/migrations/src/schemas/index.ts

@@ -2,7 +2,6 @@ import { z } from "zod";

 const dbEnvSchema = z.object({
    DATABASE_URL: z.string().url(),
-    DATABASE_SCHEMA: z.string().min(1),


If it's optional maybe could leave it in with .optional() just to validate that the user knows it's a schema name and doesn't try adding an actual schema

this one is no longer needed, schema is now script arg not an env variable, that's the difference. no longer a secret xd

jahabeebs

bien

0xnigir1 added 4 commits December 10, 2024 17:42

feat: db strategy repository, registry, cached version

b3095ad

feat: update migrations script to handle different schemas

a851079

test: add unit tests

6b7e3ab

docs: update readme files

3978835

0xnigir1 requested review from 0xkenj1, jahabeebs and 0xyaco December 11, 2024 20:19

0xnigir1 self-assigned this Dec 11, 2024

feat: revert multiple schemas setup

6adb666

0xkenj1 reviewed Dec 13, 2024

View reviewed changes

jahabeebs reviewed Dec 13, 2024

View reviewed changes

0xnigir1 added 3 commits December 13, 2024 15:02

fix: imports, renamings and natspecs

aa3ee27

fix: rename class and argument

fe62481

feat: improve logic on saving strategy id

9223013

0xnigir1 requested review from 0xkenj1 and jahabeebs December 13, 2024 22:58

jahabeebs approved these changes Dec 14, 2024

View reviewed changes

		Optional arguments:

		- `--schema` or `-s`: Database schema name where migrations are applied. Defaults to `public`.


		const schema = getSchemaName(db.schema);

		console.log("schema", schema);

feat: persisting strategy registry into db #40

Are you sure you want to change the base?

feat: persisting strategy registry into db #40

Conversation

0xnigir1 commented Dec 11, 2024 • edited Loading

🤖 Linear

Description

Checklist before requesting a review

linear bot commented Dec 11, 2024

0xkenj1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnigir1 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahabeebs left a comment

Choose a reason for hiding this comment

0xnigir1 commented Dec 11, 2024 •

edited

Loading

0xnigir1 Dec 13, 2024 •

edited

Loading