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

transactional: Wrap pallet::calls directly in storage layers #11927

Merged
merged 14 commits into from
Aug 10, 2022

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jul 27, 2022

Before this pr we only wrapped pallet::calls into storage layers when executing the calls with
dispatch. This pr is solving that by wrapping each call function inside a storage layer.

Before this pr we only wrapped `pallet::calls` into storage layers when executing the calls with
`dispatch`. This pr is solving that by wrapping each call function inside a storage layer.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 27, 2022
@bkchr bkchr requested a review from shawntabrizi July 27, 2022 22:56
@bkchr bkchr added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 27, 2022
item_impl.items.iter_mut().for_each(|i| {
if let syn::ImplItem::Method(method) = i {
let block = &method.block;
method.block = syn::parse_quote! {{
Copy link
Member

Choose a reason for hiding this comment

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

This pr is solving that by wrapping each call function inside a storage layer

You change the logic of an extrinsic to be wrapped inside a with_storage_layer here?
So we now have the invariant that the logic inside an extrinsic is always transactional, no matter how its called?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the idea, if you are thinking of making it configurable, there is this PR that attempts to solve it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should make it configurable eventually

@bkchr bkchr added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 28, 2022
@KiChjang
Copy link
Contributor

I'm an idiot when code touches the primitives, maybe someone else can review?

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

I am not sure about the state-machine basic changes, but otherwise awesome.

ty basti

@jakoblell jakoblell removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Aug 9, 2022
@jakoblell jakoblell added the D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited label Aug 9, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Maybe @cheme can look at the trie changes but to me it looks good.

Self::halt(&error);
Err(DispatchErrorWithPostInfo { post_info, error })
Self::halt(error);
Ok(().into())
Copy link
Member

Choose a reason for hiding this comment

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

Why is there no post dispatch weight correction anymore?
Again in line 748

primitives/state-machine/src/basic.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/basic.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/basic.rs Show resolved Hide resolved
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
frame/state-trie-migration/src/lib.rs Outdated Show resolved Hide resolved
frame/state-trie-migration/src/lib.rs Outdated Show resolved Hide resolved
frame/state-trie-migration/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

Big thanks for switching basic.rs to use the overlaychanges.
Logic looks good to me, added a few question to be sure I did not miss something.

frame/state-trie-migration/src/lib.rs Show resolved Hide resolved
frame/state-trie-migration/src/lib.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/basic.rs Show resolved Hide resolved
primitives/state-machine/src/basic.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/basic.rs Outdated Show resolved Hide resolved
bkchr and others added 2 commits August 10, 2022 21:51
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
@bkchr bkchr merged commit a1a9b47 into master Aug 10, 2022
@bkchr bkchr deleted the bkchr-wrap-calls-in-storage-layer branch August 10, 2022 20:27
@bkchr bkchr added B7-runtimenoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Aug 10, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
…ytech#11927)

* transactional: Wrap `pallet::calls` directly in storage layers

Before this pr we only wrapped `pallet::calls` into storage layers when executing the calls with
`dispatch`. This pr is solving that by wrapping each call function inside a storage layer.

* Teach `BasicExternalities` transactions support

* Fix crates

* FMT

* Fix benchmarking tests

* Use correct span

* Support old decl macros

* Fix test

* Apply suggestions from code review

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/state-trie-migration/src/lib.rs

* Update frame/state-trie-migration/src/lib.rs

* Update frame/state-trie-migration/src/lib.rs

* Feedback

* Apply suggestions from code review

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ytech#11927)

* transactional: Wrap `pallet::calls` directly in storage layers

Before this pr we only wrapped `pallet::calls` into storage layers when executing the calls with
`dispatch`. This pr is solving that by wrapping each call function inside a storage layer.

* Teach `BasicExternalities` transactions support

* Fix crates

* FMT

* Fix benchmarking tests

* Use correct span

* Support old decl macros

* Fix test

* Apply suggestions from code review

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/state-trie-migration/src/lib.rs

* Update frame/state-trie-migration/src/lib.rs

* Update frame/state-trie-migration/src/lib.rs

* Feedback

* Apply suggestions from code review

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants