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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 52 commits into from
Oct 18, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
bc27b10
add extra arg to specify extra gas portion
Sep 16, 2023
313d351
changelog
Sep 16, 2023
2578e5a
fmt
Sep 16, 2023
116caf6
fix the test
Sep 18, 2023
024405b
add call builders
Sep 25, 2023
e7b9c0e
Update changelog
Sep 25, 2023
da053c6
fix spelling
Sep 25, 2023
e872567
introduce bare calls alongside builder api
Sep 27, 2023
9df632f
add docs
Sep 27, 2023
86fab73
add upload builder
Sep 27, 2023
9e7814e
remove async in upload
Sep 27, 2023
2b3260f
ignore doctest
Sep 27, 2023
3bef64c
update integration tests
Sep 27, 2023
ad43cce
`submit_dry_run()` -> `dry_run()`
Sep 27, 2023
5109e64
format tests
Sep 27, 2023
9e5b311
fixes
Sep 27, 2023
7e7a02e
fmt
Sep 27, 2023
8e8713f
another fmt
Sep 27, 2023
8cdf350
Make env cloneable and take constructor as mut ref
Sep 28, 2023
505e582
one more fix
Sep 28, 2023
48e9543
remove unnecessary trait bound
Sep 28, 2023
93221db
remove more trait bounds
Sep 28, 2023
a8ccedf
fix tests and fmt
Sep 28, 2023
3c600b4
fix tests and tweak drink backend
Sep 28, 2023
0c076f4
move margin call to separate trait
Sep 28, 2023
3d3e04d
update docs
Sep 28, 2023
f1a3228
add BuilderClient as supertrait of E2EBackend and fix docs
Sep 28, 2023
bb332ad
fix more tests and docs
Sep 28, 2023
975762a
one last doc fix
Sep 28, 2023
714d7d8
Merge branch 'master' into gn/extra_gas_config
Sep 28, 2023
2855db2
UI tests
Sep 28, 2023
c091243
Remove unecessary trait bounds
Sep 29, 2023
5f11cbc
remove move unecessary trait bounds
Sep 29, 2023
62b1608
one more missing trait
Sep 29, 2023
d96fed4
Revert "UI tests"
Sep 29, 2023
61a0fa0
keep Clone with custom env in UI tests
Sep 29, 2023
6af9c46
remove duplicate derive
Sep 29, 2023
02bb3ec
add proper dry-run in drink
Oct 3, 2023
2d62be8
reformat the trait and add gas limit option
Oct 3, 2023
6be1cdc
fmt example
Oct 3, 2023
8036919
update docs
Oct 3, 2023
1bebfc9
Merge branch 'master' into gn/extra_gas_config
Oct 3, 2023
d7c6be4
fix CI by adding necessary trait bounds
Oct 4, 2023
7d6aafa
fix CI
Oct 4, 2023
3d8f9ba
Update integration-tests/call-builder-return-value/lib.rs
Oct 7, 2023
d3560c3
Merge branch 'master' into gn/extra_gas_config
Oct 7, 2023
7542d5a
Suggestions for removing code duplication in #1917 (#1938)
ascjones Oct 17, 2023
aef2dcd
add dummy error struct to drink
Oct 17, 2023
c5b78de
Merge branch 'master' into gn/extra_gas_config
Oct 17, 2023
c649b58
remove default
Oct 17, 2023
c5c1de5
pass args to instantiate dry run correctly
Oct 18, 2023
ccac9ff
add license files
Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]

## Changed
- [E2E] Add extra arg to specify extra gas portion - [#1917](https://github.com/paritytech/ink/pull/1917)
- Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906)

## Version 5.0.0-alpha
Expand Down
1 change: 1 addition & 0 deletions crates/e2e/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ pub trait ContractsBackend<E: Environment> {
caller: &Keypair,
message: &CallBuilderFinal<E, Args, RetType>,
value: E::Balance,
extra_gas_portion: Option<usize>,
storage_deposit_limit: Option<E::Balance>,
) -> Result<CallResult<E, RetType, Self::EventLog>, Self::Error>
where
Expand Down
8 changes: 7 additions & 1 deletion crates/e2e/src/drink_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ where
caller: &Keypair,
message: &CallBuilderFinal<E, Args, RetType>,
value: E::Balance,
extra_gas_portion: Option<usize>,
storage_deposit_limit: Option<E::Balance>,
) -> Result<CallResult<E, RetType, Self::EventLog>, Self::Error>
where
Expand All @@ -300,12 +301,17 @@ where
let exec_input = Encode::encode(message.clone().params().exec_input());
let account_id = (*account_id.as_ref()).into();

let mut gas_limit = DEFAULT_GAS_LIMIT;
if let Some(gas_portion) = extra_gas_portion {
gas_limit += gas_limit / 100 * (gas_portion as u64);
}

let result = self.sandbox.call_contract(
account_id,
value,
exec_input,
keypair_to_account(caller),
DEFAULT_GAS_LIMIT,
gas_limit,
storage_deposit_limit,
);

Expand Down
8 changes: 7 additions & 1 deletion crates/e2e/src/subxt_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ where
caller: &Keypair,
message: &CallBuilderFinal<E, Args, RetType>,
value: E::Balance,
extra_gas_portion: Option<usize>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

storage_deposit_limit: Option<E::Balance>,
) -> Result<CallResult<E, RetType, Self::EventLog>, Self::Error>
where
Expand All @@ -542,12 +543,17 @@ where
return Err(Error::<E>::CallDryRun(dry_run.exec_result))
}

let mut gas_limit = dry_run.exec_result.gas_required;
if let Some(gas_portion) = extra_gas_portion {
gas_limit += gas_limit / 100 * (gas_portion as u64);
}

