feat: all remaining event handlers for DVMD strategy #30

0xnigir1 · 2024-11-12T20:39:12Z

🤖 Linear

Closes GIT-170 GIT-155 GIT-156 GIT-160 GIT-167

Description

adds remaining Event Handlers for DVMD Direct Transfer Strategy
- TimestampsUpdated
- DistributionUpdated (common to all strategies)
- FundsDistributed (common to all strategies)
- RecipientStatusUpdated (common to all strategies)
- UpdatedRegistration

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-11-12T20:39:14Z

GIT-155 DistributionUpdated (Common)

GIT-170 DVMDDirectTransferStrategy

GIT-156 FundsDistributed (Common)

GIT-160 RecipientStatusUpdated (Common)

GIT-167 MerkleDistributionDirectTransferStrategy

0xkenj1

Just a smoll comments. Huge work here, sir! 👏💎

0xkenj1 · 2024-11-12T21:12:06Z

packages/processors/src/strategy/common/baseDistributionUpdated.handler.ts

+        if (!rawDistribution) {
+            logger.warn(`No matching distribution found for pointer: ${pointer}`);
+
+            throw new MetadataNotFound(`No matching distribution found for pointer: ${pointer}`);
+        }


Should MetadataProvider be responsible for throwing a "not found" error instead of having the handler doing it?

0xkenj1 · 2024-11-12T21:19:31Z

packages/processors/src/strategy/common/baseRecipientStatusUpdated.handler.ts

+        private readonly chainId: ChainId,
+        private readonly dependencies: Dependencies,
+    ) {
+        this.bitmap = new StatusesBitmap(256n, 4n);


values to build StatusesBitmap are always the same ?

yes, i followed original implementation. are constants values

0xkenj1 · 2024-11-12T21:33:26Z

packages/processors/src/strategy/helpers/applicationStatus.ts

+/**
+ * Checks if an application status index is valid (between 1 and 3)
+ * @see ApplicationStatus
+ */
+export function isValidApplicationStatus(status: number): boolean {
+    return status >= 1 && status <= 3;
+}


Don't like this hardcoding that much. But it is something that probably won't change.

What about using Object.keys(ApplicationStatus).length ?

didn't follow exactly your idea, you mean to use the length as the upper bound limit?

0xkenj1 · 2024-11-12T21:44:56Z

...gy/donationVotingMerkleDistributionDirectTransfer/handlers/timestampsUpdated.handler.spec.ts

+    // it("correctly convert timestamps to Date objects", async () => {
+    //     const timestamps = {
+    //         registrationStartTime: 1704067200n, // 2024-01-01 00:00:00
+    //         registrationEndTime: 1704153600n, // 2024-01-02 00:00:00
+    //         allocationStartTime: 1704240000n, // 2024-01-03 00:00:00
+    //         allocationEndTime: 1704326400n, // 2024-01-04 00:00:00
+    //     };
+
+    //     mockEvent = createMockEvent({
+    //         params: timestamps,
+    //     });
+    //     const mockRound = { id: "round1" } as Round;
+
+    //     vi.spyOn(mockRoundRepository, "getRoundByStrategyAddress").mockResolvedValue(mockRound);
+
+    //     handler = new DVMDTimestampsUpdatedHandler(mockEvent, chainId, {
+    //         roundRepository: mockRoundRepository,
+    //     });
+
+    //     const result = await handler.handle();
+
+    //     expect(result.length).toBe(1);
+    //     const changeset = result[0] as {
+    //         type: "UpdateRound";
+    //         args: { chainId: ChainId; roundId: string; round: PartialRound };
+    //     };
+
+    //     expect(changeset.type).toBe("UpdateRound");
+    //     expect(changeset.args.chainId).toBe(chainId);
+    //     expect(changeset.args.roundId).toBe("round1");
+    //     expect(changeset.args.round).toBeDefined();
+
+    //     const partialRound = changeset.args.round;
+
+    //     expect(partialRound.applicationsStartTime).toEqual(new Date("2024-01-01T00:00:00.000Z"));
+    //     expect(partialRound.applicationsEndTime).toEqual(new Date("2024-01-02T00:00:00.000Z"));
+    //     expect(partialRound.donationsStartTime).toEqual(new Date("2024-01-03T00:00:00.000Z"));
+    //     expect(partialRound.donationsEndTime).toEqual(new Date("2024-01-04T00:00:00.000Z"));
+    // });
+});


why is this commented?

0xkenj1 · 2024-11-12T21:45:58Z

...gy/donationVotingMerkleDistributionDirectTransfer/handlers/timestampsUpdated.handler.spec.ts

+        const timestamps = {
+            registrationStartTime: 1704067200n, // 2024-01-01 00:00:00
+            registrationEndTime: 1704153600n, // 2024-01-02 00:00:00
+            allocationStartTime: 1704240000n, // 2024-01-03 00:00:00
+            allocationEndTime: 1704326400n, // 2024-01-04 00:00:00
+        };


may we check if envio is actually returning this params of the event as string or number ?

you're right here, it's string. should we treat as string or have the indexer-client parse it and coerce to bigint?

jahabeebs · 2024-11-13T03:56:41Z

packages/processors/src/schemas/matchingDistribution.ts

+
+export type MatchingDistribution = z.infer<typeof MatchingDistributionSchema>;
+
+// handle ethers bigint serialization


do you mean viem?

just copy-pasted the comment, will delete it

jahabeebs · 2024-11-13T04:00:32Z

packages/processors/src/schemas/matchingDistribution.ts

+            projectPayoutAddress: z.string(),
+            projectId: z.string(),
+            projectName: z.string(),
+            matchPoolPercentage: z.coerce.number(),


this will coerce empty strings to 0 rather than fail validation--just checking if you're ok with this. If you'd prefer to fail validation you can check out this: colinhacks/zod#2461 (comment)

interesting 👀 , have to say that '' being 0 is kinda unexpected

jahabeebs · 2024-11-13T04:03:49Z

packages/processors/src/strategy/common/baseDistributionUpdated.handler.ts

+        private readonly dependencies: Dependencies,
+    ) {}
+
+    async handle(): Promise<Changeset[]> {


inheritdoc?

jahabeebs · 2024-11-13T04:09:24Z

packages/processors/src/strategy/common/baseRecipientStatusUpdated.handler.ts

+
+type ApplicationUpdate = {
+    application: Application;
+    status: number;


if the status can only be a number between 1-3 maybe we should validate it rather than just typing it as number

there's kinda a weird thing because the original implementation expected more statutes but currently only 3 of the status flow are in production. however the status on Index 0 is not one of them so I rather keep this as it is for now

jahabeebs · 2024-11-13T04:14:37Z

...ategy/donationVotingMerkleDistributionDirectTransfer/handlers/updatedRegistration.handler.ts

+        private readonly dependencies: Dependencies,
+    ) {}
+
+    async handle(): Promise<Changeset[]> {


inherit doc?

jahabeebs · 2024-11-13T04:20:47Z

packages/processors/test/strategy/common/baseDistributionUpdated.handler.spec.ts

+        );
+    });
+
+    it("should handle empty matching distribution array", async () => {


jahabeebs · 2024-11-13T04:24:58Z

packages/repository/src/types/round.types.ts

@@ -40,7 +40,7 @@ export type Round = {
    strategyId: string;
    strategyName: string;
    readyForPayoutTransaction: string | null;
-    matchingDistribution: MatchingDistribution | null;
+    matchingDistribution: MatchingDistribution[] | null;


maybe should be called matchingDistributions if it's an array now

it was always an array, i forgot the [ ], however i'll keep the field name so we don't break the current DB schema and we have the same name across all the app

feat: all remaining event handlers for DVMD strategy

b442db8

0xnigir1 requested review from jahabeebs, 0xkenj1 and 0xyaco November 12, 2024 20:39

0xkenj1 reviewed Nov 12, 2024

View reviewed changes

feat: write XXOrThrow methods on repository

4187c7c

jahabeebs reviewed Nov 13, 2024

View reviewed changes

0xnigir1 added 2 commits November 13, 2024 16:07

fix: string type on Envio events & repository jsonb field issue

980f1f6

fix: pr comments

2bef2e4

0xnigir1 requested review from jahabeebs and 0xkenj1 November 13, 2024 19:22

0xkenj1 previously approved these changes Nov 13, 2024

View reviewed changes

jahabeebs previously approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'dev' into feat/dvmd-remaining-handlers

b0d96ab

0xnigir1 dismissed stale reviews from jahabeebs and 0xkenj1 via b0d96ab November 13, 2024 21:44

0xnigir1 requested review from 0xkenj1 and jahabeebs November 13, 2024 21:45

0xkenj1 approved these changes Nov 13, 2024

View reviewed changes

jahabeebs approved these changes Nov 13, 2024

View reviewed changes

0xnigir1 merged commit bd5be45 into dev Nov 13, 2024
6 checks passed

0xnigir1 deleted the feat/dvmd-remaining-handlers branch November 13, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: all remaining event handlers for DVMD strategy #30

feat: all remaining event handlers for DVMD strategy #30

0xnigir1 commented Nov 12, 2024 •

edited

Loading

linear bot commented Nov 12, 2024

0xkenj1 left a comment

0xkenj1 Nov 12, 2024

0xkenj1 Nov 12, 2024

0xnigir1 Nov 13, 2024

0xkenj1 Nov 12, 2024

0xnigir1 Nov 13, 2024

0xkenj1 Nov 12, 2024

0xnigir1 Nov 13, 2024

0xkenj1 Nov 12, 2024

0xnigir1 Nov 13, 2024

jahabeebs Nov 13, 2024

0xnigir1 Nov 13, 2024

jahabeebs Nov 13, 2024

0xnigir1 Nov 13, 2024 •

edited

Loading

jahabeebs Nov 13, 2024

jahabeebs Nov 13, 2024

0xnigir1 Nov 13, 2024

jahabeebs Nov 13, 2024

jahabeebs Nov 13, 2024

jahabeebs Nov 13, 2024

0xnigir1 Nov 13, 2024


		export type MatchingDistribution = z.infer<typeof MatchingDistributionSchema>;

		// handle ethers bigint serialization

feat: all remaining event handlers for DVMD strategy #30

feat: all remaining event handlers for DVMD strategy #30

Conversation

0xnigir1 commented Nov 12, 2024 • edited Loading

🤖 Linear

Description

Checklist before requesting a review

linear bot commented Nov 12, 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

0xnigir1 Nov 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

0xnigir1 commented Nov 12, 2024 •

edited

Loading

0xnigir1 Nov 13, 2024 •

edited

Loading