Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

contracts: Change define_env! to expect a Result<T, DispatchError> for every function #7762

Merged
3 commits merged into from
Dec 29, 2020
Merged

contracts: Change define_env! to expect a Result<T, DispatchError> for every function #7762

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
ea5f8c4
Make host functions return TrapReason
athei Nov 16, 2020
2c65d81
review: Remove superflous .into()
athei Dec 29, 2020
064efa2
Merge branch 'master' into at-error-handling
athei Dec 29, 2020
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
8 changes: 8 additions & 0 deletions frame/contracts/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,14 @@ decl_error! {
/// on the call stack. Those actions are contract self destruction and restoration
/// of a tombstone.
ReentranceDenied,
/// `seal_input` was called twice from the same contract execution context.
InputAlreadyRead,
/// The subject passed to `seal_random` exceeds the limit.
RandomSubjectTooLong,
/// The amount of topics passed to `seal_deposit_events` exceeds the limit.
TooManyTopics,
/// The topics passed to `seal_deposit_events` contains at least one duplicate.
DuplicateTopics,
}
}

Expand Down
47 changes: 31 additions & 16 deletions frame/contracts/src/wasm/env_def/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ macro_rules! unmarshall_then_body {
#[inline(always)]
pub fn constrain_closure<R, F>(f: F) -> F
where
F: FnOnce() -> Result<R, sp_sandbox::HostError>,
F: FnOnce() -> Result<R, crate::wasm::runtime::TrapReason>,
{
f
}
Expand All @@ -109,14 +109,20 @@ macro_rules! unmarshall_then_body_then_marshall {
>(|| {
Copy link
Contributor

@gui1117 gui1117 Dec 28, 2020

Choose a reason for hiding this comment

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

If I'm correct this macro arm is for when it returns ReturnCode, (or at least there is no currently other usage of it).

Why not also automatically run err_into_return_code here ?

Copy link
Member Author

@athei athei Dec 29, 2020

Choose a reason for hiding this comment

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

Because err_into_return_code is called onto a DispatchError and returns a Result. But at that point in the macro it must have already decided whether we are Ok or Err.

Copy link
Contributor

@gui1117 gui1117 Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I just wonder why shouldn't we decide here ?
Like we have a Result<ReturnCode, TrapReason>, we can automatically change the Err(TrapReason::SupervisorError(CodeNotFound)) to Ok(ReturnCode::CodeNotFound) at this moment (and same for other variant).

and then seal_transfer can be simply written:

	seal_transfer(
		...
	) -> ReturnCode => {
		...

		ctx.ext.transfer(&callee, value)?;
		Ok(ReturnCode::Success)
	},

But ok it might add more confusion than help. But it could allow not to miss some err_into_return_code I feel.

Anyway fine

Copy link
Member Author

@athei athei Dec 29, 2020

Choose a reason for hiding this comment

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

Okay I see. Not sure how this play out with exec_into_return_code. Any ideas there?

Copy link
Contributor

@gui1117 gui1117 Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess exec_into_return_code would just need to convert ExecResult::Err:{origin: Callee} to Ok(ReturnCode::CalleeTrapped) (and other err as err and other ok to ok with Into).

But if it is confusing we can merge as it is.

Copy link
Member Author

@athei athei Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would rather merge it as is and have these further improvements in another PR to not completely shave the yak here. Maybe you can even put up a PR with your suggestions? I will gladly review it.

Copy link
Contributor

@gui1117 gui1117 Dec 29, 2020

Choose a reason for hiding this comment

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

Yes ok, I'll make a try

unmarshall_then_body!($body, $ctx, $args_iter, $( $names : $params ),*)
});
let r = body()?;
let r = body().map_err(|reason| {
$ctx.set_trap_reason(reason);
sp_sandbox::HostError
})?;
return Ok(sp_sandbox::ReturnValue::Value({ use $crate::wasm::env_def::ConvertibleToWasm; r.to_typed_value() }))
});
( $args_iter:ident, $ctx:ident, ( $( $names:ident : $params:ty ),* ) => $body:tt ) => ({
let body = $crate::wasm::env_def::macros::constrain_closure::<(), _>(|| {
unmarshall_then_body!($body, $ctx, $args_iter, $( $names : $params ),*)
});
body()?;
body().map_err(|reason| {
$ctx.set_trap_reason(reason);
sp_sandbox::HostError
})?;
return Ok(sp_sandbox::ReturnValue::Unit)
})
}
Expand Down Expand Up @@ -207,15 +213,24 @@ mod tests {
use parity_wasm::elements::ValueType;
use sp_runtime::traits::Zero;
use sp_sandbox::{ReturnValue, Value};
use crate::wasm::tests::MockExt;
use crate::wasm::Runtime;
use crate::exec::Ext;
use crate::gas::Gas;
use crate::{
wasm::{Runtime, runtime::TrapReason, tests::MockExt},
exec::Ext,
gas::Gas,
};

struct TestRuntime {
value: u32,
}

impl TestRuntime {
fn set_trap_reason(&mut self, _reason: TrapReason) {}
}

#[test]
fn macro_unmarshall_then_body_then_marshall_value_or_trap() {
fn test_value(
_ctx: &mut u32,
_ctx: &mut TestRuntime,
args: &[sp_sandbox::Value],
) -> Result<ReturnValue, sp_sandbox::HostError> {
let mut args = args.iter();
Expand All @@ -224,15 +239,15 @@ mod tests {
_ctx,
(a: u32, b: u32) -> u32 => {
if b == 0 {
Err(sp_sandbox::HostError)
Err(crate::wasm::runtime::TrapReason::Termination)
} else {
Ok(a / b)
}
}
)
}

let ctx = &mut 0;
let ctx = &mut TestRuntime { value: 0 };
assert_eq!(
test_value(ctx, &[Value::I32(15), Value::I32(3)]).unwrap(),
ReturnValue::Value(Value::I32(5)),
Expand All @@ -243,24 +258,24 @@ mod tests {
#[test]
fn macro_unmarshall_then_body_then_marshall_unit() {
fn test_unit(
ctx: &mut u32,
ctx: &mut TestRuntime,
args: &[sp_sandbox::Value],
) -> Result<ReturnValue, sp_sandbox::HostError> {
let mut args = args.iter();
unmarshall_then_body_then_marshall!(
args,
ctx,
(a: u32, b: u32) => {
*ctx = a + b;
ctx.value = a + b;
Ok(())
}
)
}

let ctx = &mut 0;
let ctx = &mut TestRuntime { value: 0 };
let result = test_unit(ctx, &[Value::I32(2), Value::I32(3)]).unwrap();
assert_eq!(result, ReturnValue::Unit);
assert_eq!(*ctx, 5);
assert_eq!(ctx.value, 5);
}

#[test]
Expand All @@ -270,7 +285,7 @@ mod tests {
if !amount.is_zero() {
Ok(())
} else {
Err(sp_sandbox::HostError)
Err(TrapReason::Termination)
}
});
let _f: fn(&mut Runtime<MockExt>, &[sp_sandbox::Value])
Expand Down Expand Up @@ -322,7 +337,7 @@ mod tests {
if !amount.is_zero() {
Ok(())
} else {
Err(sp_sandbox::HostError)
Err(crate::wasm::runtime::TrapReason::Termination)
}
},
);
Expand Down
4 changes: 2 additions & 2 deletions frame/contracts/src/wasm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,7 +1537,7 @@ mod tests {
&mut gas_meter
),
Err(ExecError {
error: Error::<Test>::ContractTrapped.into(),
error: Error::<Test>::TooManyTopics.into(),
origin: ErrorOrigin::Caller,
})
);
Expand Down Expand Up @@ -1582,7 +1582,7 @@ mod tests {
&mut gas_meter
),
Err(ExecError {
error: Error::<Test>::ContractTrapped.into(),
error: Error::<Test>::DuplicateTopics.into(),
origin: ErrorOrigin::Caller,
})
);
Expand Down
28 changes: 17 additions & 11 deletions frame/contracts/src/wasm/prepare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,18 +491,24 @@ mod tests {
}
}

