Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add condition to terminate actors #36

Merged
merged 4 commits into from
Sep 9, 2024
Merged

Conversation

0xyaco
Copy link
Collaborator

@0xyaco 0xyaco commented Sep 6, 2024

🤖 Linear

Closes GRT-154

Description

  • Add a Request.status property to indicate the internal state of a Request
  • Add a method to update the request within the EboRegistry
  • Implement the conditions to terminate an actor (ie no active entities)

Copy link

linear bot commented Sep 6, 2024

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you're not building the command in the EboActor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice catch! Ended up adding a test to be sure that we're using & running the right command

@0xyaco 0xyaco requested a review from 0xnigir1 September 9, 2024 15:11
0xnigir1
0xnigir1 previously approved these changes Sep 9, 2024
Copy link
Collaborator

@0xnigir1 0xnigir1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, lgtm 🚀

jahabeebs
jahabeebs previously approved these changes Sep 9, 2024
Copy link
Collaborator

@jahabeebs jahabeebs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@0xkenj1 0xkenj1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just 2 brief comments, after addressing lgtm

return FinalizeRequest.buildFromEvent(
event as EboEvent<"RequestFinalized">,
this.registry,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this entire method to an isolated CommandsFactory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TODO comment already, in the docs of this method, for doing that hehe

@@ -3,12 +3,14 @@ import { NormalizedAddress } from "@ebo-agent/shared";
import { Address } from "viem";

export type RequestId = NormalizedAddress;
export type RequestStatus = "active" | "finalized";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type RequestStatus = "active" | "finalized";
export type RequestStatus = "Active" | "Finalized";

should we do it this way to have consistent naming convention for statuses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable!

@0xyaco 0xyaco dismissed stale reviews from jahabeebs and 0xnigir1 via dec8b75 September 9, 2024 18:56
@0xyaco 0xyaco merged commit 6c359c5 into dev Sep 9, 2024
5 checks passed
@0xyaco 0xyaco deleted the feat/can-be-terminated branch September 9, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants