Improve E2E interface

[SkymanOne]

A thing that would be nice is to simplify the e2e API that at the moment requires sth like this:

.call(|flipper| flipper.get());

[ascjones]

There should be some way to invoke this as an associated function e.g. FlipperRef::get. Decouple the call from the account id.

May not be entirely straightforward since get is already an inherent method so need to figure out a way around that. Could e.g. generate an entirely new struct alongside FlipperRef called e.g. FlipperCall, or use different method names.

[SkymanOne]

May not be entirely straightforward since get is already an inherent method so need to figure out a way around that. Could e.g. generate an entirely new struct alongside FlipperRef called e.g. FlipperCall, or use different method names.

Not a fun of using different method names since it harms readability of the tests.
Do I understand correctly that we want to move the message call outside the .call() function?

[SkymanOne]

Proposed design

We keep the contract instantiation. Let's take mapping_integration_tests as an example:

let constructor = MappingsRef::new();
 let contract_id = client
    .instantiate(
        "mapping-integration-tests",
        &ink_e2e::charlie(),
        constructor,
        0,
        None,
    )
    .await
    .expect("instantiate failed")
    .account_id;

Instead of manually building a message:

let first_insert = ink_e2e::build_message::<MappingsRef>(contract_id)
    .call(|contract| contract.insert_balance(1_000));

and making an explicit call:

let _ = client
    .call(&ink_e2e::charlie(), first_insert, 0, None)
    .await
    .expect("Calling `insert_balance` failed")
    .return_value();

Let's unify the process under the automatically-generated associated message function. i.e we have mapping_integration_tests contract, when instantiating the contract as in the first snippet, we generate an associated trait MappingsRefCal as proposed by @ascjones which has the same message functions and incorporates the call dispatch logic.

So we can do something like:

// get the contracts' constructor
let constructor = MappingsRef::new();

// generate the associated trait for E2E
 let contract = client
    .instantiate(
        "mapping-integration-tests",
        &ink_e2e::charlie(),
        constructor,
        0,
        None,
    )
    .await
    .expect("instantiate failed")
    .account_id;

// we call a contract, await the result, unwrap `LangError` and return the value automatically
let _ = contract.insert_balance(1_000)
    .call()
    .await
    .expect("Calling `insert_balance` failed")
    .return_value()

In case the message is payable and/or we want to set a deposit limit, we can use Builder pattern and chain arguments into the final call:

let _ = contract.insert_balance(1_000)
    .with_storage_deposit(20_123)
    .with_value(30_000)
    // specify the account to sign a transaction
    .call(&ink_e2e::alice())
    .await
    .expect("Calling `insert_balance` failed")
    .return_value()

@ascjones @cmichi and @HCastano Looking forward your thoughts :)

[ascjones]

Yes this looks like a nice API, please go ahead and produce a prototype.

Keep in mind that we should attempt to minimize changes to the core codegen to facilitate this API. Where there are changes they should be non breaking to the main purpose of the ContractRef codegen: the API for doing cross contract calls.

[SkymanOne]

Update

I managed to come up with the design that simplifies the instantiation of the contract:

let contract_acc_id = FlipperCaller::new(true)
    .instantiate("flipper", &ink_e2e::dave(), &mut client)
    .await
    .expect("instantiation failed")
    .account_id;

However, it is problematic to instantiate a contract and create an instance of FlipperCaller at the same time because with FlipperCaller::new I "simply" return the constructor builder and then implemented instantiate method on it in e2e crate using existing Client.

After a little investigation I came up with two ways how to move forward:

Easy

We have two structures:

