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

[E2E alternative backend]: Backend choice #1864

Merged
merged 28 commits into from
Aug 24, 2023

Conversation

pmikolajczyk41
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 commented Aug 2, 2023

Finally we provide implementation of E2EBackend trait for a drink-based client. Also, we add a new argument for e2e-test macro, enabling user to specify target backend architecture.

Since drink library is pretty immature yet, we are lacking some functionalities like dry-running or events. Relevant places in code are marked with the corresponding issues in drink repository. We'll fix them in the near future.

Notes:

  • added a new integration test based on flipper that uses new backend
  • had to add wasm-instrument with sign_ext feature because of compilation error: non-exhaustive patterns: &Instruction::SignExt(_) not covered for cargo 1.70
  • removed Actor and ActorId associated types from backend traits: since we have to support all backends for a single test code, we always have to operate with Keypair and E::AccountId types
  • moved common code for all backends to a common module client_utils
  • error type is for now (), I will adapt DrinkBackend to the main Error soon (required some changes to drink as well)

@pmikolajczyk41 pmikolajczyk41 marked this pull request as ready for review August 2, 2023 09:03
@deuszx deuszx mentioned this pull request Aug 18, 2023
2 tasks
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Looks very solid. Just a few nitpicks

crates/e2e/macro/src/config.rs Show resolved Hide resolved
crates/e2e/src/backend.rs Outdated Show resolved Hide resolved
/// Identifier type for an actor.
type ActorId;
/// Account type.
type AccountId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it make sense to add Clone + Send + Sync trait bounds to this associated type since it looks like you impose them later in drink_client anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

requiring bounds on the associated type here would be a bit implementation-based, so I'm not convinced if it is the correct move

but anyway, even if I put these bounds here, I would have to keep them in the impl<AccountId, *> *Backend for *, so nothing to 'gain' here

crates/e2e/src/client_utils.rs Show resolved Hide resolved
Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

LGTM!

@SkymanOne SkymanOne merged commit 7d0e0c8 into use-ink:master Aug 24, 2023
22 checks passed
@pmikolajczyk41 pmikolajczyk41 deleted the pmikolajczyk41/e2e-macro branch August 24, 2023 10:03
@@ -22,6 +22,7 @@ ink_primitives = { workspace = true, default-features = true }

cargo_metadata = { workspace = true }
contract-build = { workspace = true }
drink = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider having a feature flag for this, since it is bringing in a few heavy substrate dependencies and (at the moment) is not the default backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair point, I'll hide it tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it that heavy?

Copy link
Member Author

Choose a reason for hiding this comment

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

~70 new dependencies, some of which are substrate crates (for state-machines, externalities, few pallets etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

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