feat: implement oracle rpc calls #39

jahabeebs · 2024-09-11T19:26:05Z

🤖 Linear

Closes GRT-141

Description

Added proposeResponse, disputeResponse, escalateDispute, and finalize to protocolProvider
Added the names of all the errors corresponding to each of these methods in errorFactory.ts, however we decided we're not going to make a separate error class for each error some I'm leaving the names for now and will implement them when we do the error refactoring

linear · 2024-09-11T19:26:08Z

GRT-141 Implement Oracle RPC calls

0xyaco

Cool! Some changes needed, leaving a short summary below.

Double check that the smart contract function signatures that we are using correspond to our viem calls (the Linear task might be helpful here, as it has the respective link to each of those function signatures)
Create a custom error for a receipt status not being === success and apply it into every ProtocolProvider method
Add some more testing cases

0xyaco · 2024-09-11T22:22:31Z

packages/automated-dispute/src/protocolProvider.ts

+                address: this.oracleContract.address,
+                abi: oracleAbi,
+                functionName: "proposeResponse",
+                args: [requestId, { epoch, chainId, block: blockNumber }],


The old function signature might have been a little bit deprecated.

Based on the Oracle function we might be ok using Request["prophetData"] and Response["prophetData"]

yep, I double checked the interfaces and had to update a couple of their protocolProvider interfaces and implementations

0xyaco · 2024-09-11T22:37:19Z

packages/automated-dispute/src/protocolProvider.ts

+            });
+
+            if (receipt.status !== "success") {
+                throw new Error("Transaction failed");


We'll also need to handle this scenario too when processing the event that triggered this action.

Let's add a named error for this (probably TransactionExecutionError) as it needs to be handled by the EboProcessor by retrying the transaction.

Also, given that this error is most likely not going to be a child class of ContractFunctionRevertedError we'll (probably) end up modifying the method we have/adding a new method to the ErrorFactory class:

// ErrorFactory.ts static async createTransactionError(error) { ... } static async createContractRevertedError(error) { ... }

For these last ErrorFactory changes mentioned, let's wait until we have a solid design for the error handling and we'll come back to it after that.

Ok, for now I added a TransactionExecutionError w/test cases

0xyaco · 2024-09-11T22:40:18Z

packages/automated-dispute/src/protocolProvider.ts

+                address: this.oracleContract.address,
+                abi: oracleAbi,
+                functionName: "disputeResponse",
+                args: [requestId, responseId, proposer],


Same here, based on the contract function signatures we should use Request["prophetData"], Response["prophetData'}, Dispute["prophetData"].

I'm surprised that the types are not being enforced by viem with the help of the TypeScript.

0xyaco · 2024-09-11T22:43:26Z

packages/automated-dispute/src/protocolProvider.ts

+            });
+
+            if (receipt.status !== "success") {
+                throw new Error("Transaction failed");


Custom error here!

0xyaco · 2024-09-11T22:44:27Z

packages/automated-dispute/src/protocolProvider.ts


+            if (receipt.status !== "success") {
+                throw new Error("Transaction failed");


Custom error here!

0xyaco · 2024-09-11T22:44:35Z

packages/automated-dispute/src/protocolProvider.ts

+            });
+
+            if (receipt.status !== "success") {
+                throw new Error("Transaction failed");


Custom error here!

0xyaco · 2024-09-11T22:49:00Z

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

+            await expect(
+                protocolProvider.proposeResponse("0x123", 1n, "eip155:1" as Caip2ChainId, 100n),
+            ).resolves.not.toThrow();
+        });


Let's test failures too when:

Transaction couldn't be confirmed

A ContractFunctionRevertedError is thrown by viem

Can the waitForTransactionReceipt throw a timeout error or something like that? Let's test that too

Testing failure behavior (specially in this class) is almost as important as the happy path. This applies to every Oracle provider method implemented.

✅ added these for proposeResponse

jahabeebs · 2024-09-12T19:16:12Z

@0xyaco I think I've addressed all the PR comments but have some questions about changes I had to make to eboActor

for example, in proposeResponse I had to refactor to this:

  const response: Response["prophetData"] = {
            // TODO: check if this is the correct proposer
            proposer: request.prophetData.requester,
            requestId: request.id,
            response: responseBody,
        };

but I don't think this is the correct way to get the proposer.

