feat: implement get events #50

jahabeebs · 2024-09-24T23:46:00Z

🤖 Linear

Closes GRT-143

Description

Implementation of getEvents—still have to fix type issue

linear · 2024-09-24T23:46:02Z

GRT-143 Implement getEvents from all relevant contracts

Signed-off-by: jahabeebs <[email protected]>

0xyaco · 2024-09-30T08:44:46Z

packages/automated-dispute/src/providers/protocolProvider.ts

+     */
+    async getEvents(fromBlock: bigint, toBlock: bigint) {
+        if (fromBlock > toBlock) {
+            throw new Error("Invalid block range: fromBlock must be less than or equal to toBlock");


Let's use a custom error here + with a message that includes fromBlock and toBlock inside

0xyaco · 2024-09-30T08:58:28Z

packages/automated-dispute/src/providers/protocolProvider.ts

+        const eventPromises = eventNames.map((eventName) =>
+            this.readClient.getLogs({
+                address: this.oracleContract.address,
+                event: oracleAbi.find(
+                    (e) => e.name === eventName && e.type === "event",
+                ) as AbiEvent,
+                fromBlock,
+                toBlock,
+            }),
+        );
+
+        const allLogs = await Promise.all(eventPromises);


Have you tried using the multiple events capabilities [1] of getLogs?

[1] https://viem.sh/docs/actions/public/getLogs#multiple-events

Suggested change

const eventPromises = eventNames.map((eventName) =>

this.readClient.getLogs({

address: this.oracleContract.address,

event: oracleAbi.find(

(e) => e.name === eventName && e.type === "event",

) as AbiEvent,

fromBlock,

toBlock,

}),

);

const allLogs = await Promise.all(eventPromises);

const eventAbis = eventNames.map((eventName) => oracleAbi.find(

(e) => e.name === eventName && e.type === "event",

) as AbiEvent);

const eventPromises = await this.readClient.getLogs({

address: this.oracleContract.address,

events: eventAbis,

fromBlock,

toBlock,

}),

);

Let's also use strict mode [2] within getLogs to avoid having to handle nullish event properties.

[2] https://viem.sh/docs/actions/public/getLogs#strict-mode

0xyaco · 2024-09-30T09:04:55Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            blockNumber: log.blockNumber ?? BigInt(0),
+            logIndex: log.logIndex ?? 0,


Do you know under which context the blockNumber and the logIndex could be nullish? It's kinda critical for the whole event processing to be working with events with these two properties defined.

I removed the nullish logic, we really only need this for requestId

0xyaco · 2024-09-30T09:27:35Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            blockNumber: log.blockNumber ?? BigInt(0),
+            logIndex: log.logIndex ?? 0,
+            rawLog: log,
+            requestId: log.topics[1] as RequestId,


I might be getting this wrong but I suspect we might need to end up doing some per-event logic to populate this property. Let's take two examples out of the Linear task:

Event Name Contract

RequestCreated EBORequestCreator

ResponseDisputed Oracle

event RequestCreated(bytes32 indexed _requestId, uint256 indexed _epoch, string indexed _chainId); event ResponseDisputed( bytes32 indexed _responseId, bytes32 indexed _disputeId, Dispute _dispute, uint256 _blockNumber );

For RequestCreated events, the requestId value is accessible through something along the lines of log.data.requestId.

For ResponseDisputed events, the requestId value needs to be extracted from the _dispute struct (eg log.data.dispute.requestId.

I tried this refactor but still getting "Property requestId does not exist on type" for the events--maybe the ABIs aren't matching up to the latest contracts but I need to do some more digging

0xyaco · 2024-09-30T09:51:12Z

packages/automated-dispute/src/interfaces/protocolProvider.ts

+ * @interface DecodedLogArgsMap
+ * Represents the mapping of event names to their respective argument structures.
+ */
+export interface DecodedLogArgsMap {


I like this approach, @0xkenj1 what do you think? Should we use this to define events' types instead of our current approach?

type EboEventData<E extends EboEventName> = E extends "RequestCreated" ? RequestCreated : E extends "ResponseProposed" ...

One way or another, we need to use a single approach as right now we have events' internal data types defined here and also defined in types/events.ts

Matt says this: https://www.totaltypescript.com/type-vs-interface-which-should-you-use

tldr; type by default, go for interfaces if you expect to do inheritance

0xyaco · 2024-09-30T09:59:12Z

packages/automated-dispute/src/providers/protocolProvider.ts

+                data: log.data,
+                topics: log.topics,
+                eventName,
+                strict: false,


Let's try strict: true

0xyaco · 2024-09-30T10:03:17Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            requestId: log.topics[1] as RequestId,
+        };
+
+        const decodedLog = this.decodeLogData(eventName, log);


After setting strict: true while getting and decoding events, is it possible to do something like this to avoid using the switch statements?

Suggested change

const decodedLog = this.decodeLogData(eventName, log);

const decodedLog = this.decodeLogData(eventName, log);

return {

...baseEvent,

metadata: decodedLog

}

I feel like there has to be a way to generalize this somehow, I might be wrong though.

I haven't found a way to get this to work yet, not sure if the ABI types are coming through properly

Could it be that the event's properties are prefixed with an underscore and we need to replicate those in our codebase?

Let's grab this event for example:

event ResponseProposed(bytes32 indexed _requestId, bytes32 indexed _responseId, Response _response);

The corresponding type inside DecodedLogArgsMap is:

ResponseProposed: { requestId: RequestId; responseId: string; response: string; blockNumber: bigint; // This property was removed yesterday };

Maybe refactoring those attributes by prefixing them with an underscore would comply with the ABI types? Feel free to do some quick validation on this.

0xyaco · 2024-09-30T10:14:33Z

packages/automated-dispute/tests/services/protocolProvider.spec.ts

+            expect(result).toEqual([
+                { name: "RequestCreated", blockNumber: 3n, logIndex: 0 },
+                { name: "ResponseDisputed", blockNumber: 2n, logIndex: 1 },
+                { name: "ResponseProposed", blockNumber: 2n, logIndex: 0 },
+                { name: "RequestCreated", blockNumber: 1n, logIndex: 0 },
+            ]);


If this test case is passing tests, we need to invert the mergeEventStreams old sorting logic to have this pass with blocks sorted the other way round.

Suggested change

expect(result).toEqual([

{ name: "RequestCreated", blockNumber: 3n, logIndex: 0 },

{ name: "ResponseDisputed", blockNumber: 2n, logIndex: 1 },

{ name: "ResponseProposed", blockNumber: 2n, logIndex: 0 },

{ name: "RequestCreated", blockNumber: 1n, logIndex: 0 },

]);

expect(result).toEqual([

{ name: "RequestCreated", blockNumber: 1n, logIndex: 0 },

{ name: "ResponseDisputed", blockNumber: 2n, logIndex: 0 },

{ name: "ResponseProposed", blockNumber: 2n, logIndex: 1 },

{ name: "RequestCreated", blockNumber: 3n, logIndex: 0 },

]);

Inverted these, still need to do some build cleanup before I can test though

jahabeebs · 2024-10-02T00:05:07Z

@0xyaco I updated the types to be the latest contract types, both in events.ts and the decodedlog types (I know we probably will just stick with one but for simplicity I have them as the same for now)

what I'm wondering is how we should handle all the broken eboActor logic like...

 proposer: eventResponse.proposer

or

        const decodedResponse = ProtocolProvider.decodeResponse(eventResponse.response);

if our event response no longer has a proposer or response object for example how are we constructing these?

0xyaco

@jahabeebs interesting question, check comments!

I'll get back to you on what to do with RequestCreated event, I think that's the only one that might be problematic.

All the other event properties' types that have been fixed (swapping type["prophetData"] with string) will imply that we need to decode the bytes string to generate the actual "prophetData" object with ProtocolProvider.decode*.

0xyaco · 2024-10-02T07:12:17Z

packages/automated-dispute/src/services/eboActor.ts

@@ -610,6 +611,7 @@ export class EboActor {
        const disputer = this.protocolProvider.getAccountAddress();
        const dispute: Dispute["prophetData"] = {
            disputer: disputer,
+            // TODO: populate proposer if does not exist on eventResponse
            proposer: eventResponse.proposer,


I think we are ok on this case, as the ResponseProposed event [1] is:

event ResponseProposed(bytes32 indexed _requestId, bytes32 indexed _responseId, Response _response);

And the Response struct [2] is:

struct Response { address proposer; bytes32 requestId; bytes response; }

Thus, the proposer can be accessed through event.metadata.proposer while the response body is accessed through ProtocolProvider.decodeResponse(event.metadata.response.response).

Do you know any other case that might be broken?

[1] https://github.com/defi-wonderland/prophet-core/blob/4d28a0ec7e8cb082f4932c52a0845b122a0ca71a/solidity/interfaces/IOracle.sol#L27
[2] https://github.com/defi-wonderland/prophet-core/blob/4d28a0ec7e8cb082f4932c52a0845b122a0ca71a/solidity/interfaces/IOracle.sol#L227-L231

0xyaco · 2024-10-02T07:15:00Z

packages/automated-dispute/src/interfaces/protocolProvider.ts

@@ -1,4 +1,4 @@
-import { Caip2ChainId } from "@ebo-agent/blocknumber";
+import { Caip2ChainId } from "@ebo-agent/blocknumber/src/index.js";


I think the blocknumber package exposes Caip2ChainId already, it shouldn't be necessary to use /src/index.js

Removed the /src/index.js that I added everywhere--not sure why my IDE was flagging it

0xyaco · 2024-10-02T07:15:21Z

packages/automated-dispute/src/providers/protocolProvider.ts

@@ -1,4 +1,4 @@
-import { Caip2ChainId, Caip2Utils, InvalidChainId } from "@ebo-agent/blocknumber";
+import { Caip2ChainId, Caip2Utils, InvalidChainId } from "@ebo-agent/blocknumber/src/index.js";


0xyaco · 2024-10-02T07:18:15Z

packages/automated-dispute/src/providers/protocolProvider.ts

-                throw new UnsupportedEvent(`Unsupported event name: ${eventName}`);
+                throw new Error(`Unsupported event name: ${eventName}`);


Was removing UnsupportedEvent intentional?

No, I think this happened when i was merging things. Added it back in

0xyaco · 2024-10-02T08:04:21Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            case "ResponseProposed":
+                requestId = decodedLog.requestId;
+                break;
+            case "ResponseDisputed":
+                requestId = decodedLog.requestId;
+                break;
+            case "DisputeStatusChanged":
+            case "DisputeEscalated":
+                requestId = decodedLog.requestId;
+                break;
+            case "RequestFinalized":
+                requestId = decodedLog.requestId;
+                break;


Let's add a TODO here and we'll open a new PR later to extract the requestId values out of each event type. I suspect this might be failing to build, right?

Task for this: https://linear.app/defi-wonderland/issue/GRT-189/extract-requestid-out-of-every-event

Added a TODO over these as well as a TODO over the this.mergeEventStreams because it depends on the requestId logic

0xyaco · 2024-10-02T08:07:51Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            requestId: log.topics[1] as RequestId,
+        };
+
+        const decodedLog = this.decodeLogData(eventName, log);


Could it be that the event's properties are prefixed with an underscore and we need to replicate those in our codebase?

Let's grab this event for example:

event ResponseProposed(bytes32 indexed _requestId, bytes32 indexed _responseId, Response _response);

The corresponding type inside DecodedLogArgsMap is:

ResponseProposed: { requestId: RequestId; responseId: string; response: string; blockNumber: bigint; // This property was removed yesterday };

Maybe refactoring those attributes by prefixing them with an underscore would comply with the ABI types? Feel free to do some quick validation on this.

0xyaco · 2024-10-02T08:10:14Z

packages/automated-dispute/src/types/events.ts

-    //TODO: remove request?
-    request: Request["prophetData"];


We might end up using Oracle's RequestCreated event instead of EBORequestCreator's RequestCreated event. I'll get back to you on this one

So, we'll be using both events, but only reacting to EBORequestCreator.RequestCreated so this can stay as it is.

This will probably cause some type failure during request creation internally in the AddRequest command, as it won't be able to populate the prophetData field; for that, let's add a TODO and fill it in with DEFAULT_MOCKED_REQUEST_CREATED_DATA["prophetData"] mocked data.

We got GRT-205 already created to tackle this in another PR.

are the ABIs up to date now since your PR was merged?

Oracle's ABI should be up to date, yep. Viem types should be working as expected for all related functionality.

jahabeebs · 2024-10-08T23:50:39Z

@0xyaco Fixed the merge conflicts, added in the new types (without blockNumber) so at least EboEvent and DecodedLogArgsMap are matching, and fixed all the tests except one (I think we need to do the request ID extraction to fix this)

0xyaco

Almost ready! Feel free to add a it.todo + a TODO comment to the failing test as we can address it later.

0xyaco · 2024-10-07T09:03:47Z

packages/automated-dispute/src/types/events.ts

-    //TODO: remove request?
-    request: Request["prophetData"];


So, we'll be using both events, but only reacting to EBORequestCreator.RequestCreated so this can stay as it is.

This will probably cause some type failure during request creation internally in the AddRequest command, as it won't be able to populate the prophetData field; for that, let's add a TODO and fill it in with DEFAULT_MOCKED_REQUEST_CREATED_DATA["prophetData"] mocked data.

We got GRT-205 already created to tackle this in another PR.

0xyaco · 2024-10-09T08:55:30Z

packages/automated-dispute/src/types/events.ts

-    //TODO: remove request?
-    request: Request["prophetData"];


Oracle's ABI should be up to date, yep. Viem types should be working as expected for all related functionality.

0xyaco · 2024-10-09T09:34:14Z

packages/automated-dispute/src/providers/protocolProvider.ts

+        const logs = await this.readClient.getLogs({
+            address: this.eboRequestCreatorContract.address,
+            event: eboRequestCreatorAbi.find(
+                (e) => e.type === "event" && e.name === "RequestCreated",
+            ) as AbiEvent,
+            fromBlock,
+            toBlock,
+        });


I think we can use this.readClient.getContractEvents [1] method to work directly with already decoded events, saving us the hassle of manually decoding the events here! Let's also include the strict: true property too so we don't have nullable event properties and the RPC call does not return non-comply events.

If the getContractEvents thing work, we could do something similar in getOracleEvents method now that we have a working ABI for Oracle; I feel like it's not worth right now for getOracleEvents though, as it'd imply doing N requests and we need to think about the consequences it might have. Let's add an OPTIMIZE comment for getOracleEvents function though, explaining that we could potentially remove the decodeLogData function and get improved typing (because we'd be directly using ABI types) if using a getContractEvents call for each Oracle event — with the trade-off of doing N requests instead of only 1.

[1] https://viem.sh/docs/contract/getContractEvents

jahabeebs · 2024-10-09T19:55:51Z

@0xyaco thanks for the helpful comments 🙏🏻 I made the following changes based on your comments:

Fixed the test that was failing
Updated getEBORequestCreatorEvents to use the type safe ways of accessing the ABI (it was with underscores as you suggested) and now it's using this.readClient.getContractEvents and strict: true
Added a ts-expect-error to a place in addRequest where we will have to fetch the request differently since we no longer have this on our current ABI

For some reason the build is failing on the CI but not locally 🤔

0xyaco · 2024-10-10T08:47:05Z

@jahabeebs there might be something wrong with some cached files on the GH actions env? Merge dev into this branch and let's see if it fixes the issue, I'm seeing some error messages as if tsc was trying to check this branch types against dev last merged types.

jahabeebs · 2024-10-10T15:14:23Z

@jahabeebs there might be something wrong with some cached files on the GH actions env? Merge dev into this branch and let's see if it fixes the issue, I'm seeing some error messages as if tsc was trying to check this branch types against dev last merged types.

You were right about the caching, I merged dev in and fixed some types and now everything is passing 🥳

0xyaco

Noiceeee let's merge this if the rest of the team is ok with it!

0xnigir1

interesting debate the one on interfaces vs types, not sure tbh

0xnigir1 · 2024-10-10T19:29:58Z

packages/automated-dispute/src/interfaces/protocolProvider.ts

+ * @interface DecodedLogArgsMap
+ * Represents the mapping of event names to their respective argument structures.
+ */
+export interface DecodedLogArgsMap {


Matt says this: https://www.totaltypescript.com/type-vs-interface-which-should-you-use

tldr; type by default, go for interfaces if you expect to do inheritance

0xnigir1 · 2024-10-10T19:34:57Z

packages/automated-dispute/src/providers/protocolProvider.ts

+            case "ResponseProposed":
+                abi = oracleAbi;
+                break;
+            case "ResponseDisputed":
+                abi = oracleAbi;
+                break;
+            case "DisputeStatusUpdated":
+                abi = oracleAbi;
+                break;
+            case "DisputeEscalated":
+                abi = oracleAbi;
+                break;
+            case "OracleRequestFinalized":
+                abi = oracleAbi;
+                break;


Suggested change

case "ResponseProposed":

abi = oracleAbi;

break;

case "ResponseDisputed":

abi = oracleAbi;

break;

case "DisputeStatusUpdated":

abi = oracleAbi;

break;

case "DisputeEscalated":

abi = oracleAbi;

break;

case "OracleRequestFinalized":

abi = oracleAbi;

break;

case "ResponseProposed":

case "ResponseDisputed":

case "DisputeStatusUpdated":

case "DisputeEscalated":

case "OracleRequestFinalized":

abi = oracleAbi;

break;

idk if you like fallthrough or not xd

changed DecodedLogArgsMap to a type

I changed it to fallthrough because the class is already super long so probably it's for the best even though I like writing things out generally 😅

0xnigir1 · 2024-10-10T19:36:36Z

packages/automated-dispute/src/providers/protocolProvider.ts

+        const eventNames = [
+            "ResponseProposed",
+            "ResponseDisputed",
+            "DisputeStatusChanged",
+            "DisputeEscalated",
+            "RequestFinalized",
+        ];


super minor, extracting this to class level attribute and avoid re-declaring the array on every function call

…-getEvents # Conflicts: # packages/automated-dispute/src/providers/protocolProvider.ts # packages/automated-dispute/src/services/eboActor.ts # packages/automated-dispute/src/services/eboProcessor.ts # packages/automated-dispute/src/types/events.ts # packages/automated-dispute/src/types/prophet.ts

jahabeebs self-assigned this Sep 24, 2024

jahabeebs added 3 commits September 27, 2024 10:20

feat: initial getEvents and errors and tests

09ec614

Signed-off-by: jahabeebs <[email protected]>

feat: type fixes

3858030

Signed-off-by: jahabeebs <[email protected]>

fix: more type fixes

d845dd9

Signed-off-by: jahabeebs <[email protected]>

jahabeebs force-pushed the feat/grt-143-implement-getEvents branch from dcb0c66 to d845dd9 Compare September 27, 2024 15:20

0xyaco reviewed Sep 30, 2024

View reviewed changes

jahabeebs added 3 commits October 1, 2024 18:45

feat: refactor event types

5f946da

fix: remove nullish

9effca9

fix: remove nullish

fd5a1f1

0xyaco reviewed Oct 2, 2024

View reviewed changes

feat: fix merge conflicts and types and tests

5344f9c

fix: lint

e0d11c0

jahabeebs requested a review from 0xyaco October 8, 2024 23:56

0xyaco reviewed Oct 9, 2024

View reviewed changes

fix: address PR comments

ecf15f5

jahabeebs requested a review from 0xyaco October 9, 2024 19:55

jahabeebs added 2 commits October 10, 2024 10:05

Merge branch 'dev' into feat/grt-143-implement-getEvents

cad9ae6

chore: sync with dev

10aba16

0xyaco previously approved these changes Oct 10, 2024

View reviewed changes

jahabeebs requested review from 0xkenj1 and 0xnigir1 October 10, 2024 18:09

0xnigir1 reviewed Oct 10, 2024

View reviewed changes

jahabeebs dismissed 0xyaco’s stale review via 4e39307 October 11, 2024 15:02

jahabeebs added 2 commits October 11, 2024 12:49

Merge branch 'dev' into feat/grt-143-implement-getEvents

eb2d090

feat: refactor DecodedLogArgsMap to type and fallthrough switch

77ada49

jahabeebs requested review from 0xyaco and 0xnigir1 October 11, 2024 21:52

0xyaco approved these changes Oct 14, 2024

View reviewed changes

0xnigir1 approved these changes Oct 14, 2024

View reviewed changes

jahabeebs merged commit b7b63e9 into dev Oct 14, 2024
5 checks passed

jahabeebs deleted the feat/grt-143-implement-getEvents branch October 14, 2024 12:46

		blockNumber: log.blockNumber ?? BigInt(0),
		logIndex: log.logIndex ?? 0,

-        const decodedLog = this.decodeLogData(eventName, log);
+        const decodedLog = this.decodeLogData(eventName, log);
+        return {
+            ...baseEvent,
+            metadata: decodedLog
+        }

		@@ -1,4 +1,4 @@
		import { Caip2ChainId } from "@ebo-agent/blocknumber";
		import { Caip2ChainId } from "@ebo-agent/blocknumber/src/index.js";

		@@ -1,4 +1,4 @@
		import { Caip2ChainId, Caip2Utils, InvalidChainId } from "@ebo-agent/blocknumber";
		import { Caip2ChainId, Caip2Utils, InvalidChainId } from "@ebo-agent/blocknumber/src/index.js";

		throw new UnsupportedEvent(`Unsupported event name: ${eventName}`);
		throw new Error(`Unsupported event name: ${eventName}`);

feat: implement get events #50

feat: implement get events #50

Conversation

jahabeebs commented Sep 24, 2024

🤖 Linear

Description

linear bot commented Sep 24, 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

jahabeebs commented Oct 2, 2024 • edited Loading

0xyaco 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahabeebs commented Oct 8, 2024

0xyaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xyaco Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

jahabeebs commented Oct 9, 2024 • edited Loading

0xyaco commented Oct 10, 2024

jahabeebs commented Oct 10, 2024

0xyaco left a comment

Choose a reason for hiding this comment

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

jahabeebs commented Oct 2, 2024 •

edited

Loading

0xyaco Oct 9, 2024 •

edited

Loading

jahabeebs commented Oct 9, 2024 •

edited

Loading