[E2E] Call builders and extra gas margin option #1917

SkymanOne · 2023-09-16T17:50:30Z

Summary

[y] y/n | Does it introduce breaking changes?
[y] y/n | Is it dependent on the specific version of cargo-contract or pallet-contracts? - ink! 5.0.0-alpha

This PR reworks the E2E API by introducing the builder API. As part of this PR, the user can specify gas margin in percentage as part of the on-chain call.

Description

Builder API

instantiate, call and upload will now return a builder instance. The user can specify optional arguments with builder methods, and submit the call for on-chain execution with .submit() method, or dry-run it with dry_run().

let contract = client
    .instantiate("flipper", &ink_e2e::alice(), &mut constructor)
    .submit()
    .await
    .expect("instantiate failed");
let mut call = contract.call::<Flipper>();

let get = call.get();
let get_res = client.call(&ink_e2e::bob(), &get).dry_run().await;
assert!(matches!(get_res.return_value(), false));

Extra gas margin

There are cases when gas estimates may not necessarily be accurate enough due to the complexity of the smart contract logic that adds additional overhead and gas consumption. Therefore, it is helpful to allow to specify an extra portion of the gas to be added to the limit (i.e. 5%, 10%). This functionality was achieved with .extra_gas_portion(margin: u64) method as part of the builder API.

Internal changes

Environment trait now extends Clone in order to make CreateBuilder cloneable. There are other structs that add Clone trait bound as well.

Extract from flipper:

let constructor = FlipperRef::new(false);
let contract = client
    .create_instantiate_call("flipper", &ink_e2e::alice(), constructor)
    .submit()
    .await
    .expect("instantiate failed");
let mut call = contract.call::<Flipper>();

let get = call.get();
let get_res = client
    .create_call(&ink_e2e::bob(), &get)
    .submit_dry_run()
    .await;

Checklist before requesting a review

My code follows the style guidelines of this project
I have added an entry to CHANGELOG.md
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

@pmikolajczyk41 Pinging for review as well

ascjones · 2023-09-18T10:17:27Z

integration-tests/call-builder-return-value/lib.rs

            let call = call_builder_call.delegate_call(code_hash, selector);
            let call_result = client
-                .call(&origin, &call, 0, None)
+                .call(&origin, &call, 0, None, None)


Maybe now is the time to finally convert to a builder style API, since the default values 0, None, None are the most common.

Maybe it is 😄
Something like:

let call_result = client .build_call(&origin, &call) .value(100) .storage_deposit_limit(100) .extra_gas_portion(5) .send() .await() ...

We should do a builder API, but maybe as a separate task where we can design it properly

I think a more minimal approach to this problem (applying the extra gas portion without breaking the call API) would be:

Have the Client store a value for the "extra portion"

Set it to some default low value e.g. 2%

Allow overriding the value via e.g. client.set_extra_gas(2u8) (this could also be configured via the e2e test attribute which initialises the client)

ascjones · 2023-09-25T14:10:05Z

crates/e2e/src/subxt_client.rs

        caller: &Keypair,
        message: &CallBuilderFinal<E, Args, RetType>,
        value: E::Balance,
+        extra_gas_portion: Option<usize>,


the old instantiate and call are left unchanged so this PR doesn't introduce breaking changes

This looks like a breaking change.

Not a huge problem for it to be a breaking change, just the fact that in 99% of cases the value passed will be None.

One approach would be to have a new method e.g. bare_call which just passes raw arguments to (in this case) self.api.call(). The gas_limit would be an absolute value and the part of adding the "portion" could just be done in the builder exec method.

Then we could even keep this method as is so it is a non-breaking change if we wanted. But this is not strictly necessary since we are doing a major release...it will be just annoying for users to have to update all their tests.

The gas_limit would be an absolute value and the part of adding the "portion" could just be done in the builder exec method.

I am not fun of an idea of having 3 different ways of building a call. We are going to end up with call(), bare_call() and call builder which serve similar purposes. This will just confuse people

Additionally, there is no way of specifying gas limit in E2E anyway, so it will be a breaking change nonetheless

ascjones · 2023-09-25T14:15:35Z

integration-tests/contract-transfer/lib.rs


            let call_res = client
-                .call(&ink_e2e::eve(), &transfer, 0, None)
+                .call(&ink_e2e::eve(), &transfer, 0, None, None)


As far as I can see we are not using this API anywhere in the examples.

If we are going to move to this API and have it a breaking change anyway, we can just change call to return the call builder, so the default becomes in this case:

let call_res = client .call(&ink_e2e::eve(), &transfer) .submit()

SkymanOne · 2023-09-26T11:14:06Z

I think the best course of action is to have bare_call() where you can specify all args explicitly (including the gas limit), and we are going to have a builder with a gas limit margin.

pmikolajczyk41

looks nice, we might want a similar excess to be specified in cargo-contract as well

crates/e2e/src/backend.rs

crates/e2e/src/backend_calls.rs

SkymanOne · 2023-09-28T00:08:47Z

Converting to draft as Instantiatebuilder needs to be reworked. This is due to submit() taking self and a move occurs which is because CreateBuilder<> does not implement Clone

pmikolajczyk41

dry-running in drink!: it's already there, ready to be used at any time

the new API: I really like the new flow with client.action().modifier1().modifier2().submit/dry_run(). However, IMHO, the current traiting and API needs some encapsulation. In particular:

For the user (e2e test author) I would expose only a single API for interacting with a contract (upload/instantiate/call). Preferably, I would remove bare_* and bare_*_dry_run from ContractsBackend. These methods do not add any more expressiveness, but instead may introduce some confusion + it makes us to support and care for broader API.
I would move things like bare_instantiate and bare_instantiate_dry_run into a crate private trait, like Submitter or whatever name you like. This trait would be completely internal, used only for callbacks from CallBuilder/InstantiateBuilder/UploadBuilder implementations. Of course, this trait can and probably should be implemented by clients, but anyway, shouldn't be accessible by the end user. Please notice, that you don't need both bare_instantiate and instantiate_with_gas_margin (and thus BuilderClient is not needed).

Side note/question: I don't know if it is intended, but the E2E backend is missing instantiation without upload. I mean the scenario where we upload the code and then create contract instance in separate transactions. Should we use then upload and instantiate calls? Probably it should work... This is why there's some discrepancy with drink! where I used deploy term for packing both operations into a single action.

crates/e2e/src/backend.rs

crates/e2e/src/backend_calls.rs

crates/e2e/src/backend.rs

SkymanOne · 2023-10-02T10:25:46Z

For the user (e2e test author) I would expose only a single API for interacting with a contract (upload/instantiate/call). Preferably, I would remove bare_* and bare_*_dry_run from ContractsBackend. These methods do not add any more expressiveness, but instead may introduce some confusion + it makes us to support and care for broader API.

That's a fair point. Th initial idea behind having bare_ calls is that allow more "precise" control over execution such as running dry-run and submitting extrinsics in separate calls and specifying gas limit separately.

I'll see if I can encapsulate this login into the builder API as well

ascjones

Main thing would be to simplify the BuilderClient trait, could potentially remove a lot of code, and some duplication between the client implementations (moved to the builder itself).

Builder API itself looks good 👍

crates/e2e/src/backend_calls.rs

integration-tests/call-builder-return-value/lib.rs

crates/e2e/src/backend.rs

crates/e2e/src/subxt_client.rs

pmikolajczyk41

the new API looks very nice to me; I agree with Andrew's suggestions, but apart from them, it's good for me

I guess some minor implementation parts might be changed or refactored in the future, but the public interface seems to be final

pmikolajczyk41 · 2023-10-07T18:13:48Z

crates/e2e/src/backend.rs

+    fn instantiate<'a, Contract: Clone, Args: Send + Clone + Encode + Sync, R>(
+        &'a mut self,
+        contract_name: &'a str,
+        caller: &'a Keypair,


just wondering... maybe alice() should be moved to default as well (similarly to value, storage deposit and margin)?

IMHO, the caller should be specified explicitly. I don't want to hide this information from the developer per se

pmikolajczyk41 · 2023-10-07T18:18:57Z

crates/e2e/src/backend_calls.rs

+    }
+
+    /// Provide value with a call
+    pub fn value(&mut self, value: E::Balance) -> &mut Self {


why not pub fn value(mut self, value: E::Balance) -> Self?

We don't want to move values out of references, and, generally speaking, it is a good practice to implement non-consuming builders where possible:
https://doc.rust-lang.org/1.0.0/style/ownership/builders.html#non-consuming-builders-(preferred):

Co-authored-by: Andrew Jones <[email protected]>

SkymanOne · 2023-10-07T22:24:33Z

@ascjones To address your comments with regard to using bare_call and bare_instantiate and handling dry-run results from the builder: the main problem lies in error handling from the dry-run result as I pointed out above. To address this issue, there needs to be a standardised error handling between drink and subxt. Specifically, we need to have

type Error: ExecutionErrors where ExecutionError is a trait with method like return_dry_run_error and so on. Then I can do something like:

if dry_run.result.is_err() {
    return Err(return_dry_run_error(dry_run))
}

But then we are going to end up with the same amount of code as drink and subxt clients will have to implement ExectionErrors trait.

ascjones · 2023-10-17T14:36:31Z

@ascjones To address your comments with regard to using bare_call and bare_instantiate and handling dry-run results from the builder: the main problem lies in error handling from the dry-run result as I pointed out above. To address this issue, there needs to be a standardised error handling between drink and subxt. Specifically, we need to have

type Error: ExecutionErrors where ExecutionError is a trait with method like return_dry_run_error and so on. Then I can do something like:
if dry_run.result.is_err() {
    return Err(return_dry_run_error(dry_run))
}
But then we are going to end up with the same amount of code as drink and subxt clients will have to implement ExectionErrors trait.

See #1938 for an approach of how to solve this.

* WIP * Attempt to refactor to unify backend impls * Trait types * submit instantiate * Warnings

paritytech-cicd-pr · 2023-10-18T11:45:52Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-acc02b1 and comparing them to ink! master:

Contract	Upstream Size (kB)	PR Size (kB)	Diff (kB)	Diff (%)	Change
basic-contract-caller	2.998	2.998	0	0	➖
basic-contract-caller/other-contract	1.331	1.331	0	0	➖
call-builder-return-value	8.734	8.734	0	0	➖
call-runtime	1.77	1.77	0	0	➖
conditional-compilation	1.204	1.204	0	0	➖
contract-terminate	1.087	1.087	0	0	➖
contract-transfer	1.446	1.446	0	0	➖
custom-allocator	7.39	7.39	0	0	➖
dns	7.313	7.313	0	0	➖
e2e-call-runtime	1.057	1.057	0	0	➖
e2e-runtime-only-backend	1.633	1.633	0	0	➖
erc1155	14.113	14.107	-0.006	-0.042514	📉
erc20	6.572	6.572	0	0	➖
erc721	9.547	9.547	0	0	➖
events	4.816	4.816	0	0	➖
flipper	1.387	1.387	0	0	➖
incrementer	1.217	1.217	0	0	➖
lang-err-integration-tests/call-builder-delegate	2.321	2.321	0	0	➖
lang-err-integration-tests/call-builder	4.875	4.875	0	0	➖
lang-err-integration-tests/constructors-return-value	1.772	1.772	0	0	➖
lang-err-integration-tests/contract-ref	4.363	4.363	0	0	➖
lang-err-integration-tests/integration-flipper	1.565	1.565	0	0	➖
mapping-integration-tests	2.975	2.975	0	0	➖
mother	9.499	9.499	0	0	➖
multi-contract-caller	5.977	5.977	0	0	➖
multi-contract-caller/accumulator	1.09	1.09	0	0	➖
multi-contract-caller/adder	1.672	1.672	0	0	➖
multi-contract-caller/subber	1.693	1.693	0	0	➖
multisig	21.406	21.406	0	0	➖
payment-channel	5.544	5.544	0	0	➖
sr25519-verification	0.865	0.865	0	0	➖
static-buffer	1.411	1.411	0	0	➖
trait-dyn-cross-contract-calls	2.491	2.491	0	0	➖
trait-dyn-cross-contract-calls/contracts/incrementer	1.3	1.3	0	0	➖
trait-erc20	6.956	6.956	0	0	➖
trait-flipper	1.204	1.204	0	0	➖
trait-incrementer	1.365	1.365	0	0	➖
upgradeable-contracts/delegator	2.902	2.902	0	0	➖
upgradeable-contracts/delegator/delegatee	1.368	1.368	0	0	➖
upgradeable-contracts/set-code-hash	1.46	1.46	0	0	➖
upgradeable-contracts/set-code-hash/updated-incrementer	1.439	1.439	0	0	➖
wildcard-selector	2.619	2.619	0	0	➖

Link to the run | Last update: Wed Oct 18 13:45:51 CEST 2023

add extra arg to specify extra gas portion

bc27b10

SkymanOne requested review from ascjones and cmichi as code owners September 16, 2023 17:50

SkymanOne added 3 commits September 16, 2023 19:51

changelog

313d351

fmt

2578e5a

fix the test

116caf6

ascjones reviewed Sep 18, 2023

View reviewed changes

SkymanOne added 2 commits September 25, 2023 14:38

add call builders

024405b

Update changelog

e7b9c0e

SkymanOne changed the title ~~[E2E] Add extra arg to specify extra gas portion~~ [E2E] E2E call builders and extra gas margin option Sep 25, 2023

SkymanOne requested a review from ascjones September 25, 2023 13:40

fix spelling

da053c6

ascjones reviewed Sep 25, 2023

View reviewed changes

pmikolajczyk41 approved these changes Sep 26, 2023

View reviewed changes

crates/e2e/src/backend.rs Show resolved Hide resolved

crates/e2e/src/backend_calls.rs Show resolved Hide resolved

SkymanOne added 11 commits September 27, 2023 01:55

introduce bare calls alongside builder api

e872567

add docs

9df632f

add upload builder

86fab73

remove async in upload

9e7814e

ignore doctest

2b3260f

update integration tests

3bef64c

submit_dry_run() -> dry_run()

ad43cce

format tests

5109e64

fixes

9e5b311

fmt

7e7a02e

another fmt

8e8713f

SkymanOne requested review from ascjones and pmikolajczyk41 September 27, 2023 15:36

SkymanOne marked this pull request as draft September 28, 2023 00:08

SkymanOne added 2 commits September 29, 2023 01:40

keep Clone with custom env in UI tests

61a0fa0

remove duplicate derive

6af9c46

pmikolajczyk41 suggested changes Sep 29, 2023

View reviewed changes

crates/e2e/src/backend.rs Outdated Show resolved Hide resolved

crates/e2e/src/backend.rs Outdated Show resolved Hide resolved

crates/e2e/src/backend_calls.rs Outdated Show resolved Hide resolved

crates/e2e/src/backend.rs Outdated Show resolved Hide resolved

SkymanOne added 5 commits October 3, 2023 18:24

add proper dry-run in drink

02bb3ec

reformat the trait and add gas limit option

2d62be8

fmt example

6be1cdc

update docs

8036919

Merge branch 'master' into gn/extra_gas_config

1bebfc9

SkymanOne requested a review from pmikolajczyk41 October 3, 2023 21:32

SkymanOne added 2 commits October 4, 2023 12:11

fix CI by adding necessary trait bounds

d7c6be4

fix CI

7d6aafa

ascjones reviewed Oct 6, 2023

View reviewed changes

pmikolajczyk41 approved these changes Oct 7, 2023

View reviewed changes

Update integration-tests/call-builder-return-value/lib.rs

3d8f9ba

Co-authored-by: Andrew Jones <[email protected]>

Merge branch 'master' into gn/extra_gas_config

d3560c3

ascjones and others added 6 commits October 17, 2023 17:36

Suggestions for removing code duplication in #1917 (#1938)

7542d5a

* WIP * Attempt to refactor to unify backend impls * Trait types * submit instantiate * Warnings

add dummy error struct to drink

aef2dcd

Merge branch 'master' into gn/extra_gas_config

c5b78de

remove default

c649b58

pass args to instantiate dry run correctly

c5c1de5

add license files

ccac9ff

SkymanOne merged commit 9354274 into master Oct 18, 2023

SkymanOne deleted the gn/extra_gas_config branch October 18, 2023 12:23

SkymanOne mentioned this pull request Nov 30, 2023

5.0.0-rc Release #2011

Merged

SkymanOne mentioned this pull request Jan 29, 2024

Add migration guide from ink! 4.x to 5.0 use-ink/ink-docs#301

Merged

SkymanOne mentioned this pull request Mar 4, 2024

Release 5.0.0 #2133

Merged

[E2E] Call builders and extra gas margin option #1917

[E2E] Call builders and extra gas margin option #1917

Uh oh!

Conversation

SkymanOne commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description

Builder API

Extra gas margin

Internal changes

Checklist before requesting a review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SkymanOne Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SkymanOne commented Sep 26, 2023

Uh oh!

pmikolajczyk41 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SkymanOne commented Sep 28, 2023

Uh oh!

pmikolajczyk41 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SkymanOne commented Oct 2, 2023

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmikolajczyk41 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SkymanOne Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SkymanOne commented Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascjones commented Oct 17, 2023

Uh oh!

paritytech-cicd-pr commented Oct 18, 2023

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

Uh oh!

Reviewers

Assignees

Labels

SkymanOne commented Sep 16, 2023 •

edited

Loading

SkymanOne Sep 18, 2023 •

edited

Loading

pmikolajczyk41 left a comment •

edited

Loading

SkymanOne Oct 7, 2023 •

edited

Loading

SkymanOne commented Oct 7, 2023 •

edited

Loading