Skip to content
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

Refactor Sandbox #84

Merged
merged 20 commits into from
Nov 10, 2023
Merged

Refactor Sandbox #84

merged 20 commits into from
Nov 10, 2023

Conversation

pgherveou
Copy link
Collaborator

@pgherveou pgherveou commented Nov 8, 2023

This PR does the following refactoring, to prepare further update that will make it easier to integrate Drink with a Network of chains connected through XCM:

Specifically it does the following:

  • Use generics instead of hard coded types, so we can adapt e2e tests to various runtimes
  • Refactor Sandbox so that it can be used with runtime that do not implement contracts (such as a Relay chain Runtime for example)

For example token APIs only require the Sandbox to be generic over something that implement pallet_contracts

impl<R: pallet_balances::Config> Sandbox<R> {
    /// Add tokens to an account.
    ///
    /// # Arguments
    ///
    /// * `address` - The address of the account to add tokens to.
    /// * `amount` - The number of tokens to add.
    pub fn add_tokens(
        &mut self,
        address: AccountIdFor<R>,
        amount: BalanceOf<R>,
    ) -> Result<BalanceOf<R>, DispatchError> {
        self.execute_with(|| pallet_balances::Pallet::<R>::mint_into(&address, amount))
    }

    /// Add tokens to an account.
    ///
    /// # Arguments
    ///
    /// * `address` - The address of the account to add tokens to.
    /// * `amount` - The number of tokens to add.
    pub fn balance(&mut self, address: &AccountIdFor<R>) -> BalanceOf<R> {
        self.execute_with(|| pallet_balances::Pallet::<R>::free_balance(address))
    }
}

drink/src/chain_api.rs Outdated Show resolved Hide resolved
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

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

I love the direction of this changes, that would finally allow to work freely with any runtime, not only for contract purposes, but for general interaction as well

if I understand the intention correctly, the idea is that:

  • Sandbox essentially wraps externalities and has a generic argument for a runtime, which can be actually anyhow configured
  • depending on this generic argument, Sandbox enables pallet-specific operations, but not through traits (like ChainApi and ContractApi), but rather through generic bounds

is that so?

drink/src/session.rs Outdated Show resolved Hide resolved
drink/src/session.rs Outdated Show resolved Hide resolved
drink/src/runtime/mod.rs Show resolved Hide resolved
drink/src/lib.rs Outdated Show resolved Hide resolved
drink/src/chain_api.rs Outdated Show resolved Hide resolved
drink/src/chain_api.rs Outdated Show resolved Hide resolved
drink/src/chain_api.rs Outdated Show resolved Hide resolved
@pgherveou
Copy link
Collaborator Author

I love the direction of this changes, that would finally allow to work freely with any runtime, not only for contract purposes, but for general interaction as well

if I understand the intention correctly, the idea is that:

* `Sandbox` essentially wraps externalities and has a generic argument for a runtime, which can be actually anyhow configured

* depending on this generic argument, `Sandbox` enables pallet-specific operations, but not through traits (`like ChainApi` and `ContractApi`), but rather through generic bounds

is that so?

Yes. That's pretty much what Sandbox was doing already (as far as I understand).
I just moved the constraints around to make it more flexible and work with any Runtime as long as they implement the right config. We could re-introduce the traits and implement them for runtime that implement the right configs but I don't think it's necessary, simple generics bounds implementation should be enough here.

@pgherveou
Copy link
Collaborator Author

Addressed your comments

Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

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

thank you very much for this contribution!

@@ -61,7 +61,7 @@ mod tests {
ContractMock::new().with_message(CALLEE_SELECTOR, mock_message(|()| RETURN_VALUE));

// Secondly, we deploy it, similarly to a standard deployment action.
let mock_address = session.mocking_api().deploy(mocked_contract);
let mock_address = session.sandbox().deploy(mocked_contract);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a regression that users now have to even know about the Sandbox. It was supposed to be a low-level object and developers use high-level session.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah right this is indeed a bit confusing without the "mocking_api" name
will update

Copy link
Collaborator

@deuszx deuszx Nov 9, 2023

Choose a reason for hiding this comment

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

It's not only about the name - name is just a "gate" into the Sandbox world. Of course, we can't stop users from using it, if they want, but I think the API should lead developer through the path of least resistance and suggest usage of simple constructs.

I'd prefer to leave the previous approach, if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the issue here is that the mock utilities should not be tied to the Sandbox struct.
I restored the API and moved these props to the session in the last commit.
lmk what you think

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good, thanks :)

Makefile Outdated Show resolved Hide resolved
@pgherveou
Copy link
Collaborator Author

@deuszx @pmikolajczyk41 not sure what kind of approval is required to trigger CI here, you mind re-approving so we can merge this asap?

@pmikolajczyk41 pmikolajczyk41 merged commit c3f4904 into inkdevhub:main Nov 10, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants