Aladeenb / forta-bots

MIT License
1 stars 0 forks source link

handleTransaction() returning empty array when bot is created #2

Open Aladeenb opened 1 year ago

Aladeenb commented 1 year ago

Description:

I am trying to test the case when a bot is created. The function is supposed to return a Finding object if a bot is created, but it is returning an empty array instead.

Here is the code for the test:

describe("handleTransaction", () => {
    it("returns a finding if a bot is created", async () => {
      const mockCreateBot = {
        args: {
          agentId: "0001",
          owner: "0xabc",
          metadata: "metadata",
          chainIds: "[001, 002, 003, 004]",
        },
      };
      mockTxEvent.filterFunction = jest.fn().mockReturnValue([mockCreateBot]);

      const findings = await handleTransaction(mockTxEvent);

      expect(findings).toStrictEqual([
        Finding.fromObject({
          name: "Nethermind Bot",
          description: `Bot created from (${NETHERMIND_ADDRESS}).`,
          alertId: "NETHERMIND-1",
          type: FindingType.Info,
          severity: FindingSeverity.Info,
          metadata: {
            from: mockCreateBot.args.owner,
          },
        }),
      ]);
      expect(mockTxEvent.filterFunction).toHaveBeenCalledTimes(2);
      expect(mockTxEvent.filterFunction).toHaveBeenCalledWith(CREATE_BOT_FUNCTION, DEPLOY_UPDATE_CONTRACT_ADDRESS);
    });
  });

I am expecting the test to pass, but it is failing with the error message:

    - Expected  - 15
    + Received  +  1

It seems that handleTransaction() function is not correctly handling the returned value from mockTxEvent.filterFunction.

I would appreciate any help in resolving this issue. Please let me know if you need any more information or context.

Steps to Reproduce:

  1. Run the test case
  2. See the test case is failing

Expected outcome:

npx jest
 PASS  src/agent.spec.ts
  Bot created
    handleTransaction
      √ return empty findings if there are no bots created (7 ms)
      √ returns a finding if a bot is created (10 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total

Actual outcome:

npx jest
 FAIL  src/agent.spec.ts (5.279 s)
  Bot created
    handleTransaction
      √ return empty findings if there are no bots created (7 ms)
      × returns a finding if a bot is created (10 ms)

  ● Bot created › handleTransaction › returns a finding if a bot is created

    expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 15
    + Received  +  1

    - Array [
    -   Finding {
    -     "addresses": Array [],
    -     "alertId": "NETHERMIND-1",
    -     "description": "Bot created from (0x88dC3a2284FA62e0027d6D6B1fCfDd2141a143b8).",
    -     "labels": Array [],
    -     "metadata": Object {
    -       "from": "0xabc",
    -     },
    -     "name": "Nethermind Bot",
    -     "protocol": "ethereum",
    -     "severity": 1,
    -     "type": 4,
    -   },
    - ]
    + Array []

      47 |       const findings = await handleTransaction(mockTxEvent);
      48 |
    > 49 |       expect(findings).toStrictEqual([
         |                        ^
      50 |         Finding.fromObject({
      51 |           name: "Nethermind Bot",
      52 |           description: `Bot created from (${NETHERMIND_ADDRESS}).`,

      at src/agent.spec.ts:49:24
      at step (src/agent.spec.ts:56:23)
      at Object.next (src/agent.spec.ts:37:53)
      at fulfilled (src/agent.spec.ts:28:58)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Vxatz commented 1 year ago

Hey @Aladeenb, can you send a link to the code as well? Also, you may want to look at TestTransactionEvent from the GAM, which I think will make mocking the event easier for you.

Aladeenb commented 1 year ago

Code link: https://github.com/Aladeenb/learning-forta-bots-development/blob/%40intern-challenges/one/detection-bot/src/agent.spec.ts#L17

I will check GAM @Vxatz, thank you

Vxatz commented 1 year ago

Thanks for the link.

So, firstly, there are some issues in the bot logic.

1) In this const createUpdateTransaction = Object.keys(txEvent.addresses).find(() => NETHERMIND_ADDRESS); The NETHERMIND_ADDRESS is a checksum address, while the txEvent.addresses returns the addresses in lowercase. 2) The simpler way to perform this check is to check if txEvent.from is equal to Nethermind Address. 3) The e.g. else if (!createBots && updateBots) check in L51 is not correct because createBots in this case is not undefined, but an empty array (the correct check would be !(createBots.length >0)

(You can re-open the PR and ask for a re-review)

Regarding your question, you're currently not mocking the txEvent object correctly. E.g. in the real logic you're using txEvent.addresses, which is not mocked. But the main issues have to do with the logic, so you can focus on that first (then you can use the TestTransactionEvent as I mentioned before)

Another issue is you're using mockTxEvent.filterFunction = jest.fn().mockReturnValue([mockCreateBot]), instead of mockTxEvent.filterFunction = jest.fn().mockReturnValueOnce([mockCreateBot]); (which would call the mock only for the create functions - not for update functions).

Lastly, both Forta and Nethermind address should be mocked while testing, so you can pass them as parameters to the provideHandleTransaction (which will be a wrapper function for the handleTransaction).