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

contracts: add events to ContractResult #13807

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
47200ff
contracts: add events to ContractResult
juangirini Apr 3, 2023
a534758
contracts: add encoded events to ContractResult
juangirini Apr 3, 2023
28a9643
contracts: add generic Event to ContractResult
juangirini Apr 3, 2023
cf43686
Merge remote-tracking branch 'origin/master' into jg/12412-contracts-…
juangirini Apr 3, 2023
08db645
contracts: test bare_call events
juangirini Apr 3, 2023
7ca1b4b
contracts: update bare_call test name
juangirini Apr 4, 2023
5ad0868
contracts: add better comments to dry run events
juangirini Apr 4, 2023
dbd8735
contracts: fix pallet contracts primitives implementation
juangirini Apr 4, 2023
1718400
contracts: add EventRecord generic to ContractInstantiateResult
juangirini Apr 4, 2023
782aba7
contracts: make event collection optional
juangirini Apr 5, 2023
74d4b70
contracts: impreved notes on `collect_events`
juangirini Apr 5, 2023
45e6eab
contracts: update benchmarking calls
juangirini Apr 6, 2023
4426bbe
contracts: change bare_call and bare_instantiate to accept enums inst…
juangirini Apr 12, 2023
0de45b6
contracts: add partial eq to new enums
juangirini Apr 17, 2023
8d36e91
contracts: improve comments
juangirini Apr 17, 2023
02c41d3
contracts: improve comments
juangirini Apr 17, 2023
5d4fd5c
contracts: merge master and resolve conflicts
juangirini Apr 17, 2023
b9cb4ef
contracts: fix bare_call benchmarking
juangirini Apr 18, 2023
a8b0c8b
contracts: fix bare call and instantiate in impl_runtime_apis
juangirini Apr 18, 2023
160cfb3
contracts: add api versioning to new ContractsApi functions
juangirini Apr 20, 2023
dacdc53
contracts: modify api versioning to new ContractsApi functions
juangirini Apr 20, 2023
e7fa394
contracts: merge master and fix conflicts
juangirini Apr 28, 2023
337e54e
contracts: create new contracts api trait
juangirini Apr 28, 2023
b9838b1
contracts: clean up code
juangirini May 2, 2023
fca001f
contracts: remove the contract results with events
juangirini May 2, 2023
a8da1ff
contracts: undo contracts api v3
juangirini May 2, 2023
af6587a
contracts: remove commented out code
juangirini May 2, 2023
9919fd1
contracts: add new test bare call result does not return events
juangirini May 2, 2023
51894be
contracts: merge master and resolve conflicts
juangirini May 3, 2023
1e09bdb
contracts: add code review improvements
juangirini May 3, 2023
eef0f97
contracts: remove type imports
juangirini May 3, 2023
3023dfd
contracts: minor code review improvements
juangirini May 3, 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
17 changes: 12 additions & 5 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1880,6 +1880,11 @@ type Migrations = (
pallet_contracts::Migration<Runtime>,
);

type EventRecord = frame_system::EventRecord<
<Runtime as frame_system::Config>::RuntimeEvent,
<Runtime as frame_system::Config>::Hash,
>;

/// MMR helper types.
mod mmr {
use super::Runtime;
Expand Down Expand Up @@ -2141,7 +2146,7 @@ impl_runtime_apis! {
}
}

