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

Conversation

juangirini
Copy link
Contributor

@juangirini juangirini commented Apr 3, 2023

Fix #12412

It adds an Option events to the ContractResult struct (ContractExecResult and ContractInstantiateResult) so that calls to bare_call and bare_instantiate can return the events collected. This is useful when dry running.

Collecting events though should only be done outside of the runtime block execution, so that the bare_call and bare_instantiate functions accept a new debug parameter of type DebugInfo which is an enum. When debug is set to DebugInfo::UnsafeDebug the events are collected, when set to DebugInfo::Skip events return None.

Related Cargo Contract issue

cumulus companion: paritytech/cumulus#2510

@juangirini juangirini linked an issue Apr 3, 2023 that may be closed by this pull request
@juangirini juangirini self-assigned this Apr 6, 2023
@juangirini juangirini added A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C3-medium PR touches the given topic and has a medium impact on builders. D2-breaksapi D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 6, 2023
@juangirini juangirini marked this pull request as ready for review April 6, 2023 08:31
@juangirini juangirini requested a review from athei as a code owner April 6, 2023 08:31
@juangirini juangirini added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 6, 2023
@juangirini
Copy link
Contributor Author

@athei This changes have been tested successfully against the contract-ui tool, but not against cargo-contract due to its issue reported yesterday

@juangirini juangirini requested a review from xermicus April 6, 2023 08:38
@juangirini juangirini added the T2-API This PR/Issue is related to APIs. label Apr 6, 2023
@juangirini
Copy link
Contributor Author

bot help

@command-bot
Copy link

command-bot bot commented May 2, 2023

Here's a link to docs

@juangirini juangirini requested a review from athei May 2, 2023 09:38
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

Shouldn't this PR have a cumulus companion?

frame/contracts/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/src/lib.rs Outdated Show resolved Hide resolved
@@ -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

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Nice. Ready to merge after those last nits are removed.

Comment on lines 55 to 57
use pallet_contracts_primitives::{
Code, CodeUploadResult, ContractExecResult, ContractInstantiateResult, GetStorageResult,
};
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@@ -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

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.

@athei
Copy link
Member

athei commented May 3, 2023

Shouldn't this PR have a cumulus companion?

Correct. CI is complaining.

@juangirini
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 44130d5 into master May 3, 2023
@paritytech-processbot paritytech-processbot bot deleted the jg/12412-contracts-return-emitted-events-for-dry-runs branch May 3, 2023 13:45
gpestana pushed a commit that referenced this pull request May 4, 2023
* contracts: add events to ContractResult

* contracts: add encoded events to ContractResult

* contracts: add generic Event to ContractResult

* contracts: test bare_call events

* contracts: update bare_call test name

* contracts: add better comments to dry run events

* contracts: fix pallet contracts primitives implementation

* contracts: add EventRecord generic to ContractInstantiateResult

* contracts: make event collection optional

* contracts: impreved notes on `collect_events`

* contracts: update benchmarking calls

* contracts: change bare_call and bare_instantiate to accept enums instead of bools

* contracts: add partial eq to new enums

* contracts: improve comments

* contracts: improve comments

* contracts: fix bare_call benchmarking

* contracts: fix bare call and instantiate in impl_runtime_apis

* contracts: add api versioning to new ContractsApi functions

* contracts: modify api versioning to new ContractsApi functions

* contracts: create new contracts api trait

* contracts: clean up code

* contracts: remove the contract results with events

* contracts: undo contracts api v3

* contracts: remove commented out code

* contracts: add new test bare call result does not return events

* contracts: add code review improvements

* contracts: remove type imports

* contracts: minor code review improvements
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* contracts: add events to ContractResult

* contracts: add encoded events to ContractResult

* contracts: add generic Event to ContractResult

* contracts: test bare_call events

* contracts: update bare_call test name

* contracts: add better comments to dry run events

* contracts: fix pallet contracts primitives implementation

* contracts: add EventRecord generic to ContractInstantiateResult

* contracts: make event collection optional

* contracts: impreved notes on `collect_events`

* contracts: update benchmarking calls

* contracts: change bare_call and bare_instantiate to accept enums instead of bools

* contracts: add partial eq to new enums

* contracts: improve comments

* contracts: improve comments

* contracts: fix bare_call benchmarking

* contracts: fix bare call and instantiate in impl_runtime_apis

* contracts: add api versioning to new ContractsApi functions

* contracts: modify api versioning to new ContractsApi functions

* contracts: create new contracts api trait

* contracts: clean up code

* contracts: remove the contract results with events

* contracts: undo contracts api v3

* contracts: remove commented out code

* contracts: add new test bare call result does not return events

* contracts: add code review improvements

* contracts: remove type imports

* contracts: minor code review improvements
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C3-medium PR touches the given topic and has a medium impact on builders. D2-breaksapi T2-API This PR/Issue is related to APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contracts: Return emitted events for dry-runs
5 participants