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

Enable mocking contracts #1331

Merged
merged 20 commits into from
Sep 29, 2023
Merged

Enable mocking contracts #1331

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7c84bc1
Setup trait and invoking
pmikolajczyk41 Aug 31, 2023
5123152
Fill arguments
pmikolajczyk41 Aug 31, 2023
66b2a84
Unused import
pmikolajczyk41 Aug 31, 2023
2526857
Add testcase
pmikolajczyk41 Aug 31, 2023
b994e94
Merge branch 'master' into mock-contracts
pmikolajczyk41 Aug 31, 2023
6563469
fmt
pmikolajczyk41 Aug 31, 2023
c59df2e
Merge branch 'master' into mock-contracts
pmikolajczyk41 Aug 31, 2023
d66d7cc
Merge remote-tracking branch 'origin/master' into mock-contracts
pmikolajczyk41 Sep 5, 2023
ffdfe5d
Remove CallInterceptorResult
pmikolajczyk41 Sep 5, 2023
b9399af
Adapt test
pmikolajczyk41 Sep 5, 2023
b09a677
Review
pmikolajczyk41 Sep 5, 2023
e36da54
Expected
pmikolajczyk41 Sep 5, 2023
e82d6b3
Merge branch 'master' into mock-contracts
pmikolajczyk41 Sep 6, 2023
5ec45b4
fmt
pmikolajczyk41 Sep 6, 2023
e8a5204
Merge branch 'master' into mock-contracts
pmikolajczyk41 Sep 25, 2023
3690ee0
rename testcase
pmikolajczyk41 Sep 28, 2023
5ed54ea
exportedfunction by ref
pmikolajczyk41 Sep 28, 2023
ba0817c
Merge branch 'master' into mock-contracts
pmikolajczyk41 Sep 29, 2023
a6a9be7
Dummy commit to retrigger CI
pmikolajczyk41 Sep 29, 2023
d479b9e
Merge branch 'master' into mock-contracts
pmikolajczyk41 Sep 29, 2023
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
53 changes: 44 additions & 9 deletions substrate/frame/contracts/src/debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@
// See the License for the specific language governing permissions and
// limitations under the License.

pub use crate::exec::ExportedFunction;
use crate::{CodeHash, Config, LOG_TARGET};
use pallet_contracts_primitives::ExecReturnValue;
pub use crate::exec::{ExecResult, ExportedFunction};
use crate::{Config, LOG_TARGET};
pub use pallet_contracts_primitives::ExecReturnValue;

/// Umbrella trait for all interfaces that serves for debugging.
pub trait Debugger<T: Config>: Tracing<T> {}
pub trait Debugger<T: Config>: Tracing<T> + CallInterceptor<T> {}

impl<T: Config, V> Debugger<T> for V where V: Tracing<T> {}
impl<T: Config, V> Debugger<T> for V where V: Tracing<T> + CallInterceptor<T> {}

/// Defines methods to capture contract calls, enabling external observers to
/// measure, trace, and react to contract interactions.
Expand All @@ -37,11 +37,11 @@ pub trait Tracing<T: Config> {
///
/// # Arguments
///
/// * `code_hash` - The code hash of the contract being called.
/// * `contract_address` - The address of the contract that is about to be executed.
/// * `entry_point` - Describes whether the call is the constructor or a regular call.
/// * `input_data` - The raw input data of the call.
fn new_call_span(
code_hash: &CodeHash<T>,
contract_address: &T::AccountId,
entry_point: ExportedFunction,
input_data: &[u8],
) -> Self::CallSpan;
Expand All @@ -60,8 +60,12 @@ pub trait CallSpan {
impl<T: Config> Tracing<T> for () {
type CallSpan = ();

fn new_call_span(code_hash: &CodeHash<T>, entry_point: ExportedFunction, input_data: &[u8]) {
log::trace!(target: LOG_TARGET, "call {entry_point:?} hash: {code_hash:?}, input_data: {input_data:?}")
fn new_call_span(
contract_address: &T::AccountId,
entry_point: ExportedFunction,
input_data: &[u8],
) {
log::trace!(target: LOG_TARGET, "call {entry_point:?} account: {contract_address:?}, input_data: {input_data:?}")
}
}

Expand All @@ -70,3 +74,34 @@ impl CallSpan for () {
log::trace!(target: LOG_TARGET, "call result {output:?}")
}
}

pub enum CallInterceptorResult {
Proceed,
Stop(ExecResult),
}

pub trait CallInterceptor<T: Config> {
pmikolajczyk41 marked this conversation as resolved.
Show resolved Hide resolved
/// Allows to intercept contract calls and decide whether they should be executed or not.
/// If the call is intercepted, the mocked result of the call is returned.
///
/// # Arguments
///
/// * `contract_address` - The address of the contract that is about to be executed.
/// * `entry_point` - Describes whether the call is the constructor or a regular call.
/// * `input_data` - The raw input data of the call.
fn intercept_call(
contract_address: &T::AccountId,
entry_point: ExportedFunction,
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
input_data: &[u8],
) -> CallInterceptorResult;
}

impl<T: Config> CallInterceptor<T> for () {
fn intercept_call(
_contract_address: &T::AccountId,
_entry_point: ExportedFunction,
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
_input_data: &[u8],
) -> CallInterceptorResult {
CallInterceptorResult::Proceed
}
}
22 changes: 14 additions & 8 deletions substrate/frame/contracts/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// limitations under the License.

use crate::{
debug::{CallSpan, Tracing},
debug::{CallInterceptor, CallInterceptorResult, CallSpan, Tracing},
gas::GasMeter,
storage::{self, meter::Diff, WriteOutcome},
BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf,
Expand Down Expand Up @@ -907,13 +907,19 @@ where
// Every non delegate call or instantiate also optionally transfers the balance.
self.initial_transfer()?;

let call_span =
T::Debug::new_call_span(executable.code_hash(), entry_point, &input_data);

// Call into the Wasm blob.
let output = executable
.execute(self, &entry_point, input_data)
.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?;
let contract_address = &top_frame!(self).account_id;

let call_span = T::Debug::new_call_span(contract_address, entry_point, &input_data);
let interception_result =
T::Debug::intercept_call(contract_address, entry_point, &input_data);
let output = match interception_result {
// Call into the Wasm blob.
CallInterceptorResult::Proceed => executable
.execute(self, &entry_point, input_data)
.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?,
// Return the output of the interceptor.
CallInterceptorResult::Stop(output) => output?,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move the details of intercept_call to the trait implementation, so we don't leak the struct that could then be internal to the implementation.

We could move that behind a feature-flag like this

let output = if cfg!(feature = "unsafe-debug") {
    T::Debug::wrap_execute(self, contract_address, &entry_point, input_data)
} else {
    executable.execute(self, &entry_point, input_data)
}.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?

or maybe simply

let output = T::Debug::wrap_execute(self, contract_address, &entry_point, input_data)
  .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move that behind a feature-flag like this

we have removed the feature flag, haven't we?

we don't leak the struct

what if we return just Result<(), ExecResult> instead of the current new enum type? same semantics, no new types

Copy link

Choose a reason for hiding this comment

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

what if we return just Result<(), ExecResult> instead of the current new enum type? same semantics, no new types

Or Option<ExecResult> which is less confusing to the implementor of the trait but would require a bit more work here to handle it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have removed the feature flag, haven't we?

We can always re-introduce it. For tracing we needed to have basic traces regardless of the environment, so we could get away without introducing a feature flag. Intercepting calls seems more like a dev only feature, so I think it could make sense to have that gated behind a feature flag.

Moving it all behind a T::Debug::wrap_execute that would just call the actual code with the default () Debug trait might be good alternative as well. @athei any thought on this?

what if we return just Result<(), ExecResult> instead of the current new enum type? same semantics, no new types

I think that's what I am suggesting with the code snippet above.

Copy link

Choose a reason for hiding this comment

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

I think that's what I am suggesting with the code snippet above.

That's not how we saw it. You suggested (assuming it's the 2nd snippet):

let output = T::Debug::wrap_execute(self, contract_address, &entry_point, input_data)
  .map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?

which looked like the wrap_execute should also call execute?

I think we can leave the T::Debug::intercept_call as-is but change the return type.

if we agree to the general idea I think we can pin down the details (method name and return type).

Copy link
Contributor

@pgherveou pgherveou Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

which looked like the wrap_execute should also call execute?

Yes right

I think we can leave the T::Debug::intercept_call as-is but change the return type.

So you would do something like

if let Some(result) = T::Debug::intercept_call(contract_address, entry_point, &input_data) {
  result
} else {
// current logic
}

Yes I guess we can start with this

Copy link

@deuszx deuszx Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Or slightly more cleanly IMHO

if let Some(output) = T::Debug::intercept_call(contract_address, entry_point, &input_data) {
  output?
}

// business as usual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so all in all I've removed the new CallInterceptorResult enum in favor of Option... does it meet the consensus above?


call_span.after_call(&output);

Expand Down
116 changes: 93 additions & 23 deletions substrate/frame/contracts/src/tests/test_debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,31 @@
// limitations under the License.

use super::*;
use crate::debug::{CallSpan, ExportedFunction, Tracing};
use crate::{
debug::{CallInterceptor, CallInterceptorResult, CallSpan, ExportedFunction, Tracing},
AccountIdOf,
};
use frame_support::traits::Currency;
use pallet_contracts_primitives::ExecReturnValue;
use pretty_assertions::assert_eq;
use std::cell::RefCell;

#[derive(Clone, PartialEq, Eq, Debug)]
struct DebugFrame {
code_hash: CodeHash<Test>,
contract_account: AccountId32,
call: ExportedFunction,
input: Vec<u8>,
result: Option<Vec<u8>>,
}

thread_local! {
static DEBUG_EXECUTION_TRACE: RefCell<Vec<DebugFrame>> = RefCell::new(Vec::new());
static INTERCEPTED_ADDRESS: RefCell<Option<AccountId32>> = RefCell::new(None);
}

pub struct TestDebug;
pub struct TestCallSpan {
code_hash: CodeHash<Test>,
contract_account: AccountId32,
call: ExportedFunction,
input: Vec<u8>,
}
Expand All @@ -45,27 +49,50 @@ impl Tracing<Test> for TestDebug {
type CallSpan = TestCallSpan;

fn new_call_span(
code_hash: &CodeHash<Test>,
contract_account: &AccountIdOf<Test>,
entry_point: ExportedFunction,
input_data: &[u8],
) -> TestCallSpan {
DEBUG_EXECUTION_TRACE.with(|d| {
d.borrow_mut().push(DebugFrame {
code_hash: *code_hash,
contract_account: contract_account.clone(),
call: entry_point,
input: input_data.to_vec(),
result: None,
})
});
TestCallSpan { code_hash: *code_hash, call: entry_point, input: input_data.to_vec() }
TestCallSpan {
contract_account: contract_account.clone(),
call: entry_point,
input: input_data.to_vec(),
}
}
}

impl CallInterceptor<Test> for TestDebug {
fn intercept_call(
contract_address: &<Test as frame_system::Config>::AccountId,
_entry_point: ExportedFunction,
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
_input_data: &[u8],
) -> CallInterceptorResult {
INTERCEPTED_ADDRESS.with(|i| {
if i.borrow().as_ref() == Some(contract_address) {
CallInterceptorResult::Stop(Ok(ExecReturnValue {
flags: ReturnFlags::REVERT,
data: vec![],
}))
} else {
CallInterceptorResult::Proceed
}
})
}
}

impl CallSpan for TestCallSpan {
fn after_call(self, output: &ExecReturnValue) {
DEBUG_EXECUTION_TRACE.with(|d| {
d.borrow_mut().push(DebugFrame {
code_hash: self.code_hash,
contract_account: self.contract_account,
call: self.call,
input: self.input,
result: Some(output.data.clone()),
Expand All @@ -76,8 +103,8 @@ impl CallSpan for TestCallSpan {

#[test]
fn unsafe_debugging_works() {
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
let (wasm_caller, code_hash_caller) = compile_module::<Test>("call").unwrap();
let (wasm_callee, code_hash_callee) = compile_module::<Test>("store_call").unwrap();
let (wasm_caller, _) = compile_module::<Test>("call").unwrap();
let (wasm_callee, _) = compile_module::<Test>("store_call").unwrap();

fn current_stack() -> Vec<DebugFrame> {
DEBUG_EXECUTION_TRACE.with(|stack| stack.borrow().clone())
Expand All @@ -100,18 +127,18 @@ fn unsafe_debugging_works() {
.account_id
}

fn constructor_frame(hash: CodeHash<Test>, after: bool) -> DebugFrame {
fn constructor_frame(contract_account: AccountId32, after: bool) -> DebugFrame {
DebugFrame {
code_hash: hash,
contract_account,
call: ExportedFunction::Constructor,
input: vec![],
result: if after { Some(vec![]) } else { None },
}
}

fn call_frame(hash: CodeHash<Test>, args: Vec<u8>, after: bool) -> DebugFrame {
fn call_frame(contract_account: AccountId32, args: Vec<u8>, after: bool) -> DebugFrame {
DebugFrame {
code_hash: hash,
contract_account,
call: ExportedFunction::Call,
input: args,
result: if after { Some(vec![]) } else { None },
Expand All @@ -129,19 +156,19 @@ fn unsafe_debugging_works() {
assert_eq!(
current_stack(),
vec![
constructor_frame(code_hash_caller, false),
constructor_frame(code_hash_caller, true),
constructor_frame(code_hash_callee, false),
constructor_frame(code_hash_callee, true),
constructor_frame(addr_caller.clone(), false),
constructor_frame(addr_caller.clone(), true),
constructor_frame(addr_callee.clone(), false),
constructor_frame(addr_callee.clone(), true),
pmikolajczyk41 marked this conversation as resolved.
Show resolved Hide resolved
]
);

let main_args = (100u32, &addr_callee).encode();
let main_args = (100u32, &addr_callee.clone()).encode();
let inner_args = (100u32).encode();

assert_ok!(Contracts::call(
RuntimeOrigin::signed(ALICE),
addr_caller,
addr_caller.clone(),
0,
GAS_LIMIT,
None,
Expand All @@ -152,11 +179,54 @@ fn unsafe_debugging_works() {
assert_eq!(
stack_top,
vec![
call_frame(code_hash_caller, main_args.clone(), false),
call_frame(code_hash_callee, inner_args.clone(), false),
call_frame(code_hash_callee, inner_args, true),
call_frame(code_hash_caller, main_args, true),
call_frame(addr_caller.clone(), main_args.clone(), false),
call_frame(addr_callee.clone(), inner_args.clone(), false),
call_frame(addr_callee, inner_args, true),
call_frame(addr_caller, main_args, true),
]
);
});
}

#[test]
fn call_interception_works() {
let (wasm, _) = compile_module::<Test>("dummy").unwrap();

ExtBuilder::default().existential_deposit(200).build().execute_with(|| {
let _ = Balances::deposit_creating(&ALICE, 1_000_000);

let account_id = Contracts::bare_instantiate(
ALICE,
0,
GAS_LIMIT,
None,
Code::Upload(wasm),
vec![],
// some salt to ensure that the address of this contract is unique among all tests
vec![0x41, 0x41, 0x41, 0x41],
DebugInfo::Skip,
CollectEvents::Skip,
)
.result
.unwrap()
.account_id;

// no interception yet
assert_ok!(Contracts::call(
RuntimeOrigin::signed(ALICE),
account_id.clone(),
0,
GAS_LIMIT,
None,
vec![],
));

// intercept calls to this contract
INTERCEPTED_ADDRESS.with(|i| *i.borrow_mut() = Some(account_id.clone()));

assert_err_ignore_postinfo!(
Contracts::call(RuntimeOrigin::signed(ALICE), account_id, 0, GAS_LIMIT, None, vec![],),
<Error<Test>>::ContractReverted,
);
});
}