-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add external transient_storage to pallet_revive::ExecConfig #10493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 11 commits
3cc8d6d
fdba022
093782d
d9f4b93
8becba6
12d6c8b
d31790d
fc09d1d
d37a69b
561cdef
cc82a2b
de4d819
840ad71
68b627c
d70b9d0
75f36cf
a97d852
f5a7e42
e27ed4d
e00b2c1
3d87b31
8ca6e61
037c675
8e35cc9
28d5d53
fc0394d
eaae888
2cdbab6
6b2dea7
8f0cbcc
469b6ba
5f097b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| title: add external transient_storage to pallet_revive::ExecConfig | ||
| doc: | ||
| - audience: Node Dev | ||
| description: |- | ||
| # Description | ||
|
|
||
| This PR adds the ability to supply external copy of `TransientStorage` to `pallet_revive::ExecConfig` to be used during execution. | ||
| This is required by testing in foundry as we only enter `pallet_revive` during a `CALL` or `CREATE` instruction and we need to carryover | ||
| the transient storage to other following calls as they happen within an external tx | ||
| crates: | ||
| - name: pallet-revive | ||
| bump: major |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,11 +18,12 @@ | |||||
| //! A crate that hosts a common definitions that are relevant for the pallet-revive. | ||||||
|
|
||||||
| use crate::{ | ||||||
| evm::DryRunConfig, mock::MockHandler, storage::WriteOutcome, BalanceOf, Config, Time, H160, | ||||||
| U256, | ||||||
| evm::DryRunConfig, mock::MockHandler, storage::WriteOutcome, | ||||||
| transient_storage::TransientStorage, BalanceOf, Config, Time, H160, U256, | ||||||
| }; | ||||||
| use alloc::{boxed::Box, fmt::Debug, string::String, vec::Vec}; | ||||||
| use alloc::{boxed::Box, fmt::Debug, rc::Rc, string::String, vec::Vec}; | ||||||
| use codec::{Decode, Encode, MaxEncodedLen}; | ||||||
| use core::cell::RefCell; | ||||||
| use frame_support::{traits::tokens::Balance, weights::Weight}; | ||||||
| use pallet_revive_uapi::ReturnFlags; | ||||||
| use scale_info::TypeInfo; | ||||||
|
|
@@ -391,6 +392,10 @@ pub struct ExecConfig<T: Config> { | |||||
| /// This is primarily used for testing purposes and should be `None` in production | ||||||
| /// environments. | ||||||
| pub mock_handler: Option<Box<dyn MockHandler<T>>>, | ||||||
| /// External transient storage useful for testing. | ||||||
| /// | ||||||
| /// Should be `None` in production environments. | ||||||
| pub transient_storage: Option<Rc<RefCell<TransientStorage<T>>>>, | ||||||
|
||||||
| pub transient_storage: Option<Rc<RefCell<TransientStorage<T>>>>, | |
| pub initial_transient_storage: Option<TransientStorage<T>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still need to have it back in foundry, so moving into into the stack will not allow us to get the instance back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh okay got it. But is the Rc needed? The calling code (foundry) owns TransientStorage and can just borrow when it wants to access it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to introduce lifetimes and possible refactor parts of the interface to avoid an RC. Rc is needed because ownership of ExecConfig goes directly to pallet-revive from the caller so it's a one way trip unless we return parts of ExecConfig at the end of the call if requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rcis needed because ownership ofExecConfiggoes directly to pallet-revive
Ahh okay. I thought it takes a reference. It should not take exec_config by value. This makes no sense as it is just forwarded as reference to Stack. So this should be refactored to get rid of the Rc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new life times. Just changing the signature of the bare_* calls to take ExecConfig by reference instead of value. This will not require any new explicit life time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot pass TransientStorage by reference inside ExecConfig without a lifetime, no? so this would cause us to add lifetime param to every callsite of ExecConfig and to its definition.
Anyways, i'll try and see whether the lifetimes would line up inside exec with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot pass TransientStorage by reference inside ExecConfig without a lifetime, no?
I am not asking you to do this. Pass ExecConfig as reference into bare_*. The TransientStorage is passed as RefCell<TransientStorage>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
athei marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.