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

Update api to enable custom runtime #99

Merged
merged 13 commits into from
Feb 9, 2024
Merged

Conversation

pgherveou
Copy link
Collaborator

@pgherveou pgherveou commented Feb 5, 2024

This PR update how the API is structured:

the Runtime trait is gone. The way it was structured,made it impossible to bring in an existing Runtime and pass it to a drink or ink! test. This PR moves these APIs to a SandboxConfig trait, and make the Sandbox generic over that trait.

This let us write tests like this

impl_sandbox_config!(
    struct MyConfig {
        runtime: MyRuntime;
        default_balance: DEFAULT_BALANCE;
        default_actor: ALICE;
    }
);

#[drink::test(config = MyConfig)]
fn test_flipping_with_custom_runtime(mut session: Session) -> Result<(), Box<dyn std::error::Error>> {
    // snip
}

this create_minimal_runtime macro is still there, it just now call the new impl_sandbox_config macro internally to implement the SandboxConfig trait.

I have an upcoming PR to ink! that will leverage these new API to call e2e tests against a custom runtime.
Specifically it will be used with the pallet-contracts-mock-network crate to test XCM host functions

use pallet_contracts_mock_network::{parachain::Runtime as ParachainRuntime, ALICE};

impl_sandbox_config!(
    struct ParachainSandbox {
        runtime: ParachainRuntime;
        default_balance: 1_000_000_000_000_000;
        default_actor: ALICE;
    }
);

/// Positive case scenario:
#[ink_e2e::test(backend(runtime_only(runtime = ParachainSandbox)))]
async fn transfer_with_xcm_execute_works<Client: E2EBackend>(
    mut client: Client,
) -> E2EResult<()> {
    // snip
}

@pgherveou pgherveou marked this pull request as ready for review February 5, 2024 19:26
@pmikolajczyk41 pmikolajczyk41 self-requested a review February 6, 2024 11:02
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.

can you please elaborate on why the previous API was inconvenient? the difference is quite subtle

Cargo.toml Outdated Show resolved Hide resolved
examples/flipper/Cargo.toml Show resolved Hide resolved
drink/src/session/record.rs Outdated Show resolved Hide resolved
drink/src/session.rs Show resolved Hide resolved
@pgherveou
Copy link
Collaborator Author

can you please elaborate on why the previous API was inconvenient? the difference is quite subtle

With the current API we expect that our (frame) runtime also implement drink::Runtime. We can do that because we create the runtime with the macro, so it's local to the crate where we use it.

If instead we try to bring in a (frame) Runtime that is declared in another crate. Then because of the orphan rule we won't be able to implement the foreign trait (drink::Runtime) on our (foreign frame) runtime.

To make this use case work, this change compose drink::Runtime inside a new Config trait, instead of using trait inheritance

@pgherveou
Copy link
Collaborator Author

pgherveou commented Feb 8, 2024

For reference this is how I am using this PR in ink!

https://github.com/paritytech/ink/blob/b0762ce5c066d66ffb9f8b27ff58ae9a2e9a1974/crates/e2e/src/drink_client.rs#L463-L479

/// Exposes preset sandbox configurations to be used in tests.
pub mod preset {
    use drink::impl_sandbox_config;
    use pallet_contracts_mock_network::{
        parachain::Runtime as ParachainRuntime,
        ALICE, INITIAL_BALANCE
    };

    drink::impl_sandbox_config!(
        #[doc = "A Sandbox configuration for the parachain runtime of pallet-contracts-mock-network."]
        pub struct ParachainConfig {
            runtime: ParachainRuntime;
            default_balance: INITIAL_BALANCE;
            default_actor: ALICE;
        }
    );
}

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.

'orphan rule' is the thing I was missing - thank you for pointing this, as well as for the contribution!

@pmikolajczyk41 pmikolajczyk41 merged commit d47e00c into main Feb 9, 2024
2 checks passed
@pmikolajczyk41 pmikolajczyk41 deleted the pg/update-sandbox branch February 9, 2024 10:34
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.

3 participants