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

contracts: add events to ContractResult #13807

Merged
paritytech-processbot merged 32 commits into from
May 3, 2023
Merged

contracts: add events to ContractResult #13807

Show file tree
Hide file tree
Changes from 23 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
117 changes: 104 additions & 13 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ use frame_system::{
};
pub use node_primitives::{AccountId, Signature};
use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Index, Moment};
use pallet_contracts_primitives::{
Code, CodeUploadResult, ContractExecResult, ContractExecResultWEvents,
ContractInstantiateResult, ContractInstantiateResultWEvents, GetStorageResult,
};
use pallet_election_provider_multi_phase::SolutionAccuracyOf;
use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
use pallet_nfts::PalletFeatures;
Expand Down Expand Up @@ -139,7 +143,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
// and set impl_version to 0. If only runtime
// implementation changes and behavior does not, then leave spec_version as
// is and increment impl_version.
spec_version: 268,
spec_version: 269,
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.

Was this necessary for CI? I don't think we need to bump in normal PRs.

Copy link
Contributor Author

@juangirini juangirini May 2, 2023

Choose a reason for hiding this comment

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

Good catch, that shouldn't go there

impl_version: 0,
apis: RUNTIME_API_VERSIONS,
transaction_version: 2,
Expand Down Expand Up @@ -1880,6 +1884,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 +2150,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,49 +2159,67 @@ impl_runtime_apis! {
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
input_data: Vec<u8>,
) -> pallet_contracts_primitives::ContractExecResult<Balance> {
) -> ContractExecResult<Balance> {
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_call(
let result = Contracts::bare_call(
origin,
dest,
value,
gas_limit,
storage_deposit_limit,
input_data,
true,
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::Determinism::Enforced,
)
pallet_contracts::CollectEvents::Skip,
);
ContractExecResult {
gas_consumed: result.gas_consumed,
gas_required: result.gas_required,
storage_deposit: result.storage_deposit,
debug_message: result.debug_message,
result: result.result,
events: None,
}
}

fn instantiate(
origin: AccountId,
value: Balance,
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
code: pallet_contracts_primitives::Code<Hash>,
code: Code<Hash>,
data: Vec<u8>,
salt: Vec<u8>,
) -> pallet_contracts_primitives::ContractInstantiateResult<AccountId, Balance>
) -> ContractInstantiateResult<AccountId, Balance>
{
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_instantiate(
let result = Contracts::bare_instantiate(
origin,
value,
gas_limit,
storage_deposit_limit,
code,
data,
salt,
true
)
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::CollectEvents::Skip,
);
ContractInstantiateResult {
gas_consumed: result.gas_consumed,
gas_required: result.gas_required,
storage_deposit: result.storage_deposit,
debug_message: result.debug_message,
result: result.result,
events: None,
}
}

fn upload_code(
origin: AccountId,
code: Vec<u8>,
storage_deposit_limit: Option<Balance>,
determinism: pallet_contracts::Determinism,
) -> pallet_contracts_primitives::CodeUploadResult<Hash, Balance>
) -> CodeUploadResult<Hash, Balance>
{
Contracts::bare_upload_code(
origin,
Expand All @@ -2205,14 +2232,78 @@ impl_runtime_apis! {
fn get_storage(
address: AccountId,
key: Vec<u8>,
) -> pallet_contracts_primitives::GetStorageResult {
) -> GetStorageResult {
Contracts::get_storage(
address,
key
)
}
}

impl pallet_contracts::ContractsApiV3<Block, AccountId, Balance, BlockNumber, Hash, EventRecord> for Runtime
{
fn call(
origin: AccountId,
dest: AccountId,
value: Balance,
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
input_data: Vec<u8>,
) -> ContractExecResultWEvents<Balance, EventRecord> {
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_call(
origin,
dest,
value,
gas_limit,
storage_deposit_limit,
input_data,
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::Determinism::Enforced,
pallet_contracts::CollectEvents::UnsafeCollect,
)
}

fn instantiate(
origin: AccountId,
value: Balance,
gas_limit: Option<Weight>,
storage_deposit_limit: Option<Balance>,
code: Code<Hash>,
data: Vec<u8>,
salt: Vec<u8>,
) -> ContractInstantiateResultWEvents<AccountId, Balance, EventRecord>
{
let gas_limit = gas_limit.unwrap_or(RuntimeBlockWeights::get().max_block);
Contracts::bare_instantiate(
origin,
value,
gas_limit,
storage_deposit_limit,
code,
data,
salt,
pallet_contracts::DebugInfo::UnsafeDebug,
pallet_contracts::CollectEvents::UnsafeCollect,
)
}

fn upload_code(
origin: AccountId,
code: Vec<u8>,
storage_deposit_limit: Option<Balance>,
determinism: pallet_contracts::Determinism,
) -> CodeUploadResult<Hash, Balance>
{
Contracts::bare_upload_code(
origin,
code,
storage_deposit_limit,
determinism,
)
}
}

impl pallet_transaction_payment_rpc_runtime_api::TransactionPaymentApi<
Block,
Balance,
Expand Down
21 changes: 16 additions & 5 deletions frame/contracts/primitives/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -71,15 +71,26 @@ 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 options as event collection is
/// optional.
pub events: Option<Vec<EventRecord>>,
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.

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

@juangirini juangirini May 2, 2023

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

@juangirini juangirini May 2, 2023

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

@athei athei May 3, 2023

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.
/// Result type of a `bare_call` call as well as `ContractsApiV3::call`.
pub type ContractExecResultWEvents<Balance, EventRecord> =
ContractResult<Result<ExecReturnValue, DispatchError>, Balance, EventRecord>;

/// Result type of the `ContractsApi::call` call.
pub type ContractExecResult<Balance> =
ContractResult<Result<ExecReturnValue, DispatchError>, Balance>;
ContractResult<Result<ExecReturnValue, DispatchError>, Balance, ()>;

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

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

/// 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,8 +929,9 @@ benchmarks! {
Weight::MAX,
None,
vec![],
true,
DebugInfo::UnsafeDebug,
Determinism::Enforced,
CollectEvents::Skip,
)
.result?;
}
Expand Down Expand Up @@ -978,8 +979,9 @@ benchmarks! {
Weight::MAX,
None,
vec![],
true,
DebugInfo::UnsafeDebug,
Determinism::Enforced,
CollectEvents::Skip,
)
.result?;
}
Expand Down Expand Up @@ -3170,8 +3172,9 @@ benchmarks! {
Weight::MAX,
None,
data,
false,
DebugInfo::Skip,
Determinism::Enforced,
CollectEvents::Skip,
)
.result?;
}
Expand Down Expand Up @@ -3219,8 +3222,9 @@ benchmarks! {
Weight::MAX,
None,
data,
false,
DebugInfo::Skip,
Determinism::Enforced,
CollectEvents::Skip,
)
.result?;
}
Expand Down
Loading