Rename & update SandboxConfig trait #105
Conversation
There was a problem hiding this comment.
I'm not that big fan of the thread_local solution, but I don't want to block your work on this - let's merge it now and see if this causes any problems
also, two kind requests:
- please bump version
- in another PR, can you please add an example showcasing how you can use drink with custom runtime? even if it requires some manual work, I think it'd be good to have it covered here
|
@deuszx @pmikolajczyk41 updated the initial fix. To avoid using the hacky
fn execute_with<T>(&mut self, execute: impl FnOnce() -> T) -> T;
fn dry_run<T>(&mut self, action: impl FnOnce(&mut Self) -> T) -> T;
This needs another pass to make sure some doc string are still up to date, but hopefully this address your concern. |
|
Thanks @pgherveou , I'll review it this week. Also, before I forget, remember to bump minor version of the crate. |
Gentle ping for review 🙏 |
| self.call_internal(Some(address), message, args, endowment) | ||
| } | ||
|
|
||
| fn call_internal<S: AsRef<str> + Debug, T: Decode>( | ||
| fn call_internal<S: AsRef<str> + Debug, V: Decode>( |
|
@deuszx you are still blocking this, would be nice if you could remove the "change requested" or delegate your review to someone else? |
| @@ -69,7 +69,7 @@ impl Default for UiState { | |||
| } | |||
|
|
|||
| pub struct AppState { | |||
| pub session: Session<MinimalRuntime>, | |||
| pub session: Session<MinimalSandbox>, | |||
There was a problem hiding this comment.
I think it's kinda weird that Session is now parameterised with the Sandbox - it encloses the sandbox but the parameter type should be something that was the common denominator for both - session and sandbox.
Cannot point my finger to anything specific, so this won't be a blocker, but I just have a gut feeling that previous typing was more "appropriate".
There was a problem hiding this comment.
it encloses the sandbox but the parameter type should be something that was the common denominator for both
The sandbox itself enclose the runtime so the common denominator still stands
drink/src/lib.rs
Outdated
| pub type BalanceOf<Runtime> = | ||
| <<Runtime as pallet_contracts::Config>::Currency as Inspect<AccountIdFor<Runtime>>>::Balance; | ||
| pub type BalanceOf<R> = <R as pallet_balances::Config>::Balance; |
There was a problem hiding this comment.
What w twist (https://github.com/inkdevhub/drink/pull/106/files#r1509471655). @pmikolajczyk41 , are you OK with this change? Since you made the previous one and had some arguments it was "more correct".
There was a problem hiding this comment.
ah this can go away, I probably missed that in the merge
There was a problem hiding this comment.
As requested @pgherveou , I'm unblocking the PR (apologies for the holding off, I forgot that request changes block the merge) but I can't approve it with clear conscience - I don't have enough time to review it thoroughly at the moment but I know it's important for you so I'm deferring to @pmikolajczyk41 here.
deuszx
dismissed
their stale review
As stated in the comment (didn't know it won't automatically dismiss it).
pgherveou
force-pushed
the
pg/update-macros
branch
from
01678a5 to
52ef3b9
Compare
Follow up from #99
I realized that the current design was not working as I wanted.
Specifically, when working with parachain and
xcm_simulator::TestExt, I need to use the existing externalities, andthe custom
execute_withto dispatch messages properly.The Sandbox is now created inside the macro (renamed
create_minimal_sandbox) along with the runtime. The default setup now uses theMinimalSandboxcreated through the macro, instead of (the now deleted)Sandboxstruct.The trait
SandboxConfigis renamedSandboxand exposes these two new APIsfn initialize_storageis removed since this can be done when the Sandbox is created, and can be customized there instead of being part of the trait.sandbox/*_api.rsare updated to be traits with blanket implementations forT: Sandboxinstead of impl blocks.These updates let us create custom sandbox environment that have full control over the storage and execution.
An example of this, is the
MockNetworkSandboxdefined in ink!that uses under the hood the xcm simulator to test connected smart contract that need to interact with a Network of chains.