feat: fetch pledges for a dispute before settling or escalating #82

jahabeebs · 2024-10-31T21:49:52Z

🤖 Linear

Closes GRT-194

Description

Idea is to avoid BondEscalationModule_ShouldBeEscalated by fetching pledges for a dispute before trying to settle or escalate

linear · 2024-10-31T21:49:55Z

GRT-194 Scenario 1.4 - Escalate dispute to Arbitrator

GRT-145 Fetch pledges for a dispute before settling or escalating

0xyaco

Sweet PR! Left some comments

0xyaco · 2024-11-01T09:17:06Z

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

+    private decodeBondEscalationStatus(status: number): BondEscalationStatus {
+        switch (status) {
+            case 0:
+                return "Active";
+            case 1:
+                return "Resolved";
+            case 2:
+                return "Escalated";
+            case 3:
+                return "NoResolution";
+            default:
+                throw new Error(`Unknown BondEscalationStatus: ${status}`);
+        }
+    }


Let's move this method to ProphetCodec class and let's make it throw a ProphetDecodingError (you'll need to pull recently merged changes I think)

0xyaco · 2024-11-01T09:19:39Z

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

+            if (amountFor > amountAgainst) {
+                await this.protocolProvider.settleDispute(
+                    request.prophetData,
+                    response.prophetData,
+                    dispute.prophetData,
+                );
+
+                this.logger.info(`Dispute ${dispute.id} settled.`);
+            } else if (amountFor <= amountAgainst) {


Any particular reason to be using less than and gte?

Suggested change

if (amountFor > amountAgainst) {

await this.protocolProvider.settleDispute(

request.prophetData,

response.prophetData,

dispute.prophetData,

);

this.logger.info(`Dispute ${dispute.id} settled.`);

} else if (amountFor <= amountAgainst) {

if (amountFor !=== amountAgainst) {

await this.protocolProvider.settleDispute(

request.prophetData,

response.prophetData,

dispute.prophetData,

);

this.logger.info(`Dispute ${dispute.id} settled.`);

} else if (amountFor === amountAgainst) {

🤦🏻‍♂️ I don't know why I thought escalation would happen if the against was greater but thanks for catching that

0xyaco · 2024-11-01T09:26:49Z

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

@@ -489,42 +489,30 @@ describe("EboActor", () => {
    });

    describe("settleDispute", () => {
-        it("escalates dispute when BondEscalationModule_ShouldBeEscalated error occurs", async () => {
+        it("escalates dispute when amountOfPledgesForDispute <= amountOfPledgesAgainstDispute", async () => {


Remember to change this test and the next one too:

Suggested change

it("escalates dispute when amountOfPledgesForDispute <= amountOfPledgesAgainstDispute", async () => {

it("escalates dispute when amountOfPledgesForDispute === amountOfPledgesAgainstDispute", async () => {

0xyaco · 2024-11-01T09:27:05Z

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

+            vi.spyOn(protocolProvider.read, "getEscalation").mockResolvedValue({
+                disputeId: dispute.id,
+                status: "Active",
+                amountOfPledgesForDispute: BigInt(5),
+                amountOfPledgesAgainstDispute: BigInt(10),


Sweet case for DI advantages here 😎

I tried doing DI with buildEboActor and passing in a protocolProvider with custom getEscalation but I actually think this isn't ideal here because the eboActor mocks file has so many default vi.spyOn statements already that if we were to refactor buildEboActor now to pass in a custom protocolProvider we would have to define all of those vi.spyOn statements already in eboActor.mocks.ts in the custom protocolProvider object that we pass in like...

const mockProtocolProvider = { read: { getEscalation: vi.fn().mockResolvedValue({ disputeId: dispute.id, status: "Active", amountOfPledgesForDispute: BigInt(10), amountOfPledgesAgainstDispute: BigInt(5), }), }, getCurrentEpoch: vi.fn().mockResolvedValue({ number: BigInt(1), firstBlockNumber: BigInt(100), startTimestamp: BigInt(Date.now()) as UnixTimestamp, }), settleDispute: vi.fn().mockResolvedValue(undefined), // a ton more methods here } as unknown as ProtocolProvider;

so, if we were to accept a custom protocolProvider object in buildEboActor we would not only have to refactor buildEboActor to not invoke a bunch of the existing vi.spyOn calls we already have there but we'd also have to pass in much larger mock protocolProvider objects in each of our tests rather than using the defaults we have in buildEboActor already

Oh yeah, I was just praising the usage of protocolProvider as a dependency! Which made the test super easy to test with this single mock.

0xyaco · 2024-11-01T09:40:21Z

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

@@ -421,13 +421,28 @@ export class EboActor {
        try {
            this.logger.info(`Settling dispute ${dispute.id}...`);

-            // OPTIMIZE: check for pledges to potentially save the ShouldBeEscalated error
+            const escalationData = await this.protocolProvider.read.getEscalation(request.id);


We should wrap this protocolProvider call in a different try-catch, log the error and return as we don't want to compromise the whole actor because a read operation was not successful.

Suggested change

const escalationData = await this.protocolProvider.read.getEscalation(request.id);

try {

const escalationData = await this.protocolProvider.read.getEscalation(request.id);

} catch (err) {

// log

return; // Will be retried again later

}

try {

// Logic to settle/escalate

}

0xyaco · 2024-11-01T09:41:18Z

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

            await expect(actor["settleDispute"](request, response, dispute)).rejects.toThrow(
                settleError,
            );
        });
+
+        it("settles dispute when amountOfPledgesForDispute > amountOfPledgesAgainstDispute", async () => {


Suggested change

it("settles dispute when amountOfPledgesForDispute > amountOfPledgesAgainstDispute", async () => {

it("settles dispute when amountOfPledgesForDispute !== amountOfPledgesAgainstDispute", async () => {

0xyaco · 2024-11-01T09:43:54Z

packages/automated-dispute/tests/services/eboActor/onLastBlockupdated.spec.ts

-            const responseToSettle = mocks.buildResponse(request, { id: "0x10" as ResponseId });
-
+            const responseToSettle = mocks.buildResponse(request, {
+                id: ("0x" + "02".repeat(32)) as ResponseId,


I think you could also use viem's pad function; just wanted to mention it!

Suggested change

id: ("0x" + "02".repeat(32)) as ResponseId,

id: pad("0x02") as ResponseId,

oops forgot about that, thanks! I replaced all my .replace statements with this

0xnigir1

nice ser, nothing to add to Yaco's comments

…dges-before # Conflicts: # packages/automated-dispute/tests/mocks/eboActor.mocks.ts # packages/automated-dispute/tests/services/eboActor/onDisputeStatusUpdated.spec.ts

0xyaco

Two minor tweaks and it's ready to go!

0xyaco · 2024-11-04T08:03:44Z

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

+            case 3:
+                return "NoResolution";
+            default:
+                throw new ProphetDecodingError(id, toHex(status.toString()));


This ID is to help us identify the property that was decoded!

Suggested change

throw new ProphetDecodingError(id, toHex(status.toString()));

throw new ProphetDecodingError("escalation.status", toHex(status.toString()));

You can remove the id param from this method too.

0xyaco · 2024-11-04T08:05:41Z

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

@@ -564,22 +564,22 @@ describe("EboActor", () => {
            );
        });

-        it("settles dispute when amountOfPledgesForDispute > amountOfPledgesAgainstDispute", async () => {
+        it("settles dispute when amountOfPledgesForDispute !== amountOfPledgesAgainstDispute", async () => {


Given that this is an extremely core case, let's explicitly test both cases here: > and <

✅ I split this into two test cases

# 🤖 Linear Closes GRT-194 ## Description - Escalates dispute to arbitrator

feat: fetch pledges before disputing or escalating

7c779a5

jahabeebs requested review from 0xkenj1, 0xnigir1 and 0xyaco October 31, 2024 21:49

jahabeebs self-assigned this Oct 31, 2024

0xyaco reviewed Nov 1, 2024

View reviewed changes

0xnigir1 reviewed Nov 1, 2024

View reviewed changes

jahabeebs added 2 commits November 1, 2024 22:01

Merge remote-tracking branch 'origin/dev' into feat/grt-145-fetch-ple…

563dfd0

…dges-before # Conflicts: # packages/automated-dispute/tests/mocks/eboActor.mocks.ts # packages/automated-dispute/tests/services/eboActor/onDisputeStatusUpdated.spec.ts

fix: pr comments

ec7aebe

jahabeebs requested review from 0xyaco and 0xnigir1 November 2, 2024 04:09

0xyaco reviewed Nov 4, 2024

View reviewed changes

chore: refactor some tests and decode bond escalation

f3ca2c7

jahabeebs requested a review from 0xyaco November 4, 2024 16:30

0xyaco previously approved these changes Nov 4, 2024

View reviewed changes

0xnigir1 previously approved these changes Nov 4, 2024

View reviewed changes

chore: escalate dispute to arbitrator (#83)

3d9399f

# 🤖 Linear Closes GRT-194 ## Description - Escalates dispute to arbitrator

jahabeebs dismissed stale reviews from 0xnigir1 and 0xyaco via 3d9399f November 4, 2024 19:36

chore: skip e2e

70db796

jahabeebs requested review from 0xnigir1 and 0xyaco November 4, 2024 19:48

0xyaco approved these changes Nov 4, 2024

View reviewed changes

0xnigir1 approved these changes Nov 4, 2024

View reviewed changes

jahabeebs merged commit 436a9a8 into dev Nov 4, 2024
5 checks passed

jahabeebs deleted the feat/grt-145-fetch-pledges-before branch November 4, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fetch pledges for a dispute before settling or escalating #82

feat: fetch pledges for a dispute before settling or escalating #82

jahabeebs commented Oct 31, 2024

linear bot commented Oct 31, 2024

0xyaco left a comment

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xyaco Nov 4, 2024

0xyaco Nov 1, 2024 •

edited

Loading

jahabeebs Nov 2, 2024

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xyaco Nov 1, 2024

jahabeebs Nov 2, 2024

0xnigir1 left a comment

0xyaco left a comment

0xyaco Nov 4, 2024

jahabeebs Nov 4, 2024

0xyaco Nov 4, 2024

jahabeebs Nov 4, 2024

	it("escalates dispute when amountOfPledgesForDispute <= amountOfPledgesAgainstDispute", async () => {
	it("escalates dispute when amountOfPledgesForDispute === amountOfPledgesAgainstDispute", async () => {

-            const escalationData = await this.protocolProvider.read.getEscalation(request.id);
+    try {
+        const escalationData = await this.protocolProvider.read.getEscalation(request.id);
+    } catch (err) {
+        // log
+        return; // Will be retried again later
+    }
+    try {
+        // Logic to settle/escalate
+    }

	it("settles dispute when amountOfPledgesForDispute > amountOfPledgesAgainstDispute", async () => {
	it("settles dispute when amountOfPledgesForDispute !== amountOfPledgesAgainstDispute", async () => {

	id: ("0x" + "02".repeat(32)) as ResponseId,
	id: pad("0x02") as ResponseId,

	throw new ProphetDecodingError(id, toHex(status.toString()));
	throw new ProphetDecodingError("escalation.status", toHex(status.toString()));

feat: fetch pledges for a dispute before settling or escalating #82

feat: fetch pledges for a dispute before settling or escalating #82

Conversation

jahabeebs commented Oct 31, 2024

🤖 Linear

Description

linear bot commented Oct 31, 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

0xyaco Nov 1, 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

0xnigir1 left a 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 Nov 1, 2024 •

edited

Loading