[E2E] Call builders and extra gas margin option #1917
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
Maybe now is the time to finally convert to a builder style API, since the default values 0, None, None are the most common.
There was a problem hiding this comment.
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()
...There was a problem hiding this comment.
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
Clientstore 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)
SkymanOne
changed the title
crates/e2e/src/subxt_client.rs
Outdated
| @@ -527,6 +530,7 @@ where | |||
| caller: &Keypair, | |||
| message: &CallBuilderFinal<E, Args, RetType>, | |||
| value: E::Balance, | |||
| extra_gas_portion: Option<usize>, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Additionally, there is no way of specifying gas limit in E2E anyway, so it will be a breaking change nonetheless
| @@ -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) | |||
There was a problem hiding this comment.
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()
|
I think the best course of action is to have |
|
Converting to draft as |
There was a problem hiding this comment.
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_*andbare_*_dry_runfromContractsBackend. 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_instantiateandbare_instantiate_dry_runinto a crate private trait, likeSubmitteror whatever name you like. This trait would be completely internal, used only for callbacks fromCallBuilder/InstantiateBuilder/UploadBuilderimplementations. 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 bothbare_instantiateandinstantiate_with_gas_margin(and thusBuilderClientis 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.
That's a fair point. Th initial idea behind having I'll see if I can encapsulate this login into the builder API as well |
| fn instantiate<'a, Contract: Clone, Args: Send + Clone + Encode + Sync, R>( | ||
| &'a mut self, | ||
| contract_name: &'a str, | ||
| caller: &'a Keypair, |
There was a problem hiding this comment.
just wondering... maybe alice() should be moved to default as well (similarly to value, storage deposit and margin)?
There was a problem hiding this comment.
IMHO, the caller should be specified explicitly. I don't want to hide this information from the developer per se
| } | ||
|
|
||
| /// Provide value with a call | ||
| pub fn value(&mut self, value: E::Balance) -> &mut Self { |
There was a problem hiding this comment.
why not pub fn value(mut self, value: E::Balance) -> Self?
There was a problem hiding this comment.
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]>
|
@ascjones To address your comments with regard to using
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 |
See #1938 for an approach of how to solve this. |
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑These are the results when building the
Link to the run | Last update: Wed Oct 18 13:45:51 CEST 2023 |
Summary
cargo-contractorpallet-contracts? -ink! 5.0.0-alphaThis 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,callanduploadwill 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 withdry_run().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
Environmenttrait now extendsClonein order to makeCreateBuildercloneable. There are other structs that addClonetrait bound as well.Extract from flipper:
Checklist before requesting a review
CHANGELOG.md@pmikolajczyk41 Pinging for review as well