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
merged 32 commits into from
May 3, 2023
Merged

contracts: add events to ContractResult #13807

Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2168,8 +2168,8 @@ impl_runtime_apis! {
storage_deposit_limit,
input_data,
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::Determinism::Enforced,
pallet_contracts::CollectEvents::UnsafeCollect,
pallet_contracts::Determinism::Enforced,
)
}

Expand Down
2 changes: 1 addition & 1 deletion frame/contracts/primitives/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub struct ContractResult<R, Balance, EventRecord> {
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 options as event collection is
/// 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 •
edited
Loading

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.

}
Expand Down
8 changes: 4 additions & 4 deletions frame/contracts/src/benchmarking/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,8 +952,8 @@ benchmarks! {
None,
vec![],
DebugInfo::UnsafeDebug,
Determinism::Enforced,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
}
Expand Down Expand Up @@ -1002,8 +1002,8 @@ benchmarks! {
None,
vec![],
DebugInfo::UnsafeDebug,
Determinism::Enforced,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
}
Expand Down Expand Up @@ -3195,8 +3195,8 @@ benchmarks! {
None,
data,
DebugInfo::Skip,
Determinism::Enforced,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
}
Expand Down Expand Up @@ -3245,8 +3245,8 @@ benchmarks! {
None,
data,
DebugInfo::Skip,
Determinism::Enforced,
CollectEvents::Skip,
Determinism::Enforced,
)
.result?;
}
Expand Down
7 changes: 4 additions & 3 deletions frame/contracts/src/lib.rs
View file
Open in desktop
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::{EventRecord, 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 @@ -980,7 +980,7 @@ pub enum CollectEvents {
///
/// # Note
///
/// Events should only be collected when called off-chain, as this would
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// collect all the Events emitted in the block so far and put them in them into the PoV.
/// collect all the Events emitted in the block so far and put them into the PoV.

missed that earlier

///
/// **Never** use this mode for on-chain execution.
Expand Down Expand Up @@ -1231,14 +1231,15 @@ impl<T: Config> Pallet<T> {
storage_deposit_limit: Option<BalanceOf<T>>,
data: Vec<u8>,
debug: DebugInfo,
juangirini marked this conversation as resolved.
Show resolved Hide resolved
determinism: Determinism,
collect_events: CollectEvents,
determinism: Determinism,
) -> 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,
value,
Expand Down
Loading