• ContractInstantiator which basically simply instantiates a contract like illustrated above and returns account_id and other output from the instantiation
• ContractCaller which takes account_id and client and initialised after instantiation (i.e. FlipperCaller::init(<account_id>, &mut client). It can then perform calls like described in the proposal.

The downside of this approach is that it is not very idiomatic as we now have two different structures for calls.

Proper

Ideally, we need to generate associated structure in e2e_tests module directly rather than in contract's module. We probably need to introduce attribute macro #[ink_e2e::tests] which will go on top of mod e2e_tests. It will then generate ContractCaller incorporating all the call build, dispatch and client logic, error handling, and other operations that present in e2e crate.

The reason why it is not possible to do in codegen crate is because it targets wasm and crates like subxt and cargo-metadata are not supported in this environment.

However, this approach is more difficult to implement, will require significantly more time than the first one and introduces breaking changes to the API whereas the first one can be used alongside the existing API.

@cmichi @ascjones please let me know how I should move forward.

[ascjones]

Out of the two I would go for the simpler approach, I don't think it is necessary to introduce a whole new layer of codegen. After all this idea of utilizing the existing ink call builders was to replace the previously used smart-bench-macro which was doing its own codegen based on the metadata. In the end we don't want to duplicate codegen logic that already exists and works very well for constructing cross-contract calls.

My feeling is there should be a way to coerce existing codegen and types to achieve a decent API, let me have a look and see whether I can see a possible third solution.

[ascjones]

I've made an experimental PR #1669 (for illustration and discussion purposes only) which is another approach to improve this call builder API. TLDR:

.call(|flipper| flipper.get());

becomes

flipper.call().get();

Let me know what you think.

[SkymanOne]

After a quick call with Michi, he also pointed out that additional_contracts and environment args can go to the module macro and be applied to all tests in the module automatically which will also improve DevEx.

With regard to #1669 this can be an intermediate improvement, but not what I had in mind.
I really want to achieve the same idiomatic syntax as we have in unit tests.

I don't see the reason why having a codegen in e2e would be bad. It doesn't bloat the WASM binary but provides better DevEx experience.

In the end we don't want to duplicate codegen logic that already exists and works very well for constructing cross-contract calls.

It won't be duplicated, it will be extended. Even if you look at my changes you'll see that the codegen for constructor simply wraps the method into ConstructorCallable.

[ascjones]

With regard to #1669 this can be an intermediate improvement, but not what I had in mind.

It's simply a solution to the problem of removing callback for generating calls, which was the original scope of the idea when I flagged it up.

I don't see the reason why having a codegen in e2e would be bad. It doesn't bloat the WASM binary but provides better DevEx experience.

Nothing inherently wrong, in some places it would most likely duplicate some boilerplate of generating and binding methods (although not complicated). My thinking was simply to minimize time and effort spent on it and extra code to write and maintain.

I really want to achieve the same idiomatic syntax as we have in unit tests.

Yes this would be great, although the way I was thinking about it was to mimic the cross-contract calling API since we have access already to all those builders and types.

Even if you look at my changes you'll see that the codegen for constructor simply wraps the method into ConstructorCallable.

I've not seen the code yet, do you have a link?

Anyway happy for you to proceed as you see fit, I am only providing some ideas, they are not necessarily correct.

[SkymanOne]

@ascjones Here is WIP PR: #1671

[ascjones]

I haven't looked in detail at the code, but much of the codegen does look similar to that of the existing contract ref. Again not so bad per se if there is a big benefit.

It would be good if one of the examples (flipper?) was updated to use the new API so we could see the finished product (even if it doesn't compile).

From my side I prefer small incremental API improvements, which was what I had in mind with the original suggestion to remove the callback from the call builder syntax. This looks so far like a much larger scoped undertaking, which may produce nice results, but we need to sure that it will be worth the effort.

[cmichi]

From my side I prefer small incremental API improvements, which was what I had in mind with the original suggestion to remove the callback from the call builder syntax. This looks so far like a much larger scoped undertaking, which may produce nice results, but we need to sure that it will be worth the effort.

@SkymanOne I agree with Andrew, this approach would become too involved right now. As we have bigger construction sites currently, let's go for a simpler approach. What are your thoughts on Andrew's #1669 ?

[SkymanOne]

I haven't looked in detail at the code, but much of the codegen does look similar to that of the existing contract ref. Again not so bad per se if there is a big benefit.

I actually did not remove a significant portion of code, as I am still working on the calling behaviour. The only method that is actually used right now is generate_contract_inherent_impl_for_constructor
One thing to note is that my changes do not break the existing codegen and call builder interfaces. They just extend them.

It would be good if one of the examples (flipper?) was updated to use the new API so we could see the finished product (even if it doesn't compile).

There is an example test with instantiation: instantiate_with_caller

From my side I prefer small incremental API improvements, which was what I had in mind with the original suggestion to remove the callback from the call builder syntax. This looks so far like a much larger scoped undertaking, which may produce nice results, but we need to sure that it will be worth the effort.

While I agree with this approach, the problem is that each incremental step with break an API which we can not afford as we rolling into main nets and our developer base grows. Ideally, I would like to get this breaking change into the next major release and do not change API after. Therefore, it sounds more reasonable to me to do introduce the major change I originally proposed and stick with it. Introducing module wide attribute and codegen for e2e tests will have more benefits in future. It will be easier to introduce new changes without breaking API.

[ascjones]

There is an example test with instantiation: instantiate_with_caller

Yes, although that does still use build_message for the callback for the call to flip(), which after all was the original motivation for this. I'd like to see that extended to show how the API will look as you have described in the

the problem is that each incremental step with break an API which we can not afford as we rolling into main nets and our developer base grows.

E2E tests are brand new and we can afford to iterate and break stuff to get to a nice API. IMO we are not that far away from having a nice API already.

Introducing module wide attribute and codegen for e2e tests will have more benefits in future. It will be easier to introduce new changes without breaking API.

What benefits do you imagine? We only need to be able to construct contract messages in an ergonomic fashion, which we are already a long way towards. Also doing a lot of work up front for possible future benefits usually not worth it.

However, this approach is more difficult to implement, will require significantly more time than the first one and introduces breaking changes to the API whereas the first one can be used alongside the existing API.

As you say this requires significantly more time from you in the short term, but there are also other costs to consider:

• Reviewing time of other developers
• Future maintenance costs and tech debt of adding more code
• Opportunity costs: time spent on other features

So, having considered those additional costs, only if we still think it is worth it for the benefits we should go ahead.

[SkymanOne]

What benefits do you imagine? We only need to be able to construct contract messages in an ergonomic fashion, which we are already a long way towards. Also doing a lot of work up front for possible future benefits usually not worth it.

As mentioned earlier with top level attribute macro we can pass additional_contracts and environment to all tests automatically which is a good improvement.

Yes, although that does still use build_message for the callback for the call to flip(), which after all was the original motivation for this.

I'll have a PoC for the messages too. For now, it'll be a separate associated structure as described earlier. In this PoC it will still use build_message under the hood, but I plan to rewrite the logic if we go with my attribute macro.

As you say this requires significantly more time from you in the short term, but there are also other costs to consider...

• Review time is a fair point. I don't expect it to be ready within this week, but closer to the next release.
• The logic itself for call building will not be too dissimilar. I will try to keep codegen logic as simple as possible.
• Opportunity cost, this is the only feature I am working in ink! apart from Add ability to use quickcheck with E2E testing #1647 which is blocked. If there are more pending issues to focus on, please let me know