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

Refactor: actors derive subscribe helper #393

Open
wants to merge 1 commit into
base: refactor/actors-derive
Choose a base branch
from

Conversation

links234
Copy link
Contributor

This builds on top of #391.

Add subscribe_actor_event! helper macro to help with code readability (and allow for more boilerplate reduction in future PRs).

Copy link
Contributor

@undercover-cactus undercover-cactus left a comment

Choose a reason for hiding this comment

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

The macros looks nice. If possible I would avoid the clone.

@@ -6,7 +6,7 @@ homepage = { workspace = true }
license = { workspace = true }
authors = { workspace = true }
repository = { workspace = true }
edition = { workspace = true }
edition = "2021"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this at the root instead ?

task: UserSendsFileTask,
service: &self.blockchain,
spawner: &self.task_spawner,
context: self.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to avoid to clone here ? Can we hold a reference of the context ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. Maybe we can wrap up it up in an Arc but this is generating the same code as before (we are cloning there too). The context in our case is StorageHubHandler which already contains either Arcs or handlers which contain Arc, etc.

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.

2 participants