feat: dvmd direct transfer allocated handler #29

0xnigir1 · 2024-11-08T20:12:14Z

🤖 Linear

Closes GIT-140 GIT-142

Description

Allocated event handler for DVMD Direct Transfer strategy
add AllocatedWithX events from Envio
adds DonationRepository

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-08T20:12:16Z

GIT-140 Merkle DirectTransfer Allocated

GIT-142 Donation Repository

jahabeebs · 2024-11-08T21:12:17Z

...rs/src/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts

+}
+
+export class MetadataParsingFailed extends Error {
+    constructor(additionalInfo?: string) {


should move these two errors to exceptions?

i thought maybe they're only specific to this handler, but if later i need it for another Handler, i can refactor and move to exceptions, wdyt?

i agree with jahabeebs, lets keep them on its specific directory to avoid noise on this file... we can have exceptions/handlers/DVMD.... or some similar directory structure, wdyt ?

jahabeebs · 2024-11-08T21:16:14Z

packages/processors/src/helpers/pricing.ts

+import { calculateAmountInToken, calculateAmountInUsd } from "./tokenMath.js";
+
+// sometimes coingecko returns no prices for 1 hour range, 2 hours works better
+const TIMESTAMP_DELTA_RANGE = 2 * 60 * 60 * 1000;


do you think this might be better as an env variable to avoid a code change if their api behavior changes?

there isn't a specific api behaviour here actually, this comment is smth Gitcoin team found out experimenting,
i think for now is not needed an env variable but now im thinking if maybe this can be the default value on Coingecko provider instead of API caller passing timestamp.now() + DELTA_RANGE everytime. i leave it for reviewing later 🫡

agree with you nigiri, lets have this value as default ( within the CoingeckoProvider) and make the param optional , wdyt ?

will add a task in linear to refactor in another PR

https://linear.app/defi-wonderland/issue/GIT-144/refactor-pricingprovider-coingecko-to-make-optional-the-to-timestamp

jahabeebs · 2024-11-08T21:18:57Z

packages/processors/src/helpers/tokenMath.ts

+    const tokenPriceInUsdBN = new BigNumber(tokenPriceInUsd);
+    const scaleFactor = new BigNumber(10).pow(tokenDecimals);
+
+    return BigInt(amountInUsdBN.multipliedBy(scaleFactor).dividedBy(tokenPriceInUsdBN).toFixed(0));


might need some protection against division by zero (and possibly invalid/NaN values, not sure all the responses coingecko can return)

i was relying on the exception that bignumber.js throws for ZeroDivision, i'll clarify in natssec that it throws an exception in that case)

jahabeebs · 2024-11-08T21:21:05Z

packages/processors/test/helpers/tokenMath.spec.ts

+        expect(calculateAmountInToken("2", "1000000000000000000", 18)).toBe(2n);
+    });
+
+    it("throw an error for zero token price", () => {


this might be useful as a custom exception given that it's a very specific case (which we can then add to the unit test case)

another test case to consider would be if you get expected results when you vary the decimal precision (unless the api always returns the same precision)

0xkenj1 · 2024-11-11T19:32:42Z

packages/processors/src/helpers/pricing.ts

+import { calculateAmountInToken, calculateAmountInUsd } from "./tokenMath.js";
+
+// sometimes coingecko returns no prices for 1 hour range, 2 hours works better
+const TIMESTAMP_DELTA_RANGE = 2 * 60 * 60 * 1000;


agree with you nigiri, lets have this value as default ( within the CoingeckoProvider) and make the param optional , wdyt ?

0xkenj1 · 2024-11-11T19:33:26Z

packages/processors/src/helpers/pricing.ts

+import { Token } from "@grants-stack-indexer/shared";
+
+import { TokenPriceNotFoundError } from "../internal.js";
+import { calculateAmountInToken, calculateAmountInUsd } from "./tokenMath.js";


Suggested change

import { calculateAmountInToken, calculateAmountInUsd } from "./tokenMath.js";

import { calculateAmountInToken, calculateAmountInUsd } from "./index.js";

how to make autoimport work magically with this? maybe it's easier to have one single file called index.js with all the code ?)

i know that is annoying but, it save us some future headaches 🤣

0xkenj1 · 2024-11-11T19:36:52Z

packages/processors/src/helpers/pricing.ts

+    const tokenPrice = await pricingProvider.getTokenPrice(
+        token.priceSourceCode,
+        timestamp,
+        timestamp + TIMESTAMP_DELTA_RANGE,


in addition to prev comment, instead timestamp + TIMESTAMP_DELTA_RANGE as param , just delta

0xkenj1 · 2024-11-11T19:36:57Z

packages/processors/src/helpers/pricing.ts

+    const closestPrice = await pricingProvider.getTokenPrice(
+        token.priceSourceCode,
+        timestamp,
+        timestamp + TIMESTAMP_DELTA_RANGE,


0xkenj1 · 2024-11-11T19:38:01Z

packages/processors/src/helpers/tokenMath.ts

+ */
+export const calculateAmountInToken = (
+    amountInUSD: string,
+    tokenPriceInUsd: string | number,


should this always be string ?

i left it open because pricing provider returns number because coingecko returns a number (anyways, it doesn't affect the purpose of the function)

oh, got it , now i remember

0xkenj1 · 2024-11-11T19:50:09Z

...t/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.spec.ts

+    it("throws OriginMissing if origin is undefined", async () => {
+        mockEvent = createMockEvent();
+        //forcefully remove origin from mockEvent
+        mockEvent.params.origin = undefined as unknown as `0x${string}`;
+
+        handler = new DVMDAllocatedHandler(mockEvent, chainId, {
+            roundRepository: mockRoundRepository,
+            applicationRepository: mockApplicationRepository,
+            pricingProvider: mockPricingProvider,
+        });
+
+        await expect(handler.handle()).rejects.toThrow(OriginMissing);
+    });


same comment as above

0xkenj1 · 2024-11-11T19:51:43Z

...rs/src/strategy/donationVotingMerkleDistributionDirectTransfer/handlers/allocated.handler.ts

+        );
+
+        if (!round) {
+            throw new RoundNotFound(this.chainId, normalizedStrategyAddress);


this is out of scope, but in this case this should be a critical error, right? i mean the round should always be defined for the strategy

0xkenj1 · 2024-11-11T19:52:20Z

packages/repository/src/external.ts

@@ -35,17 +36,21 @@ export type {
    PartialApplication,
 } from "./types/application.types.js";

+export type { Donation, NewDonation } from "./types/donation.types.js";


Suggested change

export type { Donation, NewDonation } from "./types/donation.types.js";

export type { Donation, NewDonation } from "./types/index.js";

0xkenj1 · 2024-11-11T19:52:29Z

packages/repository/src/external.ts

@@ -35,17 +36,21 @@ export type {
    PartialApplication,
 } from "./types/application.types.js";


Suggested change

} from "./types/application.types.js";

} from "./types/index.js";

0xkenj1 · 2024-11-11T19:53:55Z

packages/repository/src/types/donation.types.ts

+    timestamp: Date;
+};
+
+export type NewDonation = Donation;


why do we need this, i would rather keep just Donation

i think is better and cleaner to have separate typing (or interfaces/classes) for the "select" type, the "insert" typing "update" typing, even if they exactly match

0xkenj1

lets gooo

0xnigir1 added 3 commits November 7, 2024 11:31

feat: donation repository

1315669

feat: dvmd allocated event handler

fead1c4

Merge branch 'dev' into feat/dvmd-dt-allocated-handler

108470d

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

jahabeebs reviewed Nov 8, 2024

View reviewed changes

0xkenj1 reviewed Nov 11, 2024

View reviewed changes

fix: pr comments

4c46e0c

0xnigir1 requested review from jahabeebs and 0xkenj1 November 12, 2024 17:43

jahabeebs approved these changes Nov 12, 2024

View reviewed changes

0xkenj1 approved these changes Nov 12, 2024

View reviewed changes

0xnigir1 merged commit 658bd7b into dev Nov 12, 2024
6 checks passed

0xnigir1 deleted the feat/dvmd-dt-allocated-handler branch November 12, 2024 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dvmd direct transfer allocated handler #29

feat: dvmd direct transfer allocated handler #29

0xnigir1 commented Nov 8, 2024 •

edited

Loading

linear bot commented Nov 8, 2024

jahabeebs Nov 8, 2024

0xnigir1 Nov 11, 2024

0xkenj1 Nov 11, 2024

jahabeebs Nov 8, 2024

0xnigir1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xnigir1 Nov 11, 2024

0xnigir1 Nov 12, 2024

jahabeebs Nov 8, 2024

0xnigir1 Nov 11, 2024

jahabeebs Nov 8, 2024

jahabeebs Nov 8, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xnigir1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xnigir1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xnigir1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xkenj1 Nov 11, 2024

0xnigir1 Nov 11, 2024 •

edited

Loading

0xkenj1 left a comment

	import { calculateAmountInToken, calculateAmountInUsd } from "./tokenMath.js";
	import { calculateAmountInToken, calculateAmountInUsd } from "./index.js";

	export type { Donation, NewDonation } from "./types/donation.types.js";
	export type { Donation, NewDonation } from "./types/index.js";

		@@ -35,17 +36,21 @@ export type {
		PartialApplication,
		} from "./types/application.types.js";

	} from "./types/application.types.js";
	} from "./types/index.js";

feat: dvmd direct transfer allocated handler #29

feat: dvmd direct transfer allocated handler #29

Conversation

0xnigir1 commented Nov 8, 2024 • edited Loading

🤖 Linear

Description

Checklist before requesting a review

linear bot commented Nov 8, 2024

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

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 11, 2024 • edited Loading

Choose a reason for hiding this comment

0xkenj1 left a comment

Choose a reason for hiding this comment

0xnigir1 commented Nov 8, 2024 •

edited

Loading

0xnigir1 Nov 11, 2024 •

edited

Loading