From a2012836fb8cc1146ccba0f6978f7d9387696cc4 Mon Sep 17 00:00:00 2001 From: jahabeebs <47253537+jahabeebs@users.noreply.github.com> Date: Fri, 20 Sep 2024 15:38:13 -0500 Subject: [PATCH] feat: change processEvents logic, remove consume, fix remaining tests Signed-off-by: jahabeebs <47253537+jahabeebs@users.noreply.github.com> --- .../src/exceptions/errorFactory.ts | 82 +++++++++---------- .../src/exceptions/errorHandler.ts | 6 +- .../src/services/eboActor.ts | 60 +++++--------- .../automated-dispute/src/types/errorTypes.ts | 2 +- .../tests/services/eboActor.spec.ts | 19 ++++- 5 files changed, 81 insertions(+), 88 deletions(-) diff --git a/packages/automated-dispute/src/exceptions/errorFactory.ts b/packages/automated-dispute/src/exceptions/errorFactory.ts index 75bff67..222e301 100644 --- a/packages/automated-dispute/src/exceptions/errorFactory.ts +++ b/packages/automated-dispute/src/exceptions/errorFactory.ts @@ -7,7 +7,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -15,7 +15,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -23,7 +23,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -31,7 +31,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -42,7 +42,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -50,7 +50,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -61,7 +61,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -72,7 +72,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: true, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -80,7 +80,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -88,7 +88,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: true, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -96,7 +96,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -104,7 +104,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -112,7 +112,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: true, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -120,7 +120,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -128,7 +128,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -136,7 +136,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -144,7 +144,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -152,7 +152,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -160,7 +160,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -168,7 +168,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -176,7 +176,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -187,7 +187,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -198,7 +198,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -206,7 +206,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -214,7 +214,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeResponse(context.response.id); }, @@ -225,7 +225,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -233,7 +233,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, customAction: async (context) => { context.registry.removeDispute(context.dispute.id); }, @@ -244,7 +244,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: true, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -252,7 +252,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -260,7 +260,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -268,7 +268,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -276,7 +276,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -284,7 +284,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -292,7 +292,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -300,7 +300,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -308,7 +308,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -316,7 +316,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, }, ], [ @@ -324,7 +324,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: true, shouldTerminate: false, - shouldConsume: false, + shouldReenqueue: false, // Custom action to escalate dispute is implemented in eboActor.ts }, ], @@ -333,7 +333,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], [ @@ -341,7 +341,7 @@ const errorStrategiesEntries: [string, ErrorHandlingStrategy][] = [ { shouldNotify: false, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }, ], ]; @@ -356,7 +356,7 @@ export class ErrorFactory { return new CustomContractError(errorName, { shouldNotify: true, shouldTerminate: false, - shouldConsume: true, + shouldReenqueue: true, }); } diff --git a/packages/automated-dispute/src/exceptions/errorHandler.ts b/packages/automated-dispute/src/exceptions/errorHandler.ts index d043888..48f8b3c 100644 --- a/packages/automated-dispute/src/exceptions/errorHandler.ts +++ b/packages/automated-dispute/src/exceptions/errorHandler.ts @@ -25,9 +25,9 @@ export class ErrorHandler { } } - if (strategy.shouldConsume) { - if (context.consumeEvent) { - context.consumeEvent(); + if (strategy.shouldReenqueue) { + if (context.reenqueueEvent) { + context.reenqueueEvent(); } } } diff --git a/packages/automated-dispute/src/services/eboActor.ts b/packages/automated-dispute/src/services/eboActor.ts index 1268fa3..174522e 100644 --- a/packages/automated-dispute/src/services/eboActor.ts +++ b/packages/automated-dispute/src/services/eboActor.ts @@ -150,10 +150,11 @@ export class EboActor { this.logger.error(`Error processing event ${event.name}: ${err}`); if (err instanceof CustomContractError) { - if (!err.strategy.shouldConsume) { + if (err.strategy.shouldReenqueue) { this.eventsQueue.push(event); updateStateCommand.undo(); - return; + // Break to prevent immediate re-processing + break; } if (err.strategy.shouldTerminate) { @@ -301,9 +302,6 @@ export class EboActor { }); await ErrorHandler.handle(customError, { - consumeEvent: () => { - this.logger.info(`Consuming error: ${customError.name}`); - }, terminateActor: () => { throw customError; }, @@ -367,33 +365,29 @@ export class EboActor { * @param blockNumber block number to check if the dispute is to be settled */ private async settleDisputes(blockNumber: bigint): Promise { - try { - const request = this.getActorRequest(); - const disputes: Dispute[] = this.getActiveDisputes(); + const request = this.getActorRequest(); + const disputes: Dispute[] = this.getActiveDisputes(); - const settledDisputes = disputes.map(async (dispute) => { - const responseId = dispute.prophetData.responseId; - const response = this.registry.getResponse(responseId); + const settledDisputes = disputes.map(async (dispute) => { + const responseId = dispute.prophetData.responseId; + const response = this.registry.getResponse(responseId); - if (!response) { - this.logger.error( - `While trying to settle dispute ${dispute.id}, its response with ` + - `id ${dispute.prophetData.responseId} was not found in the registry.`, - ); + if (!response) { + this.logger.error( + `While trying to settle dispute ${dispute.id}, its response with ` + + `id ${dispute.prophetData.responseId} was not found in the registry.`, + ); - throw new DisputeWithoutResponse(dispute); - } + throw new DisputeWithoutResponse(dispute); + } - if (this.canBeSettled(request, dispute, blockNumber)) { - await this.settleDispute(request, response, dispute); - } - }); + if (this.canBeSettled(request, dispute, blockNumber)) { + await this.settleDispute(request, response, dispute); + } + }); - // Any of the disputes not being handled correctly should make the actor fail - await Promise.all(settledDisputes); - } catch (err) { - throw err; - } + // Any of the disputes not being handled correctly should make the actor fail + await Promise.all(settledDisputes); } private getActiveDisputes(): Dispute[] { @@ -589,9 +583,6 @@ export class EboActor { }); await ErrorHandler.handle(customError, { - consumeEvent: () => { - this.logger.info(`Consuming error: ${customError.name}`); - }, reenqueueEvent: () => { this.eventsQueue.push(event); }, @@ -755,9 +746,6 @@ export class EboActor { }); await ErrorHandler.handle(customError, { - consumeEvent: () => { - this.logger.info(`Consuming error: ${customError.name}`); - }, reenqueueEvent: () => { this.eventsQueue.push(event); }, @@ -879,9 +867,6 @@ export class EboActor { }); await ErrorHandler.handle(customError, { - consumeEvent: () => { - this.logger.info(`Consuming error: ${customError.name}`); - }, terminateActor: () => { throw customError; }, @@ -933,9 +918,6 @@ export class EboActor { }); await ErrorHandler.handle(customError, { - consumeEvent: () => { - this.logger.info(`Consuming error: ${customError.name}`); - }, terminateActor: () => { throw customError; }, diff --git a/packages/automated-dispute/src/types/errorTypes.ts b/packages/automated-dispute/src/types/errorTypes.ts index 773368c..30c4680 100644 --- a/packages/automated-dispute/src/types/errorTypes.ts +++ b/packages/automated-dispute/src/types/errorTypes.ts @@ -1,7 +1,7 @@ export type BaseErrorStrategy = { shouldNotify: boolean; shouldTerminate: boolean; - shouldConsume: boolean; + shouldReenqueue: boolean; customAction?: (context: any) => Promise | void; }; diff --git a/packages/automated-dispute/tests/services/eboActor.spec.ts b/packages/automated-dispute/tests/services/eboActor.spec.ts index 71750be..1a2618e 100644 --- a/packages/automated-dispute/tests/services/eboActor.spec.ts +++ b/packages/automated-dispute/tests/services/eboActor.spec.ts @@ -1,6 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { PastEventEnqueueError, RequestMismatch } from "../../src/exceptions/index.js"; +import { + CustomContractError, + PastEventEnqueueError, + RequestMismatch, +} from "../../src/exceptions/index.js"; import { EboEvent, Request, RequestId } from "../../src/types/index.js"; import mocks from "../mocks/index.js"; import { DEFAULT_MOCKED_REQUEST_CREATED_DATA } from "../services/eboActor/fixtures.js"; @@ -104,8 +108,9 @@ describe("EboActor", () => { // Expect processEvents to throw and handle the rejection await expect(actor.processEvents()).rejects.toThrow("Test Error"); - expect(mockEventsQueuePush).toHaveBeenCalledWith(event); + // The event should not be re-enqueued because it was the only event in the queue expect(mockEventsQueuePush).toHaveBeenCalledTimes(1); + expect(queue.size()).toBe(0); }); it("enqueues again an event at the top if its processing throws", async () => { @@ -115,10 +120,16 @@ describe("EboActor", () => { const firstEvent = { ...event }; const secondEvent = { ...firstEvent, blockNumber: firstEvent.blockNumber + 1n }; + const mockError = new CustomContractError("TestError", { + shouldReenqueue: true, + shouldTerminate: false, + shouldNotify: false, + }); + actor["onLastEvent"] = vi.fn().mockImplementation(() => { return new Promise((resolve, reject) => { setTimeout(() => { - reject(); + reject(mockError); }, 10); }); }); @@ -145,7 +156,7 @@ describe("EboActor", () => { expect(queue.peek()).toEqual(secondEvent); // processEvents throws and re-enqueues first event - await vi.advanceTimersByTime(10); + await vi.advanceTimersByTimeAsync(10); expect(queue.size()).toEqual(2); expect(queue.peek()).toEqual(firstEvent);