// Define test environment for tests. We need ImportSatisfyCheck
// implementation from it. So actual implementations doesn't matter.
define_env!(TestEnv, <E: Ext>,
panic(_ctx) => { unreachable!(); },
/// Using unreachable statements triggers unreachable warnings in the generated code
#[allow(unreachable_code)]
mod env {
use super::*;

// gas is an implementation defined function and a contract can't import it.
gas(_ctx, _amount: u32) => { unreachable!(); },
// Define test environment for tests. We need ImportSatisfyCheck
// implementation from it. So actual implementations doesn't matter.
define_env!(Test, <E: Ext>,
panic(_ctx) => { unreachable!(); },

nop(_ctx, _unused: u64) => { unreachable!(); },
// gas is an implementation defined function and a contract can't import it.
gas(_ctx, _amount: u32) => { unreachable!(); },

seal_println(_ctx, _ptr: u32, _len: u32) => { unreachable!(); },
);
nop(_ctx, _unused: u64) => { unreachable!(); },

seal_println(_ctx, _ptr: u32, _len: u32) => { unreachable!(); },
);
}

macro_rules! prepare_test {
($name:ident, $wat:expr, $($expected:tt)*) => {
Expand All @@ -520,7 +526,7 @@ mod tests {
},
.. Default::default()
};
let r = prepare_contract::<TestEnv, crate::tests::Test>(wasm.as_ref(), &schedule);
let r = prepare_contract::<env::Test, crate::tests::Test>(wasm.as_ref(), &schedule);
assert_matches!(r, $($expected)*);
}
};
Expand Down Expand Up @@ -931,7 +937,7 @@ mod tests {
).unwrap();
let mut schedule = Schedule::default();
schedule.enable_println = true;
let r = prepare_contract::<TestEnv, crate::tests::Test>(wasm.as_ref(), &schedule);
let r = prepare_contract::<env::Test, crate::tests::Test>(wasm.as_ref(), &schedule);
assert_matches!(r, Ok(_));
}
}
Expand Down
Loading