feat: implement EboActorsManager #25
Conversation
GRT-27 EBOActorsManager
The AC:
Resources |
| return this.requestActorMap.get(requestId); | ||
| } | ||
|
|
||
| public deleteActor(requestId: string): boolean { |
There was a problem hiding this comment.
why is returning a boolean ? Should it be void ?
There was a problem hiding this comment.
same doubt, i guess on error false is returned instead of throwing
There was a problem hiding this comment.
We can do void.
I'm just following the Map.prototype.delete() return type for the sake of consistency.
I guess it doesn't hurt to keep the boolean return type, but now that you mention, I should add some docs on these methods, it would've helped here 😬
There was a problem hiding this comment.
yep, would be very helpful for the user of this class, should we add a warn or sth if the requestId doesn't exist?
There was a problem hiding this comment.
Sounds good. The EboProcessor will most likely warn if the delete returns false when removing actors from the registry; can't think of any situation out of the top of my head where that'd happen, but better to be safe than sorry.
| import { ProtocolProvider } from "../../src/protocolProvider.js"; | ||
| import { EboEvent } from "../../src/types/events.js"; | ||
| import { Response } from "../../src/types/prophet.js"; | ||
| import mocks from "../mocks/index.js"; |
There was a problem hiding this comment.
ensure you are importing from index files
There was a problem hiding this comment.
I need to find a way to have my VSCode stop importing from non-index files lol
| const request = DEFAULT_MOCKED_REQUEST_CREATED_DATA; | ||
| const { actor } = mocks.buildEboActor(request, logger); | ||
| const actorsManager = new EboActorsManager(); | ||
| const mockSetRequestActorMap = vi.spyOn(actorsManager["requestActorMap"], "set"); | ||
|
|
||
| actorsManager.registerActor(request.id, actor); | ||
|
|
||
| expect(mockSetRequestActorMap).toHaveBeenCalledWith(request.id, actor); |
There was a problem hiding this comment.
i would check if map was effectively appended
| import { EboActor } from "./eboActor.js"; | ||
| import { RequestAlreadyHandled } from "./exceptions/index.js"; | ||
|
|
||
| export class EboActorsManager { |
There was a problem hiding this comment.
the manager implementation is always in-memory or is like the Registry that may in the future be a Redis or whatever makes sense? in that case, we should have the proper interface
There was a problem hiding this comment.
It's something interesting to think about; in this case I think The Graph should need to scale a lot for this to need something outside the in-memory solution used here. Worst case scenario, at the same there will be 1 item (ie 1 <request, actor> tuple) for every indexed chain, which are not that many.
| return this.requestActorMap.get(requestId); | ||
| } | ||
|
|
||
| public deleteActor(requestId: string): boolean { |
There was a problem hiding this comment.
same doubt, i guess on error false is returned instead of throwing
|
@0xkenj1 @0xnigir1 did a small refactor on |
| return this.requestActorMap.get(requestId); | ||
| } | ||
|
|
||
| public deleteActor(requestId: string): boolean { |
There was a problem hiding this comment.
yep, would be very helpful for the user of this class, should we add a warn or sth if the requestId doesn't exist?
🤖 Linear
Closes GRT-27
Description
Implements the
EboActorsManagerclass.