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.

Sign up for GitHub

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

DO NOT MERGE: Prototype some potential error handling ways #192

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
+207 −170
Draft

DO NOT MERGE: Prototype some potential error handling ways #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a92754
prototype error handling helpers
dignifiedquire Apr 6, 2022
09c204e
prototype abort error
dignifiedquire Apr 6, 2022
bf4c523
Update actors/runtime/src/actor_error.rs
dignifiedquire Apr 6, 2022
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
52 changes: 19 additions & 33 deletions Cargo.lock
View file
Open in desktop

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

16 changes: 8 additions & 8 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ members = [
## Uncomment entries below when working locally on ref-fvm and this repo simultaneously.
## Assumes the ref-fvm checkout is in a sibling directory with the same name.
## (Valid once FVM modules are published to crates.io)
#[patch.crates-io]
#fvm_shared = { path = "../ref-fvm/shared" }
#fvm_sdk = { path = "../ref-fvm/sdk" }
#fvm_ipld_hamt = { path = "../ref-fvm/ipld/hamt" }
#fvm_ipld_amt = { path = "../ref-fvm/ipld/amt" }
#fvm_ipld_bitfield = { path = "../ref-fvm/ipld/bitfield"}
#fvm_ipld_encoding = { path = "../ref-fvm/ipld/encoding"}
#fvm_ipld_blockstore = { path = "../ref-fvm/ipld/blockstore"}
[patch.crates-io]
fvm_shared = { path = "../ref-fvm/shared" }
fvm_sdk = { path = "../ref-fvm/sdk" }
fvm_ipld_hamt = { path = "../ref-fvm/ipld/hamt" }
fvm_ipld_amt = { path = "../ref-fvm/ipld/amt" }
fvm_ipld_bitfield = { path = "../ref-fvm/ipld/bitfield"}
fvm_ipld_encoding = { path = "../ref-fvm/ipld/encoding"}
fvm_ipld_blockstore = { path = "../ref-fvm/ipld/blockstore"}

[profile.wasm]
inherits = "release"
Expand Down
26 changes: 12 additions & 14 deletions actors/account/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ use num_derive::FromPrimitive;
use num_traits::FromPrimitive;

use fil_actors_runtime::builtin::singletons::SYSTEM_ACTOR_ADDR;
use fil_actors_runtime::cbor;
use fil_actors_runtime::runtime::{ActorCode, Runtime};
use fil_actors_runtime::{actor_error, ActorError};
use fil_actors_runtime::{actor_error, ensure_args};
use fil_actors_runtime::{cbor, Abort};

pub use self::state::State;

Expand All @@ -34,25 +34,23 @@ pub enum Method {
pub struct Actor;
impl Actor {
/// Constructor for Account actor
pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), ActorError>
pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), Abort>
Copy link
Member

Choose a reason for hiding this comment

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

Given that Abort can't happen, why not just return ()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wanted to potential for actually returning an error in debug scenarios. My idea was that we could have sth like a debug feature, where Abort would collect data and actually be a returned value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One alternative design I had was

fn foo() -> Abort<T> {}

type Abort<T> = Result<T, AbortError>;

to make it even more visual that this is a special kind of exit.

Copy link
Member

Choose a reason for hiding this comment

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

Because I wanted to potential for actually returning an error in debug scenarios. My idea was that we could have sth like a debug feature, where Abort would collect data and actually be a returned value.

Ok, that's reasonable. Although, IMO, we get better information if we abort immediately (and ask wasmtime for a backtrace). See https://docs.wasmtime.dev/api/wasmtime/struct.Config.html#method.wasm_backtrace_details.

Copy link
Member

Choose a reason for hiding this comment

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

There's some scope for divergent behaviour between the two cases here. I guess that this Abort is intended to always be immediately propagated with ?.

Up at this high level of the actor entry points, I would probably lean towards Steb's initial take that actually aborting is better. We would want a way to shim that call for testing, though, wrapping out the underlying static call so we can use a mock/fake VM instead.

where
BS: Blockstore,
RT: Runtime<BS>,
{
rt.validate_immediate_caller_is(std::iter::once(&*SYSTEM_ACTOR_ADDR))?;
match address.protocol() {
Protocol::Secp256k1 | Protocol::BLS => {}
protocol => {
return Err(actor_error!(ErrIllegalArgument;
"address must use BLS or SECP protocol, got {}", protocol));
}
}
rt.validate_immediate_caller_is([&*SYSTEM_ACTOR_ADDR])?;
ensure_args!(
matches!(address.protocol(), Protocol::Secp256k1 | Protocol::BLS),
"address must use BLS or SECP protocol, got {}",
address.protocol(),
);
rt.create(&State { address })?;
Ok(())
}

// Fetches the pubkey-type address from this actor.
pub fn pubkey_address<BS, RT>(rt: &mut RT) -> Result<Address, ActorError>
pub fn pubkey_address<BS, RT>(rt: &mut RT) -> Result<Address, Abort>
where
BS: Blockstore,
RT: Runtime<BS>,
Expand All @@ -68,7 +66,7 @@ impl ActorCode for Actor {
rt: &mut RT,
method: MethodNum,
params: &RawBytes,
) -> Result<RawBytes, ActorError>
) -> Result<RawBytes, Abort>
where
BS: Blockstore,
RT: Runtime<BS>,
Expand All @@ -82,7 +80,7 @@ impl ActorCode for Actor {
let addr = Self::pubkey_address(rt)?;
Ok(RawBytes::serialize(addr)?)
}
None => Err(actor_error!(SysErrInvalidMethod; "Invalid method")),
None => Err(actor_error!(SysErrInvalidMethod; "Invalid method").into()),
Copy link
Member

Choose a reason for hiding this comment

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

I get this is just prototyping, but I want only one of the ActorError or Abort types to survive. They're going after the same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They definitely should survive both in this construction. The conversion into an Abort is what triggers the abort, where as ActorError still can be used inside an actor to communicate recoverable errors.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think ActorError should be used for recoverable errors. I don't think there are use cases for that. I would say recoverable errors should not specify an abort code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where would that mapping to codes live then?

}
}
}
Loading