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

_:hammer_and_wrench: Refactor suggestion_ #137

Closed
ryardley opened this issue Oct 4, 2024 · 0 comments
Closed

_:hammer_and_wrench: Refactor suggestion_ #137

ryardley opened this issue Oct 4, 2024 · 0 comments

Comments

@ryardley
Copy link
Contributor

ryardley commented Oct 4, 2024

          _:hammer_and_wrench: Refactor suggestion_

Consider using a reference for EnclaveEvent in EventHook

The EventHook type and E3RequestRouter struct changes align well with the new event handling mechanism. However, the EventHook type now takes ownership of the EnclaveEvent, which might lead to unnecessary cloning.

Consider changing the EventHook type to use a reference to EnclaveEvent:

-pub type EventHook = Box<dyn FnMut(&mut E3RequestContext, EnclaveEvent)>;
+pub type EventHook = Box<dyn FnMut(&mut E3RequestContext, &EnclaveEvent)>;

This change would require updating the handle method in the Handler<EnclaveEvent> implementation for E3RequestRouter to pass a reference to msg instead of cloning it.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

/// Format of the hook that needs to be passed to E3RequestRouter
pub type EventHook = Box<dyn FnMut(&mut E3RequestContext, &EnclaveEvent)>;

/// E3RequestRouter will register hooks that receive an E3_id specific context. After hooks
/// have run e3_id specific messages are forwarded to all instances on the context. This enables
/// hooks to lazily register instances that have the correct dependencies available per e3_id
/// request
// TODO: setup typestate pattern so that we have to place hooks within correct order of
// dependencies
pub struct E3RequestRouter {
    contexts: HashMap<E3id, E3RequestContext>,
    hooks: Vec<EventHook>,
    buffer: EventBuffer,
}

Originally posted by @coderabbitai[bot] in #133 (comment)

@ryardley ryardley closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2024
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

No branches or pull requests

1 participant