let tx_events = self
.api
.call(
subxt::utils::MultiAddress::Id(account_id.clone()),
value,
dry_run.exec_result.gas_required.into(),
gas_limit.into(),
storage_deposit_limit,
exec_input,
caller,
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/call-builder-return-value/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ mod call_builder {
let selector = ink::selector_bytes!("get");
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SkymanOne SkymanOne Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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()
    ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. Have the Client store a value for the "extra portion"
  2. Set it to some default low value e.g. 2%
  3. 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)

.await
.expect("Client failed to call `call_builder::invoke`.")
.return_value();
Expand Down Expand Up @@ -232,7 +232,7 @@ mod call_builder {
let selector = ink::selector_bytes!("get");
let call = call_builder_call.forward_call(incrementer.account_id, selector);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Client failed to call `call_builder::invoke`.")
.return_value();
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/call-runtime/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ mod runtime_call {
call.transfer_through_runtime(receiver, TRANSFER_VALUE);

let call_res = client
.call(&ink_e2e::alice(), &transfer_message, 0, None)
.call(&ink_e2e::alice(), &transfer_message, 0, None, None)
.await
.expect("call failed");

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/contract-terminate/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ pub mod just_terminates {
// when
let terminate_me = call.terminate_me();
let call_res = client
.call(&ink_e2e::alice(), &terminate_me, 0, None)
.call(&ink_e2e::alice(), &terminate_me, 0, None, None)
.await
.expect("terminate_me messages failed");

Expand Down
6 changes: 4 additions & 2 deletions integration-tests/contract-transfer/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,9 @@ pub mod give_me {
// when
let transfer = call.give_me(120);

let call_res = client.call(&ink_e2e::bob(), &transfer, 10, None).await;
let call_res = client
.call(&ink_e2e::bob(), &transfer, 10, None, None)
.await;

// then
if let Err(ink_e2e::Error::<ink::env::DefaultEnvironment>::CallDryRun(
Expand Down Expand Up @@ -252,7 +254,7 @@ pub mod give_me {
let transfer = call.give_me(120);

let call_res = client
.call(&ink_e2e::eve(), &transfer, 0, None)
.call(&ink_e2e::eve(), &transfer, 0, None, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()

.await
.expect("call failed");

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/custom-allocator/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ mod custom_allocator {
// When
let flip = call.flip();
let _flip_result = client
.call(&ink_e2e::bob(), &flip, 0, None)
.call(&ink_e2e::bob(), &flip, 0, None, None)
.await
.expect("flip failed");

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/custom-environment/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ mod runtime_call {
let message = call.trigger();

let call_res = client
.call(&ink_e2e::alice(), &message, 0, None)
.call(&ink_e2e::alice(), &message, 0, None, None)
.await
.expect("call failed");

Expand Down
20 changes: 16 additions & 4 deletions integration-tests/e2e-runtime-only-backend/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ pub mod flipper {
// when
let mut call = contract.call::<Flipper>();
let _flip_res = client
.call(&ink_e2e::bob(), &call.flip(), 0, None)
.call(&ink_e2e::bob(), &call.flip(), 0, None, None)
.await
.expect("flip failed");

// then
let get_res = client
.call(&ink_e2e::bob(), &call.get(), 0, None)
.call(&ink_e2e::bob(), &call.get(), 0, None, None)
.await
.expect("get failed");
assert_eq!(get_res.return_value(), !INITIAL_VALUE);
Expand All @@ -122,7 +122,13 @@ pub mod flipper {
let call = contract.call::<Flipper>();

let old_balance = client
.call(&ink_e2e::alice(), &call.get_contract_balance(), 0, None)
.call(
&ink_e2e::alice(),
&call.get_contract_balance(),
0,
None,
None,
)
.await
.expect("get_contract_balance failed")
.return_value();
Expand All @@ -141,7 +147,13 @@ pub mod flipper {

// then
let new_balance = client
.call(&ink_e2e::alice(), &call.get_contract_balance(), 0, None)
.call(
&ink_e2e::alice(),
&call.get_contract_balance(),
0,
None,
None,
)
.await
.expect("get_contract_balance failed")
.return_value();
Expand Down
10 changes: 5 additions & 5 deletions integration-tests/erc20/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ mod erc20 {
let transfer_to_bob = 500_000_000u128;
let transfer = call.transfer(bob_account, transfer_to_bob);
let _transfer_res = client
.call(&ink_e2e::alice(), &transfer, 0, None)
.call(&ink_e2e::alice(), &transfer, 0, None, None)
.await
.expect("transfer failed");

Expand Down Expand Up @@ -579,7 +579,7 @@ mod erc20 {
// tx
let transfer_from = call.transfer_from(bob_account, charlie_account, amount);
let transfer_from_result = client
.call(&ink_e2e::charlie(), &transfer_from, 0, None)
.call(&ink_e2e::charlie(), &transfer_from, 0, None, None)
.await;

assert!(
Expand All @@ -591,15 +591,15 @@ mod erc20 {
let approved_value = 1_000u128;
let approve_call = call.approve(charlie_account, approved_value);
client
.call(&ink_e2e::bob(), &approve_call, 0, None)
.call(&ink_e2e::bob(), &approve_call, 0, None, None)
.await
.expect("approve failed");

// `transfer_from` the approved amount
let transfer_from =
call.transfer_from(bob_account, charlie_account, approved_value);
let transfer_from_result = client
.call(&ink_e2e::charlie(), &transfer_from, 0, None)
.call(&ink_e2e::charlie(), &transfer_from, 0, None, None)
.await;
assert!(
transfer_from_result.is_ok(),
Expand All @@ -614,7 +614,7 @@ mod erc20 {
// `transfer_from` again, this time exceeding the approved amount
let transfer_from = call.transfer_from(bob_account, charlie_account, 1);
let transfer_from_result = client
.call(&ink_e2e::charlie(), &transfer_from, 0, None)
.call(&ink_e2e::charlie(), &transfer_from, 0, None, None)
.await;
assert!(
transfer_from_result.is_err(),
Expand Down
6 changes: 3 additions & 3 deletions integration-tests/events/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ pub mod events {
// when
let flip = call.flip_with_foreign_event();
let flip_res = client
.call(&ink_e2e::bob(), &flip, 0, None)
.call(&ink_e2e::bob(), &flip, 0, None, None)
.await
.expect("flip failed");

Expand Down Expand Up @@ -265,7 +265,7 @@ pub mod events {
// when
let flip = call.flip_with_inline_event();
let flip_res = client
.call(&ink_e2e::bob(), &flip, 0, None)
.call(&ink_e2e::bob(), &flip, 0, None, None)
.await
.expect("flip failed");

Expand Down Expand Up @@ -305,7 +305,7 @@ pub mod events {
// when
let call = call.emit_32_byte_topic_event(None);
let call_res = client
.call(&ink_e2e::bob(), &call, 0, None)
.call(&ink_e2e::bob(), &call, 0, None, None)
.await
.expect("emit_32_byte_topic_event failed");

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/flipper/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub mod flipper {
// when
let flip = call.flip();
let _flip_res = client
.call(&ink_e2e::bob(), &flip, 0, None)
.call(&ink_e2e::bob(), &flip, 0, None, None)
.await
.expect("flip failed");

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/lang-err-integration-tests/call-builder-delegate/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ mod call_builder {
let selector = ink::selector_bytes!("invalid_selector");
let call = call_builder_call.delegate(code_hash, selector);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Calling `call_builder::delegate` failed");

Expand Down
12 changes: 6 additions & 6 deletions integration-tests/lang-err-integration-tests/call-builder/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ mod call_builder {
let selector = ink::selector_bytes!("invalid_selector");
let call = call_builder_call.call(flipper.account_id, selector);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Calling `call_builder::call` failed");

Expand Down Expand Up @@ -288,7 +288,7 @@ mod call_builder {
let call =
call_builder_call.call_instantiate(code_hash, selector, init_value);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Client failed to call `call_builder::call_instantiate`.")
.return_value();
Expand Down Expand Up @@ -327,7 +327,7 @@ mod call_builder {
let call =
call_builder_call.call_instantiate(code_hash, selector, init_value);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Client failed to call `call_builder::call_instantiate`.")
.return_value();
Expand Down Expand Up @@ -413,7 +413,7 @@ mod call_builder {
let call = call_builder_call
.call_instantiate_fallible(code_hash, selector, init_value);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Calling `call_builder::call_instantiate_fallible` failed")
.return_value();
Expand Down Expand Up @@ -454,7 +454,7 @@ mod call_builder {
let call = call_builder_call
.call_instantiate_fallible(code_hash, selector, init_value);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect("Calling `call_builder::call_instantiate_fallible` failed")
.return_value();
Expand Down Expand Up @@ -547,7 +547,7 @@ mod call_builder {
let call = call_builder_call
.call_instantiate_fallible(code_hash, selector, init_value);
let call_result = client
.call(&origin, &call, 0, None)
.call(&origin, &call, 0, None, None)
.await
.expect(
"Client failed to call `call_builder::call_instantiate_fallible`.",
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/lang-err-integration-tests/contract-ref/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ mod contract_ref {

let flip_check = call.flip_check();
let flip_call_result = client
.call(&ink_e2e::alice(), &flip_check, 0, None)
.call(&ink_e2e::alice(), &flip_check, 0, None, None)
.await
.expect("Calling `flip` failed");
assert!(
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/lang-err-integration-tests/integration-flipper/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ pub mod integration_flipper {

let flip = call.flip();
let flip_call_result = client
.call(&ink_e2e::alice(), &flip, 0, None)
.call(&ink_e2e::alice(), &flip, 0, None, None)
.await
.expect("Calling `flip` failed");
assert!(
Expand Down Expand Up @@ -132,7 +132,7 @@ pub mod integration_flipper {

let err_flip = call.err_flip();
let err_flip_call_result =
client.call(&ink_e2e::bob(), &err_flip, 0, None).await;
client.call(&ink_e2e::bob(), &err_flip, 0, None, None).await;

assert!(matches!(
err_flip_call_result,
Expand Down
Loading