feat: add auto transfer mode #235

hhio618 · 2025-01-06T09:00:16Z

Resolves #226

0x4007 · 2025-01-06T09:21:01Z

src/parser/permit-generation-module.ts

    if (!canGeneratePermits) {
      this.context.logger.error("[PermitGenerationModule] Non collaborative issue detected, skipping.");
      return Promise.resolve(result);
    }
+
+    const sumPayouts = await this._sumPayouts(result);
+    const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fudningWalletAddress);


Suggested change

const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fudningWalletAddress);

const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fundingWalletAddress);

0x4007 · 2025-01-06T09:21:21Z

src/parser/permit-generation-module.ts

+    const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fudningWalletAddress);
+
+    if (this._autoTransferMode && sumPayouts < fundingWalletBalance) {
+      this.context.logger.debug("[PermitGenerationModule] AutoTransforMode is enabled and there are enough funds, skipping.");


Suggested change

this.context.logger.debug("[PermitGenerationModule] AutoTransforMode is enabled and there are enough funds, skipping.");

this.context.logger.debug("[PermitGenerationModule] AutoTransferMode is enabled and there are enough funds, skipping.");

0x4007 · 2025-01-06T09:22:22Z

@gentlementlegen There should be formatting checks with linter and cspell

gentlementlegen · 2025-01-06T09:30:07Z

@0x4007 Yes they seems to happen? The following run failed: https://github.com/ubiquity-os-marketplace/text-conversation-rewards/actions/runs/12629551075/job/35187679129?pr=235

This is what I see

gentlementlegen · 2025-01-07T23:39:31Z

@hhio618 I saw you marked it as [WIP] in the title, if that's the case this pull-request should be a draft so we don't review it yet.

hhio618 · 2025-01-08T05:09:33Z

@gentlementlegen I'll definitely keep that in mind for the future! For now, this task is ready for review.

gentlementlegen · 2025-01-08T06:57:19Z

src/parser/payment-module.ts

-        await this._savePermitsToDatabase(result[key].userId, { issueUrl: payload.issueUrl, issueId }, permits);
-      } catch (e) {
-        this.context.logger.error(`[PermitGenerationModule] Failed to generate permits for user ${key}`, { e });
+        result[key].explorerUrl = `https://gnosisscan.io/tx/${tx.hash}`;


Given that we can use any network and any ERC20 address, wouldn't this be a problem?

Certainly! I addressed this by using the following:

import { getNetworkExplorer, NetworkId } from "@ubiquity-dao/rpc-handler";

gentlementlegen · 2025-01-08T06:59:58Z

@hhio618 If you can please fix the tests:

FAIL tests/permit-generatable.test.ts
  ● Test suite failed to run

    tests/permit-generatable.test.ts:162:49 - error TS2307: Cannot find module '../src/parser/permit-generation-module' or its corresponding type declarations.

    162 const { PermitGenerationModule } = await import("../src/parser/permit-generation-module");

(cf https://github.com/ubiquity-os-marketplace/text-conversation-rewards/actions/runs/12664483458/job/35292699217?pr=235#step:5:82)

hhio618 · 2025-01-09T06:04:44Z

@gentlementlegen I'm on it! I'll update the PR once the tests are fixed.

whilefoo · 2025-01-12T16:47:33Z

src/parser/payment-module.ts

      return Promise.resolve(result);
    }
+
+    const totalPayable = await this._getTotalPayable(result);
+    const fundingWalletBalance = await getErc20Balance(this._evmNetworkId, this._erc20RewardToken, privateKey);


shouldn't you pass the address instead of the private key?

whilefoo · 2025-01-12T19:27:15Z

src/parser/payment-module.ts

@@ -96,51 +116,115 @@ export class PermitGenerationModule extends BaseModule {

    for (const [key, value] of Object.entries(result)) {


Suggested change

for (const [key, value] of Object.entries(result)) {

for (const [username, reward] of Object.entries(result)) {

I know this is code that you didn't change but still variables shouldn't have generic names

whilefoo · 2025-01-12T19:55:49Z

src/parser/payment-module.ts

+        const beneficiaryWalletAddress = await this._getBeneficiaryWalletAddress(key);
+        const signedTxData = await createTransferSignedTx(
+          this._evmNetworkId,
+          this._erc20RewardToken,
+          privateKey,
+          beneficiaryWalletAddress,
+          value.total.toString()
+        );
+        const tx = await sendSignedTx(this._evmNetworkId, signedTxData);


is there a reason to split into two operations (sign + send) instead of just doing it in one operation

Hey! So, I initially tried mocking the ethers module to test things out, but it didn't quite work. I switched to using an Erc20Wrapper class, and that seems to do the trick for testing everything and also helps with the getErc20TokenSymbol thing you mentioned.

whilefoo · 2025-01-12T19:56:36Z

src/parser/payment-module.ts

+    let shouldTransferDirectly = false;
+    if (this._autoTransferMode && totalPayable < fundingWalletBalance) {
+      this.context.logger.debug(
+        "[PaymentModule] AutoTransformMode is enabled, " +
+          "and sufficient funds are available in the funding wallet, skipping."
+      );
+      shouldTransferDirectly = true;
+    }


you also need to check if the wallet has enough balance to cover gas fees

Okay, I'll take care of that and add some extra tests. Then we can do another review.

whilefoo · 2025-01-12T20:00:05Z

src/parser/payment-module.ts

+    const userId = userData.id;
+    const { data: walletData } = await this._supabase.from("wallets").select("address").eq("id", userId).single();
+    if (!walletData?.address) {
+      throw new Error("Beneficiary wallet not found");


we usually use logger to throw errors like

throw this.logger.error("Beneficiary wallet not found");

whilefoo · 2025-01-12T20:04:07Z

src/parser/payment-module.ts

+    if (!networkExplorer) {
+      return "https://blockscan.com";
+    }
+    return networkExplorer[0].url;


Suggested change

return networkExplorer[0].url;

return networkExplorer[0]?.url;

whilefoo · 2025-01-12T20:48:38Z

tests/helpers/web3.test.ts

+  describe("getEvmWallet()", () => {
+    it("Should return ERC20 token decimals", async () => {


wrong test name

whilefoo · 2025-01-12T20:49:28Z

tests/helpers/web3.test.ts

+        "0x94d7a85efef179560f9b821cadd20056600fdb9d",
+        parseUnits("100", 18).toString()
+      );
+      expect(tx).toEqual("sdvfds");


what is this random string?

whilefoo · 2025-01-12T20:55:06Z

tests/parser/payment-module.test.ts

@@ -182,14 +202,49 @@ describe("permit-generation-module.ts", () => {
    });
  });

+  describe("Auto transfer mode tests", () => {


you should also test the actual transfer, not just helper functions

0x4007 · 2025-01-23T20:28:01Z

I suppose that it would be best to use the existing deployments instead of deploying a new one.

hhio618 · 2025-01-23T20:31:12Z

I suppose that it would be best to use the existing deployments instead of deploying a new one.

Certainly! It's available on multiple chains at this address: 0xD152f549545093347A162Dce210e7293f1452150

Keyrxng · 2025-01-24T17:07:10Z

https://github.com/PopPunkLLC/GasliteDrop/blob/main/contracts/src/GasliteDrop.sol

https://drop.gaslite.org/

hhio618 · 2025-01-24T18:53:08Z

For handling reopen issues, you can look at the previous comment and check transaction hashes.

@whilefoo Could you provide more details on this part?

whilefoo · 2025-01-24T20:54:02Z

@whilefoo Could you provide more details on this part?

For example, the issue is closed and rewards are sent directly (auto transfer), then someone opens the issue and closes it again, it should not send the reward again. That's why you should check the previous comments which has rewards and see if it has transaction hashes, or if it has permit urls (auto transfer mode could've been toggled)

0x4007 · 2025-01-24T20:58:01Z

@whilefoo Could you provide more details on this part?

For example, the issue is closed and rewards are sent directly (auto transfer), then someone opens the issue and closes it again, it should not send the reward again. That's why you should check the previous comments which has rewards and see if it has transaction hashes, or if it has permit urls (auto transfer mode could've been toggled)

I overlooked this complexity. We rely on nonces for permits I wonder if there is a similar strategy that we can follow for transfers, and to have them be automatically invalid if the same "nonce" for the task is already used.

hhio618 · 2025-01-26T08:54:37Z

What do you think about generating a permit request with multiple recipients and executing it immediately? This way, we can take advantage of the nonce functionality again!

whilefoo · 2025-01-26T13:36:57Z

What do you think about generating a permit request with multiple recipients and executing it immediately?

That's a good idea, we can use batch version of permitTransferFrom where owner and spender are the funding wallet and transferDetails is filled with all reward recipients. Then you can immediately call the permit which will transfer tokens to all wallets and still keep the nonce functionality

0x4007 · 2025-01-26T18:18:44Z

If it's technically possible then yes it seems the most secure.

hhio618 · 2025-01-27T10:34:00Z

@whilefoo It's almost complete, but there's one thing to consider:
We need to save the transfer data to Supabase. Do you have any suggestions on how the record should be structured?

src/helpers/web3.ts

src/parser/payment-module.ts

tests/helpers/web3.test.ts

whilefoo · 2025-01-31T19:56:46Z

src/parser/payment-module.ts

+      const totalFee = await permit2Wrapper.estimatePermitTransferFromGas(fundingWallet, batchTransferPermit);
+      const totalReward = beneficiaries.amounts.reduce(
+        (accumulator, current) => accumulator.add(current),
+        BigNumber.from(0)
+      );


won't estimate throw UNPREDICTABLE_GAS_LIMIT if there are not enough funds? so the funds should be checked first and after that gas fee, but UNPREDICTABLE_GAS_LIMIT should be handled in any case

@hhio618 looks like you haven't handled this

Does this actually affect the flow? Just to mention, we will log this in the catch block regardless.

tests/payment-generatable.test.ts

src/helpers/web3.ts

src/parser/payment-module.ts

whilefoo · 2025-01-31T20:56:39Z

src/parser/payment-module.ts

@@ -114,27 +124,45 @@ export class PaymentModule extends BaseModule {
    result = await this._applyFees(result, payload.erc20RewardToken);

    if (this._autoTransferMode) {
+      // Generate the batch transfer nonce
+      const nonce = utils.keccak256(utils.toUtf8Bytes(issueId.toString()));


you need to check if the nonce is used, for example if the issue was re-opened and closed, and in that case stop permit generation

Currently, it will always generate a permit, but the permit may be invalid if it was already claimed.

This is especially useful if the assignee was unassigned just before the completion of the project, and the permits have to be regenerated.

Because this is auto transfer mode, it will try to create a permit and claim it again which will fail, so ideally it should stop the execution.

The situation you're describing will be problematic in auto transfer mode - the assignee that was unassigned will get the reward for other things like comments and it will be automatically transferred so after the user is assigned back and the issue is reopened and closed, the plugin would need to calculate the difference in rewards for each user and send it again.

the plugin would need to calculate the difference in rewards for each user and send it again.

Perhaps we can make a new spec for this, and for now lets just make it invalid because the original task nonce was already used?

for now lets just make it invalid because the original task nonce was already used?

What do you mean by make it invalid? The first time it will be automatically transferred and the second time it should execution because the nonce was used

Right. Anything after the first attempt for rewards distribution should fail due to the nonce.

src/parser/payment-module.ts

0x4007 · 2025-02-01T05:28:49Z

@whilefoo It's almost complete, but there's one thing to consider:

We need to save the transfer data to Supabase. Do you have any suggestions on how the record should be structured?

I'm pretty sure the Supabase SDK can output types which answers this question. @gentlementlegen RFC

Co-authored-by: whilefoo <[email protected]>

gentlementlegen · 2025-02-02T00:43:56Z

Yes types can be generated from supabase CLI. We are already saving permits there, and types are already present in this plugin, are changes needed in the structure?

whilefoo · 2025-02-04T21:52:40Z

src/parser/payment-module.ts

          await Promise.all(
-            beneficiaries.usernames.map(async (username) => {
-              result[username].explorerUrl = `${networkExplorer}/tx/${tx.hash}`;
+            beneficiaries.map(async (beneficiary, idx) => {


Suggested change

beneficiaries.map(async (beneficiary, idx) => {

beneficiaries.forEach(async (beneficiary, idx) => {

Thank you for your suggestion! I just wanted to double-check something: since forEach does not wait for async functions to resolve, there's a possibility that the function could return before all the database records are saved. Could we consider using an alternative approach, like for...of or Promise.all, to ensure all records are saved properly? Let me know your thoughts!

whilefoo · 2025-02-04T21:57:21Z

src/parser/payment-module.ts

      const totalFee = await permit2Wrapper.estimatePermitTransferFromGas(fundingWallet, batchTransferPermit);
-      const totalReward = beneficiaries.amounts.reduce(
-        (accumulator, current) => accumulator.add(current),
+      const totalReward = beneficiaries.reduce(
+        (accumulator, current) => accumulator.add(current.amount),
        BigNumber.from(0)
      );


also check allowance

whilefoo · 2025-02-04T22:07:32Z

src/parser/payment-module.ts

+      if (e.code === ethers.errors.INSUFFICIENT_FUNDS || e.message.includes(ethers.errors.INSUFFICIENT_FUNDS)) {
+        throw new Error(
+          this.context.logger.error(`Insufficient funds to complete the transaction`, { e }).logMessage.raw
        );
-        return [tx, permits];
-      } catch (e) {
-        if (e instanceof Object) {
-          if ("code" in e && e.code === ethers.errors.INSUFFICIENT_FUNDS) {
-            throw new Error(
-              this.context.logger.error(`Error: Insufficient funds to complete the transaction`, { e }).logMessage.raw
-            );
-          } else if (
-            "message" in e &&
-            typeof e.message === "string" &&
-            e.message.includes(ethers.errors.INSUFFICIENT_FUNDS)
-          ) {
-            throw new Error(this.context.logger.error("Error: Insufficient gas or balance detected").logMessage.raw);
-          }
-        }
-        attempt++;
-        this.context.logger.error(`Attempt ${attempt} failed: ${e}`, { e });
-        // Exponential backoff delay
-        this.context.logger.info(`Retrying in ${delay}ms...`);
-        await new Promise((resolve) => setTimeout(resolve, delay));
-        delay *= 2;
+      } else if (
+        e.code === ethers.errors.INSUFFICIENT_FUNDS ||
+        e.message.includes(ethers.errors.UNPREDICTABLE_GAS_LIMIT)
+      ) {
+        throw new Error(this.context.logger.error("The gas limit could not be estimated", { e }).logMessage.raw);
+      } else {
+        throw new Error(this.context.logger.error(`Transaction failed: ${e.message}`, { e }).logMessage.raw);


Why is UNPREDICTABLE_GAS_LIMIT checked in the message? It's in the code, isn't it?
I would just check if code is UNPREDICTABLE_GAS_LIMIT, INSUFFICIENT_FUNDS (and also TRANSFER_FROM_FAILED I think? But that is not standard error so it might be in the message)

This could happen when Ethers occasionally sends a SERVER_ERROR code or a similar error, but includes the actual code within the message field.

whilefoo · 2025-02-04T22:07:51Z

src/parser/payment-module.ts

+      );
+      return [tx, permits];
+    } catch (error) {
+      const e = error as { reason: string; code: string; message: string };


you should also check if those properties are actually present

0x4007 reviewed Jan 6, 2025

View reviewed changes

hhio618 force-pushed the development branch 3 times, most recently from 52b8f03 to c7d396e Compare January 7, 2025 16:28

gentlementlegen reviewed Jan 8, 2025

View reviewed changes

gentlementlegen changed the title ~~[WIP] feat: add auto transfer mode~~ feat: add auto transfer mode Jan 8, 2025

hhio618 force-pushed the development branch from 3d992e8 to 81aa774 Compare January 10, 2025 22:30

gentlementlegen requested a review from whilefoo January 12, 2025 09:51

whilefoo requested changes Jan 12, 2025

View reviewed changes

hhio618 force-pushed the development branch from 150c2ea to 4a0eff4 Compare January 16, 2025 16:31

hhio618 and others added 13 commits January 16, 2025 20:05

feat: add auto transfer configs

0aca3db

feat: add the payment module

9609725

feat: add direct transfer

aa88e8d

chore: remove unused plugin input

1a332f2

fix: network explorer url

f8ae45b

fix: unit tests

b0fcbc5

chore: updated manifest.json and dist build

3b9b566

test: add web3 unit tests

f8626b3

chore: updated manifest.json and dist build

79208f0

fix: include gas fees, improvements

38a1ad8

fix: unit-tests

f517799

test: add for _canTransferDirectly

4eea5f2

chore: updated manifest.json and dist build

a205697

hhio618 force-pushed the development branch from acd3ea1 to a205697 Compare January 16, 2025 16:35

feat: switch to disperse.app for erc20 transfer

fe08dd2

chore: updated manifest.json and dist build

26586ff

hhio618 and others added 2 commits January 27, 2025 13:49

feat: implement Permit2 for handling batch transfers securely

dd9ef23

chore: updated manifest.json and dist build

0e75f51

hhio618 and others added 2 commits January 30, 2025 17:43

fix: bug fix and improvements

bf69134

chore: updated manifest.json and dist build

4bc73d3

hhio618 requested a review from whilefoo January 30, 2025 14:21

whilefoo requested changes Jan 31, 2025

View reviewed changes

Update src/helpers/web3.ts

77ce60b

Co-authored-by: whilefoo <[email protected]>

fix: fixes and improvements

7721dbd

hhio618 requested a review from whilefoo February 2, 2025 08:10

chore: updated manifest.json and dist build

fe97596

whilefoo requested changes Feb 4, 2025

View reviewed changes

hhio618 and others added 4 commits February 5, 2025 11:40

feat: add check for allowance

b035d5f

chore: updated manifest.json and dist build

b11d7e9

chore: improve error handling

4f32975

chore: updated manifest.json and dist build

9a1a746

hhio618 requested a review from whilefoo February 6, 2025 16:09

	const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fudningWalletAddress);
	const fundingWalletBalance = await getFundingWalletBalance(this._evmNetworkId, this._erc20RewardToken, this._fundingWalletAddress);

	this.context.logger.debug("[PermitGenerationModule] AutoTransforMode is enabled and there are enough funds, skipping.");
	this.context.logger.debug("[PermitGenerationModule] AutoTransferMode is enabled and there are enough funds, skipping.");

		@@ -96,51 +116,115 @@ export class PermitGenerationModule extends BaseModule {

		for (const [key, value] of Object.entries(result)) {

	for (const [key, value] of Object.entries(result)) {
	for (const [username, reward] of Object.entries(result)) {

	return networkExplorer[0].url;
	return networkExplorer[0]?.url;

		describe("getEvmWallet()", () => {
		it("Should return ERC20 token decimals", async () => {

	beneficiaries.map(async (beneficiary, idx) => {
	beneficiaries.forEach(async (beneficiary, idx) => {

feat: add auto transfer mode #235

Are you sure you want to change the base?

feat: add auto transfer mode #235

Conversation

hhio618 commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x4007 commented Jan 6, 2025

gentlementlegen commented Jan 6, 2025 • edited Loading

gentlementlegen commented Jan 7, 2025

hhio618 commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlementlegen commented Jan 8, 2025

hhio618 commented Jan 9, 2025

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

0x4007 commented Jan 23, 2025

hhio618 commented Jan 23, 2025

Keyrxng commented Jan 24, 2025

hhio618 commented Jan 24, 2025

whilefoo commented Jan 24, 2025

0x4007 commented Jan 24, 2025

hhio618 commented Jan 26, 2025

whilefoo commented Jan 26, 2025 • edited Loading

0x4007 commented Jan 26, 2025

hhio618 commented Jan 27, 2025

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

0x4007 commented Feb 1, 2025

gentlementlegen commented Feb 2, 2025

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

gentlementlegen commented Jan 6, 2025 •

edited

Loading

whilefoo commented Jan 26, 2025 •

edited

Loading