-
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?
Conversation
|
/cmd prdoc |
|
haven't revivewed yet, but can you merge and use torsten/gas-fixes as the base branch, as it's a huge refactoring that should land today or tomorrow |
4a8682a to
1908c2d
Compare
1908c2d to
9869425
Compare
|
/cmd fmt |
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
Differential Tests Results (REVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
25ebc2e to
d9f4b93
Compare
|
/cmd fmt |
athei
left a comment
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.
Please also remove all the unrelated semicolon changes.
| 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>>>>, |
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.
Move into the Stack using take() when the Stack is created.
| 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
Co-authored-by: Alexander Theißen <[email protected]>
| 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>>>>, |
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.
The fmt seems to ignore those lines. Yes, if we can change it to get is consistent that would be nice. But it should be a separate fmt only PR. |
|
/cmd fmt |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd fmt |
|
I think the |
updated |
Description
This PR adds the ability to supply external copy of
TransientStoragetopallet_revive::ExecConfigto be used during execution. This is required by testing in foundry as we only enterpallet_reviveduring aCALLorCREATEinstruction and we need to carryover the transient storage to other following calls as they happen within an external txIntegration
Already being used on a branch of foundry-polkadot
Review Notes
I'm not so fond ofRc<RefCell<TransientStorage>>approach and using a function to decide to which transient storage to dispatch to during execution, so any suggestions would be welcome.changed to plain
RefCell<TransientStorage>as per suggestion from @athei