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
Merged

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

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.

@bkchr
transactional: Wrap pallet::calls directly in storage layers
0fd5b85 
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
ggwpez
ggwpez approved these changes Jul 28, 2022
View reviewed changes
frame/support/procedural/src/pallet/expand/call.rs
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
Contributor

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?

shawntabrizi
shawntabrizi approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@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

@kianenigma kianenigma added this to In progress in Runtime via automation Aug 9, 2022
@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
ggwpez
ggwpez approved these changes Aug 9, 2022
View reviewed changes
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.

frame/state-trie-migration/src/lib.rs Outdated
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
Runtime automation moved this from In progress to Needs Audit Aug 9, 2022
@bkchr @ggwpez
Apply suggestions from code review
ad18fa6 
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
bkchr
bkchr commented Aug 9, 2022
View reviewed changes
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
cheme
cheme reviewed Aug 10, 2022
View reviewed changes
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
primitives/state-machine/src/overlayed_changes/changeset.rs Outdated Show resolved Hide resolved
bkchr and others added 2 commits August 10, 2022 21:51
@bkchr
Feedback
22e5fab
@bkchr @cheme
Apply suggestions from code review
e3bd7c1 
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
Runtime automation moved this from Needs Audit to Done Aug 10, 2022
@bkchr bkchr added B7-runtimenoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Aug 10, 2022
@ggwpez ggwpez mentioned this pull request Aug 12, 2022
Merged
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@bkchr @ggwpez
@cheme @DaviRain-Su
transactional: Wrap pallet::calls directly in storage layers (parit…
6681563 
…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>
@wischli wischli mentioned this pull request Sep 2, 2022
Merged
6 tasks
@librelois librelois mentioned this pull request Sep 15, 2022
Closed
2 tasks
@shawntabrizi shawntabrizi mentioned this pull request Sep 20, 2022
Closed
2 tasks
@notlesh notlesh mentioned this pull request Sep 29, 2022
Merged
13 tasks
@liuchengxu liuchengxu mentioned this pull request Oct 10, 2022
Merged
1 task
@echevrier echevrier mentioned this pull request Oct 26, 2022
Closed
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@bkchr @ggwpez
@cheme @ark0f
transactional: Wrap pallet::calls directly in storage layers (parit…
5f46ebd 
…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.
Reviewers

@cheme cheme cheme left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@ggwpez ggwpez ggwpez approved these changes

@KiChjang KiChjang Awaiting requested review from KiChjang

Assignees
No one assigned
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
No open projects
Runtime
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bkchr @KiChjang @cheme @shawntabrizi @ggwpez @jakoblell