Enable mocking contracts

[pmikolajczyk41]

Description

This PR introduces two changes:

• the previous Tracing trait has been modified to accept contract address instead of code hash (seems to be way more convenient)
• a new trait CallInterceptor that allows intercepting contract calls; in particular the default implementation for () will just proceed in a standard way (after compilation optimizations, there will be no footprint of that); however, implementing type might decide to mock invocation and return ExecResult instead

Note: one might try merging before_call and intercept_call. However, IMHO this would be bad, since it would mix two completely different abstractions - tracing without any effects and actual intervention into execution process.

This will unblock working on mocking contracts utility in drink and similar tools (inkdevhub/drink#33)

Checklist

• My PR includes a detailed description as outlined in the "Description" section above
• My PR follows the labeling requirements of this project (at minimum one label for T required)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[pgherveou]

a new trait CallInterceptor that allows intercepting contract calls; in particular the default implementation for () will just proceed in a standard way (after compilation optimizations, there will be no footprint of that); however, implementing type might decide to mock invocation and return ExecResult instead

If you control the full environment with Drink!, wouldn't you be able to move this mocking logic outside of pallet-contract, for example by returning a mocked contract from storage when it's fetched by the runtime

[pmikolajczyk41]

If you control the full environment with Drink!, wouldn't you be able to move this mocking logic outside of pallet-contract, for example by returning a mocked contract from storage when it's fetched by the runtime

I guess that's doable, but honestly I don't have an idea now how to intervene into storage reading in test externalities. Even if I could, there are some other problems:

• pallet contracts used to do a few things regarding contract's wasm blob, including instrumentation and some other stuff, including contract ownership; therefore, handling all of these on drink! side will be hellish task
• keeping it in pallet-contracts let's you mock a subset of contract's methods, or even mock contract method for a particular set of parameters

[deuszx]

And, most importantly IMHO from the POV of a smart contract developer (and that's the audience of this feature) they don't need to write a "mocked contract" at all! In your proposal they'd have to and in that case we don't need this feature at all - just install the modified contract and use that instead.

[pgherveou]

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

[pmikolajczyk41]

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

[deuszx]

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.

[pgherveou]

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.

[deuszx]

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).

[pgherveou]

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

[deuszx]

Yes. Or slightly more cleanly IMHO

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

// business as usual

[pmikolajczyk41]

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

[agryaznov]

nice feature! just a little nit-picking