-
Notifications
You must be signed in to change notification settings - Fork 429
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 traits #1857
[E2E alternative backend]: Backend traits #1857
Conversation
@ascjones this is now a half-baked PR, but I wanted kindly ask you if you could take a quick look and tell whether the direction is okay with you; if so, then I will continue with |
LMK when it is ready for review, I will convert the PR to |
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.
Looks good so far, carry on!
Not sure about the name Actor
, would be interested to know what the equivalent concrete types are for the Drink!
backend. That is a minor detail though.
…end-trait # Conflicts: # crates/e2e/src/subxt_client.rs
okay, I'm done for the first iteration now - @SkymanOne @ascjones please take a look :) notice that the PR description has been significantly expanded, hopefully giving some help/insights into some of the decisions that I made on spot |
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.
In general this all LGTM.
Another possibility for the interface would be to make it synchronous and adapt the subxt
client into sync methods. Though that will require lots of changes to existing tests so let's keep it async for now.
Also if Actor
is referring to Account
, couldn't we just make the types Account
and AccountId
?
Anyway, nothing from my side to prevent this PR from being merged. Looking forward to the next one!
I have just merged master and added licenses to the files that I recently created. Regarding |
@@ -46,14 +46,24 @@ use ink_env::{ | |||
}, |
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 am personally not a fun of renaming the file into subxt_client.rs
. IMO, it is better either to keep the name client.rs
or rename it to the impls.rs
like it's done in other ink! crates. Since, all the create provides an implementation for the backend traits
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 was intending to put drink backend into a separate module (like drink_client.rs
) to keep clear isolation of the very different code and hence this renaming. Would you suggest putting everything into a signle client.rs
/impls.rs
module?
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.
If you plan to integrate drink backend in the ink! e2e crate directly, then it makes sense to keep subxt_client.rs
name
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.
that's the plan, so that a developer when writing tests can only specify which backend they need without worrying about dependencies or imports
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.
Then it makes sense to keep the name
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.
Looks way better now. Happy to get it merged!
The PR introduces abstraction for all available operations within e2e tests. These actions are broken into two categories:
Thus there are two corresponding separate traits:
ChainBackend
andContractsBackend
that are combined further intoE2EBackend
.Notes / dilemmas
Mutability in backend methods
While so far, the subxt-based E2E client could work just on
&self
, we have to make all backend methods to operate on&mut self
. This is because drink! client will have to keep the whole runtime (externalities) by itself and thus any action will mutate its state.While some workaround might have been done (with some
RefCells
etc), IMHO it's completely not worth the effort and complication (at least at this stage).Asynchronicity of backend traits
While drink backend is inherently synchronous, subxt-based one cannot be. Unfortunately I don't know any ethic way of keeping common facade for such two alternatives and thus I made all methods asynchronous. As a result:
Breaking
E2EBackend
into two traitsThis is just a matter of taste - for me it is just cleaner to separate general-chain logic from the contract-specific (this is how I did it in drink! as well). However, there are 3 immediate downsides:
I'm not very dedicated to that support, so if any of the reviewers is in favor of having a single trait, I'm on it.
Awful associated types bounds repetition
Since there are actually 3
impl *Backend for Client
, there are 3 long blocks of identical lists of trait bounds for the types withinE
andC
(environment and subxt config). Unfortunately, due to: rust-lang/rust#20671 I cannot do much here (unless we want to include unstable feature oftrait_alias
).Actor
vsActorId
In the full context (subxt-based client) we usually have to differentiate between user's account and user's keypair: we use the first one to send tokens there or read its state, and the second one for signing transactions, that are sent to a node. However, in drink! backend, there is no need for signing anything, and thus these two concepts can be collapsed to just a user's account.
Dry-running
I am not sure if dry-running should be a part of the backend trait. It seems useful, but it's not that obvious how it should be implemented for drink!... For now I included it and I will see soon what is possible to do about it in drink!.