near / wasmtime

Standalone JIT-style runtime for WebAssembly, using Cranelift
https://wasmtime.dev/
Apache License 2.0
3 stars 4 forks source link

`zkasm_runner::ExecutionResult` fileds redesign proposal. #242

Open MCJOHN974 opened 7 months ago

MCJOHN974 commented 7 months ago

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:

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 commented 7 months ago

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>
}