impl pallet_contracts::ContractsApi<Block, AccountId, Balance, BlockNumber, Hash> for Runtime
impl pallet_contracts::ContractsApi<Block, AccountId, Balance, BlockNumber, Hash, EventRecord> for Runtime
{
fn call(
origin: AccountId,
Expand All @@ -2150,7 +2155,7 @@ impl_runtime_apis! {
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
input_data: Vec<u8>,
) -> pallet_contracts_primitives::ContractExecResult<Balance> {
) -> pallet_contracts_primitives::ContractExecResult<Balance, EventRecord> {
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_call(
origin,
Expand All @@ -2159,7 +2164,8 @@ impl_runtime_apis! {
gas_limit,
storage_deposit_limit,
input_data,
true,
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::CollectEvents::UnsafeCollect,
pallet_contracts::Determinism::Enforced,
)
}
Expand All @@ -2172,7 +2178,7 @@ impl_runtime_apis! {
code: pallet_contracts_primitives::Code<Hash>,
data: Vec<u8>,
salt: Vec<u8>,
) -> pallet_contracts_primitives::ContractInstantiateResult<AccountId, Balance>
) -> pallet_contracts_primitives::ContractInstantiateResult<AccountId, Balance, EventRecord>
{
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_instantiate(
Expand All @@ -2183,7 +2189,8 @@ impl_runtime_apis! {
code,
data,
salt,
true
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::CollectEvents::UnsafeCollect,
)
}

Expand Down
26 changes: 18 additions & 8 deletions frame/contracts/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,18 @@ use sp_runtime::{
use sp_std::prelude::*;
use sp_weights::Weight;

/// Result type of a `bare_call` or `bare_instantiate` call.
/// Result type of a `bare_call` or `bare_instantiate` call as well as `ContractsApi::call` and
/// `ContractsApi::instantiate`.
///
/// It contains the execution result together with some auxiliary information.
///
/// #Note
///
/// It has been extended to include `events` at the end of the struct while not bumping the
/// `ContractsApi` version. Therefore when SCALE decoding a `ContractResult` its trailing data
/// should be ignored to avoid any potential compatibility issues.
#[derive(Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)]
pub struct ContractResult<R, Balance> {
pub struct ContractResult<R, Balance, EventRecord> {
/// How much weight was consumed during execution.
pub gas_consumed: Weight,
/// How much weight is required as gas limit in order to execute this call.
Expand Down Expand Up @@ -71,15 +78,18 @@ pub struct ContractResult<R, Balance> {
pub debug_message: Vec<u8>,
/// The execution result of the wasm code.
pub result: R,
/// The events that were emitted during execution. It is an option as event collection is
/// optional.
pub events: Option<Vec<EventRecord>>,
Copy link
Member

Choose a reason for hiding this comment

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

Even if EventRecords = () this will still add some bytes to the end of the struct. This is not 100% backwards compatible as clients might reject trailing data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been tested against Contracts UI and it works just fine.
The 'old' call and instantiate functions return events: None that encodes to 0x00, but as far as I understand SCALE ignores trailing data. Anyway, if we want to be on the safe side we can always create two versions of ContractResult, one with and another one without events.
WDYT?

Copy link
Member

@athei athei May 2, 2023

Choose a reason for hiding this comment

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

but as far as I understand SCALE ignores trailing data

SCALE is not really well defined by a spec or something. Whether you ignore trailing data or not is up to the implementation. See Decode vs DecodeAll.

That said, I suspect the default behavior is to ignore trailing data. Hence we can safely extend the struct in new versions. But if you think about it this means we don't even need a new version in this case because we can just always emit the events since they will be ignored by older code as trailing data. If they ignore a single 0x00 they will also ignore everything else.

I suggest we do away with the new version and two type aliases and just extend the struct. Should work the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the new version and implemented it all in the original ContractsApi. Tested it against Contracts UI and works fine

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a #Note to the type to warn users that we take the liberty to extend the type with new data without bumping the API version? It should warn them to always ignore trailing data.

}

/// Result type of a `bare_call` call.
pub type ContractExecResult<Balance> =
ContractResult<Result<ExecReturnValue, DispatchError>, Balance>;
/// Result type of a `bare_call` call as well as `ContractsApi::call`.
pub type ContractExecResult<Balance, EventRecord> =
ContractResult<Result<ExecReturnValue, DispatchError>, Balance, EventRecord>;

/// Result type of a `bare_instantiate` call.
pub type ContractInstantiateResult<AccountId, Balance> =
ContractResult<Result<InstantiateReturnValue<AccountId>, DispatchError>, Balance>;
/// Result type of a `bare_instantiate` call as well as `ContractsApi::instantiate`.
pub type ContractInstantiateResult<AccountId, Balance, EventRecord> =
ContractResult<Result<InstantiateReturnValue<AccountId>, DispatchError>, Balance, EventRecord>;

/// Result type of a `bare_code_upload` call.
pub type CodeUploadResult<CodeHash, Balance> =
Expand Down
12 changes: 8 additions & 4 deletions frame/contracts/src/benchmarking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,8 @@ benchmarks! {
Weight::MAX,
None,
vec![],
true,
DebugInfo::UnsafeDebug,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
Expand Down Expand Up @@ -1000,7 +1001,8 @@ benchmarks! {
Weight::MAX,
None,
vec![],
true,
DebugInfo::UnsafeDebug,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
Expand Down Expand Up @@ -3192,7 +3194,8 @@ benchmarks! {
Weight::MAX,
None,
data,
false,
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
Expand Down Expand Up @@ -3241,7 +3244,8 @@ benchmarks! {
Weight::MAX,
None,
data,
false,
DebugInfo::Skip,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
Expand Down
93 changes: 75 additions & 18 deletions frame/contracts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ use frame_support::{
weights::Weight,
BoundedVec, RuntimeDebugNoBound, WeakBoundedVec,
};
use frame_system::{ensure_signed, pallet_prelude::OriginFor, Pallet as System};
use frame_system::{ensure_signed, pallet_prelude::OriginFor, EventRecord, Pallet as System};
use pallet_contracts_primitives::{
Code, CodeUploadResult, CodeUploadReturnValue, ContractAccessError, ContractExecResult,
ContractInstantiateResult, ExecReturnValue, GetStorageResult, InstantiateReturnValue,
Expand Down Expand Up @@ -148,6 +148,8 @@ type CodeVec<T> = BoundedVec<u8, <T as Config>::MaxCodeLen>;
type RelaxedCodeVec<T> = WeakBoundedVec<u8, <T as Config>::MaxCodeLen>;
type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
type DebugBufferVec<T> = BoundedVec<u8, <T as Config>::MaxDebugBufferLen>;
type EventRecordOf<T> =
EventRecord<<T as frame_system::Config>::RuntimeEvent, <T as frame_system::Config>::Hash>;

/// The old weight type.
///
Expand Down Expand Up @@ -971,6 +973,35 @@ struct InstantiateInput<T: Config> {
salt: Vec<u8>,
}

/// Determines whether events should be collected during execution.
#[derive(PartialEq)]
pub enum CollectEvents {
juangirini marked this conversation as resolved.
Show resolved Hide resolved
/// Collect events.
///
/// # Note
///
/// Events should only be collected when called off-chain, as this would otherwise
/// collect all the Events emitted in the block so far and put them into the PoV.
///
/// **Never** use this mode for on-chain execution.
UnsafeCollect,
/// Skip event collection.
Skip,
}

/// Determines whether debug messages will be collected.
#[derive(PartialEq)]
pub enum DebugInfo {
juangirini marked this conversation as resolved.
Show resolved Hide resolved
/// Collect debug messages.
/// # Note
juangirini marked this conversation as resolved.
Show resolved Hide resolved
///
/// This should only ever be set to `UnsafeDebug` when executing as an RPC because
/// it adds allocations and could be abused to drive the runtime into an OOM panic.
UnsafeDebug,
/// Skip collection of debug messages.
Skip,
juangirini marked this conversation as resolved.
Show resolved Hide resolved
}

/// Return type of private helper functions.
struct InternalOutput<T: Config, O> {
/// The gas meter that was used to execute the call.
Expand Down Expand Up @@ -1187,22 +1218,27 @@ impl<T: Config> Pallet<T> {
///
/// # Note
///
/// `debug` should only ever be set to `true` when executing as an RPC because
/// it adds allocations and could be abused to drive the runtime into an OOM panic.
/// If set to `true` it returns additional human readable debugging information.
/// If `debug` is set to `DebugInfo::UnsafeDebug` it returns additional human readable debugging
/// information.
///
/// It returns the execution result and the amount of used weight.
/// If `collect_events` is set to `CollectEvents::UnsafeCollect` it collects all the Events
/// emitted in the block so far and the ones emitted during the execution of this contract.
pub fn bare_call(
origin: T::AccountId,
dest: T::AccountId,
value: BalanceOf<T>,
gas_limit: Weight,
storage_deposit_limit: Option<BalanceOf<T>>,
data: Vec<u8>,
debug: bool,
debug: DebugInfo,
juangirini marked this conversation as resolved.
Show resolved Hide resolved
collect_events: CollectEvents,
determinism: Determinism,
) -> ContractExecResult<BalanceOf<T>> {
let mut debug_message = if debug { Some(DebugBufferVec::<T>::default()) } else { None };
) -> ContractExecResult<BalanceOf<T>, EventRecordOf<T>> {
let mut debug_message = if matches!(debug, DebugInfo::UnsafeDebug) {
Some(DebugBufferVec::<T>::default())
} else {
None
};
let origin = Origin::from_account_id(origin);
let common = CommonInput {
origin,
Expand All @@ -1213,12 +1249,19 @@ impl<T: Config> Pallet<T> {
debug_message: debug_message.as_mut(),
};
let output = CallInput::<T> { dest, determinism }.run_guarded(common);
let events = if matches!(collect_events, CollectEvents::UnsafeCollect) {
Some(System::<T>::read_events_no_consensus().map(|e| *e).collect())
} else {
None
};

ContractExecResult {
result: output.result.map_err(|r| r.error),
gas_consumed: output.gas_meter.gas_consumed(),
gas_required: output.gas_meter.gas_required(),
storage_deposit: output.storage_deposit,
debug_message: debug_message.unwrap_or_default().to_vec(),
events,
}
}

Expand All @@ -1231,9 +1274,11 @@ impl<T: Config> Pallet<T> {
///
/// # Note
///
/// `debug` should only ever be set to `true` when executing as an RPC because
/// it adds allocations and could be abused to drive the runtime into an OOM panic.
/// If set to `true` it returns additional human readable debugging information.
/// If `debug` is set to `DebugInfo::UnsafeDebug` it returns additional human readable debugging
/// information.
///
/// If `collect_events` is set to `CollectEvents::UnsafeCollect` it collects all the Events
/// emitted in the block so far.
pub fn bare_instantiate(
origin: T::AccountId,
value: BalanceOf<T>,
Expand All @@ -1242,9 +1287,14 @@ impl<T: Config> Pallet<T> {
code: Code<CodeHash<T>>,
data: Vec<u8>,
salt: Vec<u8>,
debug: bool,
) -> ContractInstantiateResult<T::AccountId, BalanceOf<T>> {
let mut debug_message = if debug { Some(DebugBufferVec::<T>::default()) } else { None };
debug: DebugInfo,
collect_events: CollectEvents,
) -> ContractInstantiateResult<T::AccountId, BalanceOf<T>, EventRecordOf<T>> {
let mut debug_message = if debug == DebugInfo::UnsafeDebug {
Some(DebugBufferVec::<T>::default())
} else {
None
};
let common = CommonInput {
origin: Origin::from_account_id(origin),
value,
Expand All @@ -1254,6 +1304,12 @@ impl<T: Config> Pallet<T> {
debug_message: debug_message.as_mut(),
};
let output = InstantiateInput::<T> { code, salt }.run_guarded(common);
// collect events if CollectEvents is UnsafeCollect
let events = if collect_events == CollectEvents::UnsafeCollect {
Some(System::<T>::read_events_no_consensus().map(|e| *e).collect())
juangirini marked this conversation as resolved.
Show resolved Hide resolved
} else {
None
};
ContractInstantiateResult {
result: output
.result
Expand All @@ -1263,6 +1319,7 @@ impl<T: Config> Pallet<T> {
gas_required: output.gas_meter.gas_required(),
storage_deposit: output.storage_deposit,
debug_message: debug_message.unwrap_or_default().to_vec(),
events,
}
}

Expand Down Expand Up @@ -1370,11 +1427,12 @@ impl<T: Config> Pallet<T> {
sp_api::decl_runtime_apis! {
/// The API used to dry-run contract interactions.
#[api_version(2)]
pub trait ContractsApi<AccountId, Balance, BlockNumber, Hash> where
pub trait ContractsApi<AccountId, Balance, BlockNumber, Hash, EventRecord> where
juangirini marked this conversation as resolved.
Show resolved Hide resolved
athei marked this conversation as resolved.
Show resolved Hide resolved
AccountId: Codec,
Balance: Codec,
BlockNumber: Codec,
Hash: Codec,
EventRecord: Codec,
{
/// Perform a call from a specified account to a given contract.
///
Expand All @@ -1386,7 +1444,7 @@ sp_api::decl_runtime_apis! {
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
input_data: Vec<u8>,
) -> ContractExecResult<Balance>;
) -> ContractExecResult<Balance, EventRecord>;

/// Instantiate a new contract.
///
Expand All @@ -1399,8 +1457,7 @@ sp_api::decl_runtime_apis! {
code: Code<Hash>,
data: Vec<u8>,
salt: Vec<u8>,
) -> ContractInstantiateResult<AccountId, Balance>;

) -> ContractInstantiateResult<AccountId, Balance, EventRecord>;

/// Upload new code without instantiating a contract from it.
///
Expand Down
Loading