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

Finish implementing initial Simulation interface #26

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions eth2/simulation/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ authors = ["Greg Trowbridge <[email protected]>"]
edition = "2018"

[dependencies]
ewasm = "0.2.2"
snafu = "0.6.0"
ssz_types = { path = "../utils/ssz_types" }
types = { path = "../types" }
typenum = "1.11.2"

[dev-dependencies]
hex = "0.4.0"
27 changes: 0 additions & 27 deletions eth2/simulation/src/errors.rs

This file was deleted.

35 changes: 34 additions & 1 deletion eth2/simulation/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,36 @@
mod errors;
mod simulation;
mod store;

use snafu::Snafu;
use std::fmt;

/// Shorthand for result types returned from the Simulation simulation.
pub type Result<V, E = Error> = std::result::Result<V, E>;

#[derive(Debug)]
pub enum WhatBound {
ExecutionEnvironment,
ExecutionEnvironmentState,
ShardBlock(usize),
Shard,
}

impl fmt::Display for WhatBound {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
WhatBound::ExecutionEnvironment => write!(f, "execution environment"),
WhatBound::ExecutionEnvironmentState => write!(f, "execution environment state"),
WhatBound::Shard => write!(f, "shard"),
WhatBound::ShardBlock(shard) => write!(f, "block on shard {}", shard),
}
}
}

/// Errors arising from the simulation.
#[derive(Debug, Snafu)]
pub enum Error {
#[snafu(display("{} exceeds max allowable length", what))]
MaxLengthExceeded { what: String },
Copy link
Contributor

Choose a reason for hiding this comment

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

Is MaxLengthExceeded always triggered from a ssz_types::Error?

Either way, I'd add a source field here, to give more context on the error.

Copy link
Contributor Author

@gjtrowbridge gjtrowbridge Feb 12, 2020

Choose a reason for hiding this comment

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

Correct, currently MaxLengthExceeded is only triggered from a ssz_types::Error. However, I could envision it being useful for other types of errors in the future.

Though, unless I'm misuderstanding Snafu, I believe that 1) I can only use context()?; if I add a source field (unless I'm calling .context() on a function returning an Option), and 2) this error type will ONLY ever be able to be used for ssz_types::Error if I do that approach, which could be OK but if so I'd want to change the name from MaxLengthExceeded to something like SszTypeError or similar.

One last note: my original implementation did use Snafu's canonical .context(...)?; pattern, along with a source field. However, ssz_types::Error doesn't actually implement std::error::Error, which means that ssz_types::Error can't be used as a source error. Obviously, I can update the ssz_types::Error type to implement std:error::Error, but I opted not to do this immediately, since I anticipate that ssz_types will eventually be a crate instead of "owned code" in this repo. That said, prior to pushing this as a crate, I'm sure the ssz_types authors would make the Error type compliant with the std::error::Error trait.

TL;DR: I don't feel too strongly here, as I see it, there are 2 paths forward:

  1. Leave it as-is, and update in the future if we want the granularity / lower-level visibility into the ssz_types error.
  2. Change to have this use a source field now, which would entail:
  • Updating the ssz_types::Error field to implement the std:error:Error trait
  • Likely changing the name of this wrapper error from MaxLengthExceeded to something like SszTypeError, and have the display implementation of the wrapper just show the underlying display implementation of the ssz error (which I would have just added in the first bullet point). This way the error could wrap any of the ssz error types that may come back.

Either way works, and #2 is fairly easy to do. #2 has the advantage of the lower-level visibility, and would make it easier to give more info like what the max length was when it was exceeded. #1 is obviously already in use, and has the advantage of in my opinion being simpler and more readable.

My preference is #1 for now unless/until we have a compelling need for #2, but if you or others feel strongly #2 is easy enough to implement.

#[snafu(display("no {} exists at index: {}", what, index))]
OutOfBounds { what: WhatBound, index: usize },
}
Loading