-
Notifications
You must be signed in to change notification settings - Fork 2.6k
contracts: add events to ContractResult #13807
Changes from 30 commits
47200ff
a534758
28a9643
cf43686
08db645
7ca1b4b
5ad0868
dbd8735
1718400
782aba7
74d4b70
45e6eab
4426bbe
0de45b6
8d36e91
02c41d3
5d4fd5c
b9cb4ef
a8b0c8b
160cfb3
dacdc53
e7fa394
337e54e
b9838b1
fca001f
a8da1ff
af6587a
9919fd1
51894be
1e09bdb
eef0f97
3023dfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ use sp_weights::Weight; | |
/// | ||
/// It contains the execution result together with some auxiliary information. | ||
#[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. | ||
|
@@ -71,15 +71,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>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been tested against Contracts UI and it works just fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
SCALE is not really well defined by a spec or something. Whether you ignore trailing data or not is up to the implementation. See 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 I suggest we do away with the new version and two type aliases and just extend the struct. Should work the same way. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a |
||
} | ||
|
||
/// 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::call`. | ||
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> = | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
|
@@ -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. | ||||||
/// | ||||||
|
@@ -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 in them into the PoV. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
missed that earlier |
||||||
/// | ||||||
/// **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. | ||||||
|
@@ -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, | ||||||
|
@@ -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, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -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>, | ||||||
|
@@ -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, | ||||||
|
@@ -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 | ||||||
|
@@ -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, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -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. | ||||||
/// | ||||||
|
@@ -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. | ||||||
/// | ||||||
|
@@ -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. | ||||||
/// | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is no longer needed, right? It is a bit inconsistent now. Sometimes you use fully qualified types and sometimes you use those imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those types are used in the code, we do need either the imports or the fully qualified types.
There is indeed a mix of both in that file, not sure what is the guideline here. I am making them all fully qualified so at least it is consistent in my own code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is what I was getting at. Thanks.