We're getting the proposer in another function by doing

  const eventResponse = event.metadata.response;

then

proposer: eventResponse.proposer

but proposeResponse in eboActor is taking a chainId rather than an event...

similarly, in onResponseProposed I refactored to:

  const dispute: Dispute["prophetData"] = {
            // TODO: check if this is the correct disputer
            disputer: this.actorRequest.id,
            proposer: eventResponse.proposer,
            responseId: event.metadata.responseId,
            requestId: request.id,
        };

but to get the disputer we would do dispute.prophetData.disputer and in onResponseProposed we're only getting an event

0xyaco

Great changes, left some other comments!

0xyaco · 2024-09-12T20:35:05Z

packages/automated-dispute/src/eboActor.ts

        }

+        const response: Response["prophetData"] = {
+            // TODO: check if this is the correct proposer
+            proposer: request.prophetData.requester,


The proposer must be the address of the transaction sender as Prophet actually checks that the sender === propose.proposer [1].

Viem has some methods that might be helpful for this like getAddresses [2] [3].

[1] https://github.com/defi-wonderland/prophet-core/blob/0954a47f7dbd225ee0d48c9face954a7db6aed30/solidity/contracts/Oracle.sol#L118-L119
[2] https://viem.sh/docs/clients/wallet.html#account-optional
[3] https://viem.sh/docs/actions/wallet/getAddresses.html

✅ I added a new function to protocolProvider to get the writeClient account address, added a new custom error and some tests

0xyaco · 2024-09-12T20:36:53Z

packages/automated-dispute/src/eboActor.ts

+
+        const dispute: Dispute["prophetData"] = {
+            // TODO: check if this is the correct disputer
+            disputer: this.actorRequest.id,


Same as the proposeResponse case. The disputer is the transaction submitter address.

0xyaco · 2024-09-12T20:41:19Z

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

+        _request: Request["prophetData"],
+        _response: Response["prophetData"],


Just a small detail, we want to remove those _ prefixes for arguments.

I think they were left there to be consistent with the methods being defined but not implemented (so the params had to be prefixed with an underscore so ESLint could pass).

0xyaco · 2024-09-12T20:41:28Z

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

+        _request: Request["prophetData"],
+        _response: Response["prophetData"],
+        _dispute: Dispute["prophetData"],


✅ got rid of the _ in the whole file--no lint issues

0xyaco · 2024-09-12T20:44:59Z

packages/automated-dispute/tests/eboActor/onDisputeStatusChanged.spec.ts

-            response.prophetData.response.block + 1n,
+            expect.objectContaining({}),
+            expect.objectContaining({
+                proposer: expect.any(String),


Remember to update this when setting up the proposer, super important to have this also validated

0xyaco · 2024-09-12T20:45:16Z

packages/automated-dispute/tests/eboActor/onRequestCreated.spec.ts

-                    indexedEpochBlockNumber,
+                    expect.objectContaining(request.prophetData),
+                    expect.objectContaining({
+                        proposer: expect.any(String),


Test proposer value here too

0xyaco · 2024-09-12T20:47:11Z

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

+            const mockRequest = {} as Request["prophetData"];
+            const mockResponse = {} as Response["prophetData"];


As a sidenote, if for some reason you need to build objects populated with proper data, you have the mock builders for requests, responses and disputes at tests/mocks/eboActor.ts

In the file I replaced all the blank {} with the mocked request in fixtures and added a mocked response and dispute

0xyaco · 2024-09-12T20:52:09Z

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

+            );
+
+            (protocolProvider["readClient"].waitForTransactionReceipt as Mock).mockRejectedValue(
+                new Error("Transaction receipt timeout"),


Check this out, proper error for this scenario!

https://viem.sh/docs/glossary/errors#waitfortransactionreceipttimeouterror

thanks, made it more accurate

0xyaco

Just one last comment and you've got the green light on my end 💯

0xyaco · 2024-09-13T18:55:01Z

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

@@ -72,11 +82,23 @@ describe("ProtocolProvider", () => {
                },
            }),
            getBlock: vi.fn(),
-            waitForTransactionReceipt: vi.fn().mockResolvedValue({ status: "success" }),
+            waitForTransactionReceipt: vi.fn().mockImplementation(async ({ hash }) => {
+                await new Promise((resolve) => setTimeout(resolve, 100));


Do you need this timeout here? We don't want to simulate any delay during unit tests, let's just make the function instantly resolve to the return value we want to use.

no it's ok to take out--I removed it

0xyaco · 2024-09-13T19:01:23Z

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

-            const mockRequest = {} as Request["prophetData"];
-            const mockResponse = {} as Response["prophetData"];
+            const mockRequestProphetData = DEFAULT_MOCKED_REQUEST_CREATED_DATA.prophetData;
+            const mockResponseProphetData = DEFAULT_MOCKED_RESPONSE_DATA.prophetData;


No need to change this in this PR, just as a side note.

It might be better to use the mocks.buildResponse(request) helper function here to make it clear that there's a link between the response and the request that you're using.

Suggested change

const mockResponseProphetData = DEFAULT_MOCKED_RESPONSE_DATA.prophetData;

const mockRequest = DEFAULT_MOCKED_RESPONSE_DATA;

const mockResponse = mocks.buildResponse(mockRequest);

await expect(

protocolProvider(mockRequest.prophetData, mockResponse.prophetData)

)

Here you've got that helper function in action.

0xyaco · 2024-09-13T19:06:08Z

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

+            const expectedAddress = privateKeyToAccount(mockedPrivateKey).address;
+            expect(protocolProvider.getAccountAddress()).toBe(expectedAddress);


0xyaco

Let's gooooo! 🚀 🚀

0xnigir1

niceee 🚀

jahabeebs added 2 commits September 11, 2024 14:18

feat: add new methods to protocolProvider

827b90c

chore: clean up comments

2215c57

jahabeebs self-assigned this Sep 11, 2024

jahabeebs added 2 commits September 11, 2024 14:57

fix: fix test

501eb25

fix: clean up test

5feae91

jahabeebs requested review from 0xyaco, 0xkenj1 and 0xnigir1 September 11, 2024 20:08

0xyaco requested changes Sep 11, 2024

View reviewed changes

jahabeebs added 3 commits September 12, 2024 13:51

feat: add new function defs and tests

6f547be

feat: more tests

1ced83a

fix: no should

cf1519c

0xyaco reviewed Sep 12, 2024

View reviewed changes

feat: address PR comments

09a7d83

jahabeebs requested a review from 0xyaco September 13, 2024 18:42

fix: casing

102864f

0xyaco reviewed Sep 13, 2024

View reviewed changes

jahabeebs added 4 commits September 13, 2024 14:41

chore: rename file on remote

706054b

fix: no timeout

e8976ca

fix: complete merge

27e95e8

feat: add back changes

d84a6a7

jahabeebs requested a review from 0xyaco September 13, 2024 20:10

0xyaco approved these changes Sep 13, 2024

View reviewed changes

0xnigir1 approved these changes Sep 16, 2024

View reviewed changes

jahabeebs merged commit e58ef51 into dev Sep 16, 2024
5 checks passed

jahabeebs deleted the feat/grt-141-implement-oracle-rpc-calls branch September 16, 2024 15:29


		if (receipt.status !== "success") {
		throw new Error("Transaction failed");

		_request: Request["prophetData"],
		_response: Response["prophetData"],

		const mockRequest = {} as Request["prophetData"];
		const mockResponse = {} as Response["prophetData"];

-            const mockResponseProphetData = DEFAULT_MOCKED_RESPONSE_DATA.prophetData;
+            const mockRequest = DEFAULT_MOCKED_RESPONSE_DATA;
+            const mockResponse = mocks.buildResponse(mockRequest);
+            await expect(
+                protocolProvider(mockRequest.prophetData, mockResponse.prophetData)
+            )

		const expectedAddress = privateKeyToAccount(mockedPrivateKey).address;
		expect(protocolProvider.getAccountAddress()).toBe(expectedAddress);

feat: implement oracle rpc calls #39

feat: implement oracle rpc calls #39

Conversation

jahabeebs commented Sep 11, 2024 • edited Loading

🤖 Linear

Description

linear bot commented Sep 11, 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 Sep 11, 2024 • edited Loading

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 Sep 12, 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

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

0xyaco left a comment

Choose a reason for hiding this comment

0xnigir1 left a comment

Choose a reason for hiding this comment

jahabeebs commented Sep 11, 2024 •

edited

Loading

0xyaco Sep 11, 2024 •

edited

Loading

jahabeebs commented Sep 12, 2024 •

edited

Loading