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

zkasm_runner::ExecutionResult fileds redesign proposal. #242

Open
MCJOHN974 opened this issue Mar 4, 2024 · 1 comment
Open

zkasm_runner::ExecutionResult fileds redesign proposal. #242

MCJOHN974 opened this issue Mar 4, 2024 · 1 comment

Comments

@MCJOHN974
Copy link

Currently zkasm_runner::ExecutionResult looks the following way:

pub struct ExecutionResult {
    /// Path to the main zkAsm file that was executed.
    path: String,
    /// Status of the execution.
    pub status: ExecutionStatus,
    /// Error message in case the execution failed.
    pub error: Option<String>,
    /// Profiling information about this execution.
    /// Only populated for the successful executions.
    counters: Option<Counters>,
}

And I have a few questions about it:

  • is it true that if and only if status is ExecutionStatus::Success, than error contains None and counters contains Some?
  • is it true that if and only if status is ExecutionStatus::RuntimeError, than error contains Some and counters contains None?

If answer for both questions above is "yes" does we really need status field? It sounds like we already have info about status in error and counters fields. And, keeping one more such field will need to add extra attention to keep this fields consistent (be sure answer for both questions above is always "yes"). Maybe it will be better to remove this field and add method:

impl ExecutionResult {
    pub fn status(&self) -> ExecutionStatus {
        match self.error {
            Some(_) => ExecutionStatus::Success,
            None => ExecutionStatus::RuntimeError,
        }
    }
}
@MCJOHN974
Copy link
Author

Or maybe even better will be to squash all this fields into just two:

pub struct ExecutionResult {
    /// Path to the main zkAsm file that was executed.
    path: String,
    /// Contains profiling information in case of successful execution and error message
    /// if execution failed
    counters: Result<Counters, String>
}

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