feat: protocol provider base #12

0xkenj1 · 2024-07-24T20:38:12Z

🤖 Linear

Closes GRT-49 GRT-50

Description

Implement Base Class for Protocol provider
Implement getCurrentEpoch()

linear · 2024-07-24T20:38:15Z

GRT-49 Implement Base Class for Protocol provider

Follow figma diagrams : https://www.figma.com/board/BbciqJb5spg35ZglTsRRBb/Offchain?node-id=233-2388&t=rxiyrWw25Xi7qLak-4

We are not implementing any of the current methods given that on-chain team is starting its development phase.

AC:

Protocol provider class implemented with all its methods empty.
Use viem as provider library to interact with evm blockchains
https://viem.sh/docs/clients/public
New directory created for contract ABI's
Unit tests.

GRT-50 Implement `getCurrentEpoch()`

This is the only method that should be implemented.

Use https://github.com/graphprotocol/contracts/blob/3eb16c80d4652c238d3e6b2c396da712af5072b4/packages/contracts/contracts/epochs/EpochManager.sol#L101-L116

AC:

getCurrentEpoch successfully implemented
getCurrentEpoch returns EpochNumber and EpochBlock
Unit tests

0xyaco

Pretty solid typing and also solid usage of vitest mocking. 🚀

Left some comments, most of them to know your stance on some particular topics.

0xyaco · 2024-07-25T01:24:03Z

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

@@ -0,0 +1,2 @@
+export type Address = `0x${string}`;


Cool, had never used template literal types.

0xyaco · 2024-07-25T01:27:45Z

packages/automated-dispute/src/protocolProvider.ts

+     * @param rpcUrls The RPC URLs to connect to the Arbitrum chain
+     * @param contracts The addresses of the protocol contracts that will be instantiated
+     */
+    constructor(rpcUrls: string[], contracts: ProtocolContractsAddresses) {


We should probably validate that the rpcUrls is not empty, as instantiating a ProtocolProvider with empty rpcUrls probably makes no sense (and might fail while trying to create the actual client?).

This would catch a potential EBO agent misconfiguration, if no rpc urls are specified for the protocol chain.

You are right , we have arbitrum variable that contains metadata, including some public rpc, but we want to be explicit here i think

0xyaco · 2024-07-25T01:38:07Z

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

+    });
+
+    describe("constructor", () => {
+        it("should create a new ProtocolProvider instance successfully", () => {


This is an extremely small matter of preference (bias from other languages); what do you think about phrasing the it descriptions by dropping the should? IMO is more concise and gives you some characters to describe better the test case.

Suggested change

it("should create a new ProtocolProvider instance successfully", () => {

it("creates a new ProtocolProvider instance successfully", () => {

Im in. Probably should is in someway redundant. I will add this to our best practices & standards doc.

0xyaco · 2024-07-25T01:58:26Z

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

+            });
+        });
+    });
+    describe("getCurrentEpoch", () => {


What's your stance on using tests also as "live/code" documentation? Apart from natspecs, I generally use tests to explicitly state the expected behavior for situations that are core to the business (sometimes tests are even more clarifying that some comments in code).

This mindset is something along the lines of "I've thought about this situation, how it impacts the product/business/service and I expect this code to behave like the test indicates as I carefully decided this to be the best option". Helps to confirm from another side that the implemented code is actually doing what I want it to do.

Taking as an example a failure in the getCurrentEpoch, from the business side I guess that we want to specify with a test that we want expect it to fail if any of the contract calls inside the method fails, right?

Love this comment. Basically you mean that we should focus more on the behaviour that we expect for the business to write our tests or in the some way being more focused on the business specification, instead of blindly write tests for the code, right ?

Will add tests for those cases.

That's right.

I mean, blindly testing stuff somewhat works but it's extremely time consuming. Let's say that for this specific case, from the "business" side, we mainly care about the situation where at least one of the requests fails while getting the epoch block number; I think we don't have to blindly test what happens if the first request fails, if the second request fails and if both requests fails. Just stating with a test that we expect a failure when at least one request fails seems enough to (semantically) convey the whole message.

In an ideal world, we should read the test suite results and understand what our code intends to do.

0xkenj1 · 2024-07-25T04:10:09Z

packages/automated-dispute/src/exceptions/rpcUrlsEmpty.exception.ts

@@ -0,0 +1,6 @@
+export class RpcUrlsEmpty extends Error {


Guys, do you have any preference or thoughts on Exceptions/Errors naming convention ?

At the begining i named it as RpcUrlsEmptyException but then i realized that the subfix was adding some noise and was a bit redundant.

@0xyaco @0xnigir1

+1 to no Exception suffix. We can always alias the imports if needed.

0xyaco

Yay 😃

0xnigir1 · 2024-07-26T16:10:13Z

packages/automated-dispute/src/protocolProvider.ts

+    private oracleContract: GetContractReturnType<
+        typeof oracleAbi,
+        typeof this.client,
+        `0x${string}`


replace with Address type

0xnigir1 · 2024-07-26T16:10:16Z

packages/automated-dispute/src/protocolProvider.ts

+    private epochManagerContract: GetContractReturnType<
+        typeof epochManagerAbi,
+        typeof this.client,
+        `0x${string}`


noticed that viem already have the Address type, lets use their type to avoid code duplication

0xnigir1 · 2024-07-26T16:16:45Z

packages/automated-dispute/src/protocolProvider.ts

+        }
+        this.client = createPublicClient({
+            chain: arbitrum,
+            transport: fallback(rpcUrls.map((url) => http(url))),


0xnigir1 · 2024-07-26T16:21:22Z

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

+        it("throws when current epoch block request fails", async () => {
+            const protocolProvider = new ProtocolProvider(mockRpcUrls, mockContractAddress);
+            const currentEpochError = new Error("Failed to get current epoch");
+            const currentEpochBlockError = new Error("Failed to get current epoch block");
+
+            (protocolProvider["epochManagerContract"].read.currentEpoch as Mock).mockRejectedValue(
+                currentEpochError,
+            );
+            (
+                protocolProvider["epochManagerContract"].read.currentEpochBlock as Mock
+            ).mockRejectedValue(currentEpochBlockError);
+
+            await expect(protocolProvider.getCurrentEpoch()).rejects.toThrow(currentEpochError);
+        });


is this test case needed? Promise.all rejects on first promise rejection so i think with the individual rejection cases is enough

you are right, just wanted to make sure how does Promise.all work in case of having 2 rejects at the same time 🤣

i will remove it

0xnigir1 · 2024-07-26T16:23:43Z

tsconfig.base.json

@@ -18,6 +18,6 @@
        "module": "NodeNext",
        "sourceMap": true,
        /* If your code doesn't run in the DOM: */
-        "lib": ["es2022"]
+        "lib": ["es2022", "DOM"]


is this needed?

yes, without this we didn't have console.log

i think what is missing then is devDependency: @types/node because ebo-agent won't be a browser app right?

oh, let me check that

0xnigir1

let's goo 🚀

0xkenj1 added 2 commits July 24, 2024 17:10

feat: base class for protocol provider & getCurrentEpoch

e9fab29

feat: add ts-node & console.log

15d3493

0xkenj1 requested review from 0xyaco and 0xnigir1 July 24, 2024 20:38

Merge remote-tracking branch 'origin/dev' into feat/protocolProviderBase

c2fc0de

0xyaco reviewed Jul 25, 2024

View reviewed changes

0xkenj1 added 2 commits July 25, 2024 00:10

fix: add validation for rpcUrls on ProtocolProvider constructor

8b6d09f

fix: exception naming

ebdf787

0xkenj1 commented Jul 25, 2024

View reviewed changes

fix: tests

a7dd68b

0xkenj1 requested a review from 0xyaco July 25, 2024 14:54

0xyaco previously approved these changes Jul 25, 2024

View reviewed changes

0xnigir1 reviewed Jul 26, 2024

View reviewed changes

0xkenj1 added 2 commits July 26, 2024 15:11

fix: pr comments

5a25cf1

Merge remote-tracking branch 'origin/dev' into feat/protocolProviderBase

82b68bc

0xkenj1 dismissed 0xyaco’s stale review via 82b68bc July 26, 2024 18:13

0xkenj1 requested review from 0xnigir1 and 0xyaco July 26, 2024 18:13

fix: pr comments

bec87f3

0xnigir1 approved these changes Jul 26, 2024

View reviewed changes

0xyaco approved these changes Jul 26, 2024

View reviewed changes

0xkenj1 merged commit c2dc309 into dev Jul 29, 2024
5 checks passed

0xkenj1 deleted the feat/protocolProviderBase branch July 29, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: protocol provider base #12

feat: protocol provider base #12

0xkenj1 commented Jul 24, 2024

linear bot commented Jul 24, 2024

0xyaco left a comment

0xyaco Jul 25, 2024

0xyaco Jul 25, 2024

0xkenj1 Jul 25, 2024

0xyaco Jul 25, 2024

0xkenj1 Jul 25, 2024

0xyaco Jul 25, 2024

0xkenj1 Jul 25, 2024

0xyaco Jul 25, 2024

0xkenj1 Jul 25, 2024

0xyaco Jul 25, 2024

0xnigir1 Jul 26, 2024

0xyaco left a comment

0xnigir1 Jul 26, 2024

0xnigir1 Jul 26, 2024

0xkenj1 Jul 26, 2024

0xnigir1 Jul 26, 2024

0xnigir1 Jul 26, 2024

0xkenj1 Jul 26, 2024 •

edited

Loading

0xnigir1 Jul 26, 2024

0xkenj1 Jul 26, 2024

0xnigir1 Jul 26, 2024 •

edited

Loading

0xkenj1 Jul 26, 2024

0xnigir1 left a comment

	it("should create a new ProtocolProvider instance successfully", () => {
	it("creates a new ProtocolProvider instance successfully", () => {

feat: protocol provider base #12

feat: protocol provider base #12

Conversation

0xkenj1 commented Jul 24, 2024

🤖 Linear

Description

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

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

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

0xkenj1 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnigir1 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnigir1 left a comment

Choose a reason for hiding this comment

0xkenj1 Jul 26, 2024 •

edited

Loading

0xnigir1 Jul 26, 2024 •

edited

Loading