feat: Implement EBORequestCreator.createRequests function #35

jahabeebs · 2024-09-04T20:33:16Z

🤖 Linear

Closes GRT-140

Description

Adds a new createWalletClient that we initialize with a new privateKey argument to the constructor

My thinking for this is it's better to have separate read and write clients so that if we end up switching to different read RPCs or something it doesn't affect the write operations--or do you think it's better to consolidate?

Adds an implementation for createRequest and handles the errors specified in the error spec sheet: https://www.notion.so/defi-wonderland/EBO-error-handling-specs-18c8a59349cb43a4bf88b0be683f7a09

^ Note we're going to do a big error refactoring later so that we can handle the 80+ errors more efficiently

Vitest tests for the new exceptions & createRequest function
Added the new errors to eboActor.ts since these errors could occur when the createRequest event is emitted--not sure if we're ready to actually implement these errors other than the current logging though.

linear · 2024-09-04T20:33:18Z

GRT-140 Implement EBORequestCreator RPC calls

Create the RPC call to handle the following functions:

Implement request creation using EBORequestCreator.createRequests function (link)
Model all revert errors thrown by the smart contract function as they will be needed during the process handling

0xnigir1

left some comments, also i wanted to raise that we probably need to add Wonder Crypto Checks to the repo now the we are working with PKs: https://github.com/defi-wonderland/crypto-husky-checks

0xnigir1 · 2024-09-04T21:47:13Z

packages/automated-dispute/src/abis/eboRequestCreator.ts

+    { type: "error", name: "EBORequestCreator_ChainNotAdded", inputs: [] },
+    { type: "error", name: "EBORequestCreator_InvalidEpoch", inputs: [] },
+    { type: "error", name: "EBORequestCreator_InvalidNonce", inputs: [] },
+];


Suggested change

];

] as const;

without const assertion, you're missing viem type inference

The crypto-husky-checks install instructions didn't work, might need to add it manually--the key I added is a mock from the viem docs just fyi

added the const assertion

it failed? i think we have to tell Security team
cc @0xkenj1

0xnigir1 · 2024-09-04T21:56:04Z

packages/automated-dispute/src/protocolProvider.ts

+    constructor(
+        rpcUrls: string[],
+        contracts: ProtocolContractsAddresses,
+        privateKey: `0x${string}`,


it's nicer to use Hex type from Viem, wdyt?

0xnigir1 · 2024-09-04T21:58:22Z

packages/automated-dispute/src/protocolProvider.ts

+            if (!this.eboRequestCreatorContract?.write?.createRequests) {
+                throw new Error("createRequests function is not available on the ABI");
+            }


is this actually possible?

ended up not needing this validationwith the new way we structured it

0xyaco

Nice start! Left some comments

0xyaco · 2024-09-04T21:57:56Z

packages/automated-dispute/src/protocolProvider.ts

+    EBORequestCreator_InvalidEpoch,
+    EBORequestModule_InvalidRequester,
+    Oracle_InvalidRequestBody,
+} from "./exceptions/index.js";
 import { RpcUrlsEmpty } from "./exceptions/rpcUrlsEmpty.exception.js";
 import { ProtocolContractsAddresses } from "./types/protocolProvider.js";

 export class ProtocolProvider {
    private client: PublicClient<FallbackTransport<HttpTransport[]>>;


Maybe we should rename this to publicClient just to make it even more explicit

I changed it to readClient and writeClient--thoughts?

0xyaco · 2024-09-04T22:20:09Z

packages/automated-dispute/src/protocolProvider.ts

+            if (!this.eboRequestCreatorContract?.write?.createRequests) {
+                throw new Error("createRequests function is not available on the ABI");
+            }
+            await this.eboRequestCreatorContract.write.createRequests([epoch, chains]);


I might be wrong but ideally, we should first simulate[1] the transaction and then, if everything went as expected (ie no reverts) write it[2].

I think this can be achieved with something along the lines of:

const { result } = await this.eboRequestCreatorContract.simulate.createRequest([epoch, chains])

[1] https://viem.sh/docs/contract/simulateContract
[2] https://viem.sh/docs/contract/writeContract

We also need to wait for tx receipt to be sure things went ok! Parameters as timeout or number of confirms can be decided later. [3]

[3] https://viem.sh/docs/actions/public/waitForTransactionReceipt#waitfortransactionreceipt

Regarding first comment, there is no need to simulate, that might be useful for staticCalls, but we actually want to write on the contract.

@0xkenj1 there's a yellow block warning in the link [2] that states the following:

The writeContract internally sends a transaction – it does not validate if the contract write will succeed (the contract may throw an error). It is highly recommended to simulate the contract write with simulateContract before you execute it.

I could be getting things wrong here but what I understand from that piece of doc is that viem allows you to submit a write transaction regarding the final result (successful or reverted). The RPC will surely not modify any contract's internal state but it might consume gas + leave some transaction traces on-chain. Simulating could help us avoid that gas consumption (as an extra, simulating write calls uses the public client so definitely no gas consumed).

I agree with @0xyaco it's best to simulate the contract and this lets us just pass in that request to a write method afterward--there's no downside. I added the logic for this following the viem docs

0xyaco · 2024-09-04T22:26:17Z

packages/automated-dispute/src/protocolProvider.ts

+            if (error instanceof EBORequestCreator_InvalidEpoch) {
+                throw new EBORequestCreator_InvalidEpoch();
+            } else if (error instanceof Oracle_InvalidRequestBody) {
+                throw new Oracle_InvalidRequestBody();
+            } else if (error instanceof EBORequestModule_InvalidRequester) {
+                throw new EBORequestModule_InvalidRequester();
+            } else if (error instanceof EBORequestCreator_ChainNotAdded) {
+                throw new EBORequestCreator_ChainNotAdded();
+            } else {
+                throw error;
+            }


Throwing the idea out in the wild, open to discussion: what do you think about wrapping all these errors under one single error class that contains the revert data when the error is instanceof ContractFunctionRevertedError?[4]

I can only think about the revertName right now as the relevant properties for this single error but we might end up adding things like block number, RPC timestamp, stuff like that.

[4] https://viem.sh/docs/contract/simulateContract.html#handling-custom-errors

I agree we need to wrap these errors--and particularly in a way that is easier to mock because the way that viem suggests is pretty awkward:

catch (error) { if (error instanceof BaseError) { const revertError = error.walk( (err) => err instanceof ContractFunctionRevertedError, ); if (revertError instanceof ContractFunctionRevertedError) { const errorName = revertError.data?.errorName ?? ""; switch (errorName) { case "EBORequestCreator_InvalidEpoch": throw new EBORequestCreator_InvalidEpoch(); case "Oracle_InvalidRequestBody": throw new Oracle_InvalidRequestBody(); case "EBORequestModule_InvalidRequester": throw new EBORequestModule_InvalidRequester(); case "EBORequestCreator_ChainNotAdded": throw new EBORequestCreator_ChainNotAdded(); default: throw new Error(`Unknown error: ${errorName}`); } } } throw error; }

I think we should do the error refactoring in a separate issue because it'll become a lot if we start adding our existing errors to a new error class too

Sounds good!

0xyaco · 2024-09-04T22:29:02Z

packages/automated-dispute/tests/eboActor/fixtures.ts

@@ -2,9 +2,12 @@ import { Address } from "viem";

 import { Request, RequestId } from "../../src/types/prophet";

+export const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";


Hoping this is not a real private key 😛

viem uses this private key in their docs. I changed the name to mockedPrivateKey to avoid heart attacks 😉

0xkenj1 · 2024-09-04T22:19:04Z

packages/automated-dispute/tests/eboActor/fixtures.ts

@@ -2,9 +2,12 @@ import { Address } from "viem";

 import { Request, RequestId } from "../../src/types/prophet";

+export const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";


Suggested change

export const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";

export const mockedPrivateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";

0xkenj1 · 2024-09-04T23:00:52Z

packages/automated-dispute/src/protocolProvider.ts

+        } catch (error) {
+            if (error instanceof EBORequestCreator_InvalidEpoch) {
+                throw new EBORequestCreator_InvalidEpoch();
+            } else if (error instanceof Oracle_InvalidRequestBody) {
+                throw new Oracle_InvalidRequestBody();
+            } else if (error instanceof EBORequestModule_InvalidRequester) {
+                throw new EBORequestModule_InvalidRequester();
+            } else if (error instanceof EBORequestCreator_ChainNotAdded) {
+                throw new EBORequestCreator_ChainNotAdded();
+            } else {
+                throw error;
+            }


Not sure if this actually works. Take a look at this .

https://viem.sh/docs/contract/simulateContract#handling-custom-errors

In your code your are considering that viem already knows about errors, and thats imposible 🤣 . Probably viem when throws just uses a generic error like ContractRevertError with the error name of the custom error and the arguments. :)

