refactor(pre-compute-app): remove unnecessary Option fields #31
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the PreComputeApp struct to remove unnecessary Option fields and improve the API design by requiring essential parameters at construction time rather than during execution.
- Removes
Option<>wrappers fromchain_task_idandpre_compute_argsfields inPreComputeApp - Updates the constructor to accept required parameters directly instead of using a parameterless
new()method - Moves initialization logic from the
run()method to the application startup inapp_runner.rs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/compute/pre_compute_app.rs | Refactors struct fields from Optional to required, updates constructor signature, simplifies field access throughout implementation |
| src/compute/app_runner.rs | Moves initialization logic from PreComputeApp to the runner, updates function signatures to pass chain_task_id as parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…pp-remove-option-fields
src/compute/app_runner.rs
Outdated
| let pre_compute_args = match PreComputeArgs::read_args() { | ||
| Ok(pre_compute_args) => pre_compute_args, | ||
| Err(_) => { | ||
| return ExitMode::InitializationFailure; |
There was a problem hiding this comment.
This exit mode is specific to missing task ID. Using it here will provoke breaking changes in the API and the loss of information in the protocol.
On read_args error, we still need to call the send_exit_cause API to send to worker the error.
This can not be done as is and should bi discussed in another refactoring operations at a later time
src/compute/pre_compute_args.rs
Outdated
| @@ -5,7 +5,7 @@ use crate::compute::utils::env_utils::{TeeSessionEnvironmentVariable, get_env_va | |||
| /// | |||
| /// This structure aggregates configuration parameters from environment variables and task context, | |||
| /// providing a validated interface for subsequent computation phases. | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | |||
There was a problem hiding this comment.
Do not forget advice from the training
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | |
| #[derive(Clone, Debug, Default, Eq, PartialEq)] |
There was a problem hiding this comment.
Are all the derive required for production code or are some only used for tests ?
src/compute/app_runner.rs
Outdated
| /// let chain_task_id = "0x123456789abcdef"; | ||
| /// let pre_compute_args = PreComputeArgs::read_args()?; | ||
| /// | ||
| /// let pre_compute_app = PreComputeApp::new(pre_compute_args, chain_task_id); | ||
| /// let exit_code = start_with_app(&pre_compute_app, &chain_task_id) |
There was a problem hiding this comment.
This needs to be fixed to reflect current state of the code
No description provided.