feat: base strategy processor & dvmd strategy factory #16

0xnigir1 · 2024-10-24T14:38:43Z

🤖 Linear

Closes GIT-60 GIT-61 GIT-62

Description

StrategyHandlerFactory to create the proper Strategy handler based on strategyId
BaseStrategyHandler abstract class w/ default implementation for fetch functions (some strategies require custom logic)
DVMDDirectTransferStrategy handler
Distributed and Registered handlers for DVMD strategy (PR Feat/dvmd dt event handlers #17)
refactored TODOs from PoolCreatedHandler

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-10-24T14:38:45Z

GIT-61 Base

GIT-60 StrategyProcessor

GIT-62 Factory

jahabeebs · 2024-10-24T18:53:18Z

packages/processors/src/strategy/strategyHandler.factory.ts

+        const _strategyId = strategyId.toLowerCase();
+
+        switch (_strategyId) {
+            case "0x6f9291df02b2664139cec5703c124e4ebce32879c74b6297faa1468aa5ff9ebf":


maybe worth extracting these contract addresses out into a map for readability

sounds nice

# 🤖 Linear Closes GIT-63 ## Description - `Registered` and `Distributed` event handlers for Direct Voting Merkle Distribution Direct Transfer strategy - write an abstract class for Distributed since the logic is not attached to any strategy in particular, so it's reusable ## Checklist before requesting a review - [x] I have conducted a self-review of my code. - [x] I have conducted a QA. - [x] If it is a core feature, I have included comprehensive tests.

jahabeebs

Added some suggestions 👀

jahabeebs · 2024-10-25T22:48:02Z

packages/processors/src/allo/handlers/poolCreated.handler.ts

+            strategyId,
+        );
+
+        // const strategy = extractStrategyFromId(strategyId);


leftover comment?

jahabeebs · 2024-10-25T22:51:40Z

packages/processors/src/allo/handlers/poolCreated.handler.ts

-                    token.decimals,
-                );
+        let matchAmount = {
+            matchAmount: 0n,


hmm maybe worth naming the internal matchAmount something like matchAmountBigInt or maybe the outer matchAmount could be matchAmountObj?

i like that

jahabeebs · 2024-10-25T22:55:32Z

packages/processors/src/strategy/common/base.strategy.ts

+        this.name = name;
+    }
+
+    async fetchStrategyTimings(_strategyAddress: Address): Promise<StrategyTimings> {


Maybe an /** @inheritdoc */ for these functions?

jahabeebs · 2024-10-25T23:03:14Z

...s/src/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts

+    "roundRepository" | "projectRepository" | "metadataProvider"
+>;
+
+export class DVMDRegisteredHandler implements IEventHandler<"Strategy", "Registered"> {


worth having docs for the class + inherit docs for the functions?

jahabeebs · 2024-10-25T23:03:58Z

...s/src/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/registered.handler.ts

+            strategyAddress,
+        );
+
+        if (!round) {


I noticed in the other handler where you have (!round) it doesn't throw an exception--should only this one throw an exception?

this is the expected flow but it's a nice catch, will write it down and have it as Q for Gitcoin

jahabeebs · 2024-10-25T23:05:56Z

...es/processors/src/strategy/donationVotingMerkleDistributionDirectTransfer/helpers/decoder.ts

+    },
+] as const;
+
+export const decodeDVMDApplicationData = (encodedData: Hex): DVMDApplicationData => {


worth it to have docs here?

jahabeebs · 2024-10-25T23:06:55Z

packages/processors/src/strategy/mapping.ts

+import type { StrategyHandlerConstructor } from "../internal.js";
+import { DVMDDirectTransferStrategyHandler } from "./donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.js";
+
+const strategyIdToHandler: Readonly<Record<string, StrategyHandlerConstructor>> = {


looks good as a map now 🗺️

jahabeebs · 2024-10-25T23:09:06Z

packages/processors/src/strategy/strategyHandler.factory.ts

+        const _strategyId = strategyId.toLowerCase() as Hex;
+        const StrategyHandlerClass = getHandler(_strategyId);
+
+        console.log("StrategyHandlerClass", StrategyHandlerClass);


did you mean to log this?

jahabeebs

✅

0xkenj1

Left some comments, but great job ser !! 🫡

0xkenj1 · 2024-10-28T16:04:31Z

packages/processors/src/exceptions/unsupportedStrategy.exception.ts

+
+export class UnsupportedStrategy extends Error {
+    constructor(strategyId: Hex) {
+        super(`Strategy ${strategyId} unsupported`);


Should we add some more context here, like the address ? Maybe this is correct and we should enrich the log once is bubbled up. wdyt?

mmm we can take EBO's error handling approach and a context object for errors. i think that with bubbling the event should be enough almost every time right? is the most generic context object

0xkenj1 · 2024-10-28T16:11:50Z

packages/processors/src/internal.ts

-export * from "./strategy/index.js";
+
+// Strategy
+export * from "./strategy/common/index.js";


we can colapse all of these using strategy/index.js

0xkenj1 · 2024-10-28T16:12:43Z

packages/processors/src/strategy/common/base.strategy.ts

+        _matchingFundsAvailable: number,
+        _token: Token,
+        _blockTimestamp: number,


is there a reason why using underscores here ? i don't remember if we had a best practice for this or sth

underscore in function args means "unused variables"

0xkenj1 · 2024-10-28T16:13:34Z

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

+        );
+
+        if (!round) {
+            //TODO: add logging that round was not found


lets add a console.log here to make it easier to replace it by the logger after :)

0xkenj1 · 2024-10-28T16:15:31Z

...rs/src/strategy/donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.ts

+import { calculateAmountInUsd } from "../../helpers/tokenMath.js";
+import { getDateFromTimestamp } from "../../helpers/utils.js";


imports from index , lets avoid using specific paths

0xkenj1 · 2024-10-28T16:20:42Z

packages/processors/src/strategy/mapping.ts

+import type { StrategyHandlerConstructor } from "../internal.js";
+import { DVMDDirectTransferStrategyHandler } from "./donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.js";
+
+const strategyIdToHandler: Readonly<Record<string, StrategyHandlerConstructor>> = {


maybe is worth to add some comment or doc here

0xkenj1 · 2024-10-28T16:22:02Z

packages/processors/src/strategy/strategyHandler.factory.ts

+        const _strategyId = strategyId.toLowerCase() as Hex;
+        const StrategyHandlerClass = getHandler(_strategyId);
+
+        return StrategyHandlerClass ? new StrategyHandlerClass(chainId, dependencies) : undefined;


shouldn't we throw an error here and do a try catch wherever we need it ?

i think is not strictly necessary, if caller's action is to throw, let the caller throw. else in everyplace where code flows goes on even if strategy is undefined, we would need the try/catch which is more costly in computer terms than just asking if it's undefined

0xkenj1 · 2024-10-28T16:28:21Z

packages/processors/test/strategy/common/base.strategy.spec.ts

+    describe("fetchStrategyTimings", () => {
+        it("returns default timings", async () => {
+            const address: Address = "0x1234567890123456789012345678901234567890";
+            const timings = await handler.fetchStrategyTimings(address);
+
+            expect(timings).toEqual({
+                applicationsStartTime: null,
+                applicationsEndTime: null,
+                donationsStartTime: null,
+                donationsEndTime: null,
+            });
+        });
+    });
+
+    describe("fetchMatchAmount", () => {
+        it("returns default match amount", async () => {
+            const matchingFundsAvailable = 1000;
+            const token: Token = {
+                address: "0x1234567890123456789012345678901234567890",
+                decimals: 18,
+                code: "ETH" as TokenCode,
+                priceSourceCode: "ETH" as TokenCode,
+            };
+            const blockTimestamp = 1625097600; // Example timestamp
+
+            const result = await handler.fetchMatchAmount(
+                matchingFundsAvailable,
+                token,
+                blockTimestamp,
+            );
+
+            expect(result).toEqual({
+                matchAmount: 0n,
+                matchAmountInUsd: "0",
+            });
+        });
+    });
+});


it is worth to test faulty scenarios here ?

like what scenario?

i imagined invalid address for example, but reading the code again, i think it doesn't make sience since its is the base class

0xkenj1 · 2024-10-28T16:28:55Z

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

+    return { ...defaultEvent, ...overrides };
+}
+
+describe("BaseDistributedHandler", () => {


ditto, faulty scenarios

like what scenario?

0xkenj1 · 2024-10-28T16:29:51Z

...t/strategy/donationVotingMerkleDistributionDirectTransfer/dvmdDirectTransfer.handler.spec.ts

+        });
+        expect(BaseDistributedHandler.prototype.handle).toHaveBeenCalled();
+    });
+


ditto, faulty scenarios, what happened if event is not supported ?

event not supported is tested, is the last test of the suite

0xnigir1 added 2 commits October 24, 2024 11:16

feat: base strategy processor & dvmd strategy factory

fabe7e3

test: dvmd direct transfer strategy handler

a180bf3

0xnigir1 requested review from jahabeebs, 0xkenj1 and 0xyaco October 24, 2024 14:38

fix: types error

8dc11aa

jahabeebs reviewed Oct 24, 2024

View reviewed changes

0xnigir1 added 2 commits October 25, 2024 16:22

feat: base strategy class and refactor strategyId mappings

8c25428

0xnigir1 marked this pull request as ready for review October 25, 2024 20:58

0xnigir1 requested a review from jahabeebs October 25, 2024 21:02

jahabeebs reviewed Oct 25, 2024

View reviewed changes

fix: pr comments

baf8c34

0xnigir1 requested a review from jahabeebs October 28, 2024 13:10

jahabeebs previously approved these changes Oct 28, 2024

View reviewed changes

0xkenj1 reviewed Oct 28, 2024

View reviewed changes

fix: pr comments

5bd2d20

0xnigir1 dismissed jahabeebs’s stale review via 5bd2d20 October 28, 2024 18:45

0xnigir1 requested a review from jahabeebs October 28, 2024 21:09

jahabeebs approved these changes Oct 28, 2024

View reviewed changes

0xkenj1 approved these changes Oct 28, 2024

View reviewed changes

0xnigir1 merged commit 9ad1569 into dev Oct 28, 2024
6 checks passed

0xnigir1 deleted the feat/base-strategy-processors branch October 28, 2024 22:39

		import { calculateAmountInUsd } from "../../helpers/tokenMath.js";
		import { getDateFromTimestamp } from "../../helpers/utils.js";

feat: base strategy processor & dvmd strategy factory #16

feat: base strategy processor & dvmd strategy factory #16

Conversation

0xnigir1 commented Oct 24, 2024 • edited Loading

🤖 Linear

Description

Checklist before requesting a review

linear bot commented Oct 24, 2024

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

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

0xkenj1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnigir1 Oct 28, 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

0xnigir1 Oct 28, 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

0xnigir1 commented Oct 24, 2024 •

edited

Loading

0xnigir1 Oct 28, 2024 •

edited

Loading

0xnigir1 Oct 28, 2024 •

edited

Loading