In your code your are considering that viem already knows about errors, and thats imposible 🤣 . Probably viem when throws just uses a generic error like ContractRevertError with the error name of the custom error and the arguments. :)

Yes I thought viem was smart enough to bubble up the error since the custom error is in the ABI, and they do indeed pass the error name back--I changed my code to implement it the way recommended by viem:

catch (error) { if (error instanceof BaseError) { const revertError = error.walk( (err) => err instanceof ContractFunctionRevertedError, ); if (revertError instanceof ContractFunctionRevertedError) { const errorName = revertError.data?.errorName ?? ""; switch (errorName) { case "EBORequestCreator_InvalidEpoch": throw new EBORequestCreator_InvalidEpoch(); case "Oracle_InvalidRequestBody": throw new Oracle_InvalidRequestBody(); case "EBORequestModule_InvalidRequester": throw new EBORequestModule_InvalidRequester(); case "EBORequestCreator_ChainNotAdded": throw new EBORequestCreator_ChainNotAdded(); default: throw new Error(`Unknown error: ${errorName}`); } } } throw error; }

What about creating an error factory, just an idea 😉

Take a look at this example, we can do sth similar i think

class TransportFactory { public static createTransport(type: string): Transport { if (type === "car") { return new Car(); } else if (type === "bike") { return new Bike(); } else { throw new Error("Unknown transport type"); } } }```

jahabeebs · 2024-09-05T23:04:51Z

Changes

Error handling now using simulations and viem should now properly bubble up the errors--I think we'll tackle a separate error handling class in a different PR since we have errors throughout all the monorepos
Refactored to have a readClient and writeClient so we can make requests like this:

const { request } = await this.readClient.simulateContract({
                address: this.eboRequestCreatorContract.address,
                abi: eboRequestCreatorAbi,
                functionName: "createRequests",
                args: [epoch, chains],
                account: this.writeClient.account,
            });

            const hash = await this.writeClient.writeContract(request);

Similarly, added IWriteProvider and IReadProvider interfaces and the IProtocolProvider interface suggested by Kenji:

export interface IProtocolProvider {
    write: IWriteProvider;
    read: IReadProvider;
}

Left to do

Still need to replace the ABI with the canary versions of the prophet library, need to add the husky key sh script manually (package install instructions not working correctly) and any other suggestions you have

0xyaco · 2024-09-06T11:53:19Z

packages/automated-dispute/src/abis/eboRequestCreator.ts

@@ -406,4 +406,4 @@ export const eboRequestCreatorAbi = [
    { type: "error", name: "EBORequestCreator_ChainNotAdded", inputs: [] },
    { type: "error", name: "EBORequestCreator_InvalidEpoch", inputs: [] },
    { type: "error", name: "EBORequestCreator_InvalidNonce", inputs: [] },
-];
+] as const;


As a sidenote, as @0xkenj1 said somewhere in the offline chat, by using the Prophet packages we won't have a "simple" way to leverage the abi as const to feed viem with a typed ABI.

TypeScript refuses to cooperate with types while importing the JSON ABIs from the Prophet package so this dependency might be at first a dev dependency to just copy/paste the JSON ABIs values into our own:

// abis/Oracle.ts export const abi = <copy paste from package>;

Later we might get to preprocess the package's JSON ABIs to automate the generation of TypeScript abi/** const values but it's not high priority right now.

Hmm ok, for now I've left it with a const assertion and added the contracts as dev dependencies

0xyaco · 2024-09-06T11:56:31Z

packages/automated-dispute/src/protocolProvider.ts

+    private writeClient: WalletClient<FallbackTransport<HttpTransport[]>>;
+    private oracleContract: GetContractReturnType<
+        typeof oracleAbi,
+        typeof this.readClient,


Oracle will need to write (it might end up not needing read):

Suggested change

typeof this.readClient,

typeof this.writeClient,

0xyaco · 2024-09-06T11:58:49Z

packages/automated-dispute/src/protocolProvider.ts

        Address
    >;
    private eboRequestCreatorContract: GetContractReturnType<
        typeof eboRequestCreatorAbi,
-        typeof this.walletClient,
+        typeof this.writeClient,


Seeing the initialization of the eboRequestCreatorContract in the constructor, it's using both the read and write client.

Does this typeof this.writeClient include both read and write clients?

Yeah, the write client (WalletClient) is just the read client (PublicClient) with an account basically.

For example, I wrote this because I wanted to be explicit:

this.eboRequestCreatorContract = getContract({ address: contracts.eboRequestCreator, abi: eboRequestCreatorAbi, client: { public: this.readClient, wallet: this.writeClient, }, });

but I could have just changed the client line to

client: this.writeClient

0xyaco · 2024-09-06T12:03:12Z

packages/automated-dispute/src/protocolProvider.ts

+
+            const receipt = await this.readClient.waitForTransactionReceipt({
+                hash,
+                confirmations: 1,


Let's create a const value with this 1:

// TODO: env var const TRANSACTION_RECEIPT_CONFIRMATIONS = 1;

at the top of this class. It'll be one of the EBO agent config values.

0xyaco · 2024-09-06T12:12:46Z

packages/automated-dispute/src/protocolProvider.ts

        if (rpcUrls.length === 0) {
            throw new RpcUrlsEmpty();
        }

-        this.client = createPublicClient({
+        this.readClient = createPublicClient({


Now that I think, every client should have:

A defined timeout (source)

A defined retryCount (source)

Let's define consts at the top of the class and add for each of them the TODO comment

added them with their viem-specified defaults for now

0xyaco · 2024-09-06T12:56:15Z

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

 import { ProtocolContractsNames } from "../constants.js";

 export type ProtocolContract = (typeof ProtocolContractsNames)[number];
 export type ProtocolContractsAddresses = Record<ProtocolContract, Address>;
+
+export interface IReadProvider {


We've got the interfaces/ folder for these

0xyaco · 2024-09-06T12:57:05Z

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

+            const mockRpcUrls = ["http://localhost:8545"];
+            const mockContractAddress: ProtocolContractsAddresses = {
+                oracle: "0x1234567890123456789012345678901234567890",
+                epochManager: "0x1234567890123456789012345678901234567890",
+                eboRequestCreator: "0x1234567890123456789012345678901234567890",
+            };


You could declare these values outside the it calls but inside the describe so you can reutilize them

0xyaco

Just some minor comments and after that the PR is ready to go! 🚀

0xyaco · 2024-09-09T13:06:49Z

packages/automated-dispute/src/exceptions/errorFactory.ts

@@ -0,0 +1,31 @@
+import { EBORequestCreator_ChainNotAdded } from "./chainNotAdded.exception.js";


This is a service, even if it's an "error" related service. Let's move this class into the src/services folder, keeping the error classes inside src/exceptions.

0xyaco · 2024-09-09T13:08:23Z

packages/automated-dispute/src/exceptions/errorFactory.ts

+        switch (errorName) {
+            case "EBORequestCreator_InvalidEpoch":
+                return new EBORequestCreator_InvalidEpoch();
+            case "Oracle_InvalidRequestBody":
+                return new Oracle_InvalidRequestBody();
+            case "EBORequestModule_InvalidRequester":
+                return new EBORequestModule_InvalidRequester();
+            case "EBORequestCreator_ChainNotAdded":
+                return new EBORequestCreator_ChainNotAdded();
+            default:
+                return new Error(`Unknown error: ${errorName}`);
+        }


Let's add a TODO here as we have not defined yet the structure of each error (should they have properties inside? should they be merged into a single EboContractReverted (or similar) class? etc...)

0xyaco · 2024-09-09T13:09:36Z

packages/automated-dispute/src/protocolProvider.ts

+// TODO: these constants should be env vars
+const TRANSACTION_RECEIPT_CONFIRMATIONS = 1;
+const TIMEOUT = 10000;
+const RETRY_INTERVAL = 150;


0xyaco · 2024-09-09T13:10:30Z

packages/automated-dispute/src/protocolProvider.ts

-                        default:
-                            throw new Error(`Unknown error: ${errorName}`);
-                    }
+                    throw ErrorFactory.createError(errorName);


0xyaco · 2024-09-09T13:13:21Z

packages/automated-dispute/src/exceptions/errorFactory.ts

+            case "EBORequestCreator_ChainNotAdded":
+                return new EBORequestCreator_ChainNotAdded();
+            default:
+                return new Error(`Unknown error: ${errorName}`);


As an extra, we'll probably want to define a "base" contract reverted error here so we are able to distinguish it from other errors. Feel free to add another TODO as we'll tackle this during the core error handling implementation in another PR

0xnigir1 · 2024-09-09T13:17:28Z

it's looking good to me 💎 but let's wait for Yaco and Kenji reviews too that digged deeper on the PR

regarding the husky-check, add a Linear issue to not forget about it but we'll have to wait for it to be fixed as you said 👍

…calls

0xyaco

GO GO GO 🫱 🫱 🫱

0xkenj1

Cograts! First PR 🚀

0xnigir1

🫡🫡

jahabeebs added 7 commits September 4, 2024 12:54

feat: add new exceptions and createRequests

b506597

chore: merge dev

97bff8c

feat: fix tests add docs

6f2a0de

feat: add tests

4e85924

chore: no env

20c6636

fix: no generic revert

24ca7b1

fix: add back deleted tests

9bd4286

jahabeebs requested review from 0xkenj1, 0xnigir1 and 0xyaco September 4, 2024 20:33

jahabeebs self-assigned this Sep 4, 2024

0xnigir1 requested changes Sep 4, 2024

View reviewed changes

0xyaco reviewed Sep 4, 2024

View reviewed changes

0xkenj1 reviewed Sep 4, 2024

View reviewed changes

feat: add new protocolProvider logic

e548352

jahabeebs requested review from 0xkenj1, 0xyaco and 0xnigir1 September 5, 2024 23:05

fix: add back deleted tests

1696d48

0xyaco reviewed Sep 6, 2024

View reviewed changes

jahabeebs added 7 commits September 6, 2024 13:01

feat: address PR comments

f296f4d

fix: merge conflicts

4e42502

fix: more merge fixes

0432968

feat: add error factory and tests

e6bdfcc

chore: add contracts as dev dependencies

ff5ab29

chore: refactor constants

d813f46

chore: write client for oracle

4bd8c86

jahabeebs requested a review from 0xyaco September 6, 2024 21:29

0xyaco reviewed Sep 9, 2024

View reviewed changes

jahabeebs added 2 commits September 10, 2024 10:44

fix: address pr comments

848efa7

Merge branch 'dev' into feat/grt-140-implement-eborequestcreator-rpc-…

f77aad9

…calls

jahabeebs requested a review from 0xyaco September 10, 2024 15:48

0xyaco approved these changes Sep 10, 2024

View reviewed changes

0xkenj1 approved these changes Sep 11, 2024

View reviewed changes

0xnigir1 approved these changes Sep 11, 2024

View reviewed changes

jahabeebs merged commit baeea02 into dev Sep 11, 2024
5 checks passed

jahabeebs deleted the feat/grt-140-implement-eborequestcreator-rpc-calls branch September 11, 2024 18:07

		@@ -2,9 +2,12 @@ import { Address } from "viem";

		import { Request, RequestId } from "../../src/types/prophet";

		export const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";

	export const privateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";
	export const mockedPrivateKey = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";

		@@ -0,0 +1,31 @@
		import { EBORequestCreator_ChainNotAdded } from "./chainNotAdded.exception.js";

feat: Implement EBORequestCreator.createRequests function #35

feat: Implement EBORequestCreator.createRequests function #35

Conversation

jahabeebs commented Sep 4, 2024

🤖 Linear

Description

linear bot commented Sep 4, 2024

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

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

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

0xkenj1 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xkenj1 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahabeebs commented Sep 5, 2024

Changes

Left to do

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

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

0xnigir1 commented Sep 9, 2024 • edited Loading

0xyaco left a comment

Choose a reason for hiding this comment

0xkenj1 left a comment

Choose a reason for hiding this comment

0xnigir1 left a comment

Choose a reason for hiding this comment

0xkenj1 Sep 4, 2024 •

edited

Loading

0xkenj1 Sep 5, 2024 •

edited

Loading

0xnigir1 commented Sep 9, 2024 •

edited

Loading