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.

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

Clean up CallBuilder return() type #1525

Merged
merged 23 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
24 changes: 16 additions & 8 deletions crates/e2e/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use super::{
};
use contract_metadata::ContractMetadata;
use ink_env::Environment;
use ink_primitives::MessageResult;

use sp_runtime::traits::{
IdentifyAccount,
Expand Down Expand Up @@ -124,7 +125,7 @@ pub struct CallResult<C: subxt::Config, E: Environment, V> {
pub events: ExtrinsicEvents<C>,
/// Contains the result of decoding the return value of the called
/// function.
pub value: Result<V, scale::Error>,
pub value: Result<MessageResult<V>, scale::Error>,
/// Returns the bytes of the encoded dry-run return value.
pub data: Vec<u8>,
}
Expand All @@ -139,12 +140,19 @@ where
/// Panics if the value could not be decoded. The raw bytes can be accessed
/// via [`return_data`].
pub fn return_value(self) -> V {
self.value.unwrap_or_else(|err| {
panic!(
"decoding dry run result to ink! message return type failed: {}",
err
)
})
self.value
.unwrap_or_else(|env_err| {
panic!(
"Decoding dry run result to ink! message return type failed: {}",
env_err
)
})
.unwrap_or_else(|lang_err| {
panic!(
"Encountered a `LangError` while decoding dry run result to ink! message: {:?}",
lang_err
)
})
}

/// Returns true if the specified event was triggered by the call.
Expand Down Expand Up @@ -655,7 +663,7 @@ where
}

let bytes = &dry_run.result.as_ref().unwrap().data;
let value: Result<RetType, scale::Error> =
let value: Result<MessageResult<RetType>, scale::Error> =
scale::Decode::decode(&mut bytes.as_ref());

Ok(CallResult {
Expand Down
4 changes: 3 additions & 1 deletion crates/env/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,9 @@ where
/// - If the called contract execution has trapped.
/// - If the called contract ran out of gas upon execution.
/// - If the returned value failed to decode properly.
pub fn invoke_contract<E, Args, R>(params: &CallParams<E, Call<E>, Args, R>) -> Result<R>
pub fn invoke_contract<E, Args, R>(
params: &CallParams<E, Call<E>, Args, R>,
) -> Result<ink_primitives::MessageResult<R>>
Comment on lines -271 to +273
Copy link
Contributor

Choose a reason for hiding this comment

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

I know not related to this PR but do we really need these standalone functions? I don't think we want multiple ways to do the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how much use external developers are making of these specific APIs, but it's
probably important to leave them in. This at least gives developers a chance to optimize
their contracts, and third-party tooling to improve upon our abstractions.

A counter-argument here might be that this API is already opinionated (e.g the use of
CallParams) and doesn't give developers enough room to optimize or improve upon it. In
this case, maybe we want to expose even lower level APIs, but I'm not sure we want to go
down that route

Copy link
Contributor

@athei athei Jan 19, 2023

Choose a reason for hiding this comment

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

I can't see how those stand alone function are more low level. Both operate on the same type CallParams. The difference is just inherent vs free function. This is just a mechanical change.

where
E: Environment,
Args: scale::Encode,
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ pub trait TypedEnvBackend: EnvBackend {
fn invoke_contract<E, Args, R>(
&mut self,
call_data: &CallParams<E, Call<E>, Args, R>,
) -> Result<R>
) -> Result<ink_primitives::MessageResult<R>>
where
E: Environment,
Args: scale::Encode,
Expand Down
95 changes: 92 additions & 3 deletions crates/env/src/call/call_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,28 @@ where
/// Invokes the contract with the given built-up call parameters.
///
/// Returns the result of the contract execution.
///
/// # Panics
///
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle
/// those use the [`try_invoke`][`CallParams::try_invoke`] method instead.
pub fn invoke(&self) -> Result<R, crate::Error> {
crate::invoke_contract(self).map(|inner| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should take the chance to also unwrap the error from pallet-contracts while we are breaking things. Let's just make the default path as safe and frictionless as possible.

Copy link
Contributor Author

@HCastano HCastano Jan 18, 2023

Choose a reason for hiding this comment

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

Yeah good idea. I'll do this in a small follow-up so that the CreateBuilder methods can also be included in the clean-up.

Edit: done in #1602

inner.expect(
"Handling `LangError`s is not supported by `CallParams::invoke`, use \
`CallParams::try_invoke` instead.",
)
athei marked this conversation as resolved.
Show resolved Hide resolved
athei marked this conversation as resolved.
Show resolved Hide resolved
})
}

/// Invokes the contract with the given built-up call parameters.
///
/// Returns the result of the contract execution.
///
/// # Note
///
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller.
pub fn try_invoke(&self) -> Result<ink_primitives::MessageResult<R>, crate::Error> {
crate::invoke_contract(self)
}
}
Expand Down Expand Up @@ -173,7 +194,7 @@ where
/// )
/// .returns::<()>()
/// .fire()
/// .unwrap();
/// .expect("Got an error from the Contract's pallet.");
/// ```
///
/// ## Example 2: With Return Value
Expand Down Expand Up @@ -209,7 +230,7 @@ where
/// )
/// .returns::<i32>()
/// .fire()
/// .unwrap();
/// .expect("Got an error from the Contract's pallet.");
/// ```
///
/// ## Example 3: Delegate call
Expand Down Expand Up @@ -237,7 +258,47 @@ where
/// )
/// .returns::<i32>()
/// .fire()
/// .unwrap();
/// .expect("Got an error from the Contract's pallet.");
/// ```
///
/// # Handling `LangError`s
///
/// It is also important to note that there are certain types of errors which can happen during
/// cross-contract calls which can be handled know as [`LangError`][`ink_primitives::LangError`].
///
/// If you want to handle these errors use the [`CallBuilder::try_fire`] methods instead of the
/// [`CallBuilder::fire`] ones.
///
/// **Note:** The shown examples panic because there is currently no cross-calling
/// support in the off-chain testing environment. However, this code
/// should work fine in on-chain environments.
///
/// ## Example: Handling a `LangError`
///
/// ```should_panic
/// # use ::ink_env::{
/// # Environment,
/// # DefaultEnvironment,
/// # call::{build_call, Selector, ExecutionInput}
/// # };
/// # use ink_env::call::Call;
/// # type AccountId = <DefaultEnvironment as Environment>::AccountId;
/// # type Balance = <DefaultEnvironment as Environment>::Balance;
/// let call_result = build_call::<DefaultEnvironment>()
/// .call_type(
/// Call::new()
/// .callee(AccountId::from([0x42; 32]))
/// .gas_limit(5000)
/// .transferred_value(10),
/// )
/// .try_fire()
/// .expect("Got an error from the Contract's pallet.");
///
/// match call_result {
/// Ok(_) => unimplemented!(),
/// Err(e @ ink_primitives::LangError::CouldNotReadInput) => unimplemented!(),
/// Err(_) => unimplemented!(),
/// }
/// ```
#[allow(clippy::type_complexity)]
pub fn build_call<E>() -> CallBuilder<
Expand Down Expand Up @@ -597,9 +658,23 @@ where
E: Environment,
{
/// Invokes the cross-chain function call.
///
/// # Panics
///
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle
/// those use the [`try_fire`][`CallBuilder::try_fire`] method instead.
pub fn fire(self) -> Result<(), Error> {
self.params().invoke()
}

/// Invokes the cross-chain function call.
///
/// # Note
///
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more accurate to clarify that the inner Result will be a LangError, and the outer Result is an environmental error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll fix it in the follow-up that removes the EnvError from the default execution path

pub fn try_fire(self) -> Result<ink_primitives::MessageResult<()>, Error> {
self.params().try_invoke()
}
}

impl<E>
Expand All @@ -626,9 +701,23 @@ where
R: scale::Decode,
{
/// Invokes the cross-chain function call and returns the result.
///
/// # Panics
///
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle
/// those use the [`try_fire`][`CallBuilder::try_fire`] method instead.
pub fn fire(self) -> Result<R, Error> {
self.params().invoke()
}

/// Invokes the cross-chain function call and returns the result.
///
/// # Note
///
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller.
pub fn try_fire(self) -> Result<ink_primitives::MessageResult<R>, Error> {
self.params().try_invoke()
}
}

impl<E, Args, R>
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/engine/off_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ impl TypedEnvBackend for EnvInstance {
fn invoke_contract<E, Args, R>(
&mut self,
params: &CallParams<E, Call<E>, Args, R>,
) -> Result<R>
) -> Result<ink_primitives::MessageResult<R>>
where
E: Environment,
Args: scale::Encode,
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/engine/on_chain/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ impl TypedEnvBackend for EnvInstance {
fn invoke_contract<E, Args, R>(
&mut self,
params: &CallParams<E, Call<E>, Args, R>,
) -> Result<R>
) -> Result<ink_primitives::MessageResult<R>>
where
E: Environment,
Args: scale::Encode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,10 @@ impl CallBuilder<'_> {
let input_types = generator::input_types(message.inputs());
let arg_list = generator::generate_argument_list(input_types.iter().cloned());
let mut_tok = callable.receiver().is_ref_mut().then(|| quote! { mut });
let return_type = message.wrapped_output();
let return_type = message
.output()
.map(quote::ToTokens::to_token_stream)
.unwrap_or_else(|| quote::quote! { () });
let output_span = return_type.span();
let output_type = quote_spanned!(output_span=>
::ink::env::call::CallBuilder<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ impl ContractRef<'_> {
) -> #wrapped_output_type {
<Self as ::ink::codegen::TraitCallBuilder>::#call_operator(self)
.#message_ident( #( #input_bindings ),* )
.fire()
.try_fire()
.unwrap_or_else(|error| ::core::panic!(
"encountered error while calling {}::{}: {:?}",
::core::stringify!(#storage_ident),
Expand Down
7 changes: 5 additions & 2 deletions crates/ink/src/env_access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,10 @@ where
/// )
/// .returns::<i32>()
/// .params();
/// self.env().invoke_contract(&call_params).unwrap_or_else(|err| panic!("call invocation must succeed: {:?}", err))
///
/// self.env().invoke_contract(&call_params)
/// .unwrap_or_else(|env_err| panic!("Received an error from the Environment: {:?}", env_err))
/// .unwrap_or_else(|lang_err| panic!("Received a `LangError`: {:?}", lang_err))
/// }
/// #
/// # }
Expand All @@ -532,7 +535,7 @@ where
pub fn invoke_contract<Args, R>(
self,
params: &CallParams<E, Call<E>, Args, R>,
) -> Result<R>
) -> Result<ink_primitives::MessageResult<R>>
where
Args: scale::Encode,
R: scale::Decode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ note: required by a bound in `ExecutionInput::<ArgumentList<ArgumentListEnd, Arg
| T: scale::Encode,
| ^^^^^^^^^^^^^ required by this bound in `ExecutionInput::<ArgumentList<ArgumentListEnd, ArgumentListEnd>>::push_arg`

error[E0599]: the method `fire` exists for struct `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ink::ink_env::call::utils::Argument<NonCodecType>, ArgumentList<ArgumentListEnd, ArgumentListEnd>>>>, Set<ReturnType<Result<(), LangError>>>>`, but its trait bounds were not satisfied
error[E0599]: the method `try_fire` exists for struct `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ink::ink_env::call::utils::Argument<NonCodecType>, ArgumentList<ArgumentListEnd, ArgumentListEnd>>>>, Set<ReturnType<()>>>`, but its trait bounds were not satisfied
--> tests/ui/contract/fail/message-input-non-codec.rs:16:9
|
16 | pub fn message(&self, _input: NonCodecType) {}
| ^^^ method cannot be called on `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ink::ink_env::call::utils::Argument<NonCodecType>, ArgumentList<ArgumentListEnd, ArgumentListEnd>>>>, Set<ReturnType<Result<(), LangError>>>>` due to unsatisfied trait bounds
| ^^^ method cannot be called on `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ink::ink_env::call::utils::Argument<NonCodecType>, ArgumentList<ArgumentListEnd, ArgumentListEnd>>>>, Set<ReturnType<()>>>` due to unsatisfied trait bounds
|
::: $WORKSPACE/crates/env/src/call/execution_input.rs
|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,19 @@ note: required by a bound in `return_value`
| R: scale::Encode,
| ^^^^^^^^^^^^^ required by this bound in `return_value`

error[E0599]: the method `fire` exists for struct `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<Result<NonCodecType, LangError>>>>`, but its trait bounds were not satisfied
error[E0599]: the method `try_fire` exists for struct `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<NonCodecType>>>`, but its trait bounds were not satisfied
--> tests/ui/contract/fail/message-returns-non-codec.rs:16:9
|
4 | pub struct NonCodecType;
| ----------------------- doesn't satisfy `NonCodecType: parity_scale_codec::Decode`
...
16 | pub fn message(&self) -> NonCodecType {
| ^^^ method cannot be called on `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<Result<NonCodecType, LangError>>>>` due to unsatisfied trait bounds
|
::: $RUST/core/src/result.rs
|
| pub enum Result<T, E> {
| --------------------- doesn't satisfy `_: parity_scale_codec::Decode`
| ^^^ method cannot be called on `ink::ink_env::call::CallBuilder<DefaultEnvironment, Set<Call<DefaultEnvironment>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<NonCodecType>>>` due to unsatisfied trait bounds
|
= note: the following trait bounds were not satisfied:
`Result<NonCodecType, LangError>: parity_scale_codec::Decode`
`NonCodecType: parity_scale_codec::Decode`
note: the following trait must be implemented
--> $CARGO/parity-scale-codec-3.2.1/src/codec.rs
|
| pub trait Decode: Sized {
| ^^^^^^^^^^^^^^^^^^^^^^^
9 changes: 3 additions & 6 deletions examples/delegator/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,7 @@ mod delegator {
.call(&ink_e2e::bob(), get, 0, None)
.await
.expect("calling `get` failed")
.return_value()
.expect("calling `get` returned a `LangError`");
.return_value();
assert_eq!(value, 1234);
let change = build_message::<DelegatorRef>(delegator_acc_id.clone())
.call(|contract| contract.change(6));
Expand All @@ -191,8 +190,7 @@ mod delegator {
.call(&ink_e2e::bob(), get, 0, None)
.await
.expect("calling `get` failed")
.return_value()
.expect("calling `get` returned a `LangError`");
.return_value();
assert_eq!(value, 1234 + 6);

// when
Expand All @@ -216,8 +214,7 @@ mod delegator {
.call(&ink_e2e::bob(), get, 0, None)
.await
.expect("calling `get` failed")
.return_value()
.expect("calling `get` returned a `LangError`");
.return_value();
assert_eq!(value, 1234 + 6 - 3);

Ok(())
Expand Down
7 changes: 4 additions & 3 deletions examples/erc1155/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ mod erc1155 {

// If our recipient is a smart contract we need to see if they accept or
// reject this transfer. If they reject it we need to revert the call.
let params = build_call::<Environment>()
let result = build_call::<Environment>()
.call_type(Call::new().callee(to).gas_limit(5000))
.exec_input(
ExecutionInput::new(Selector::new(ON_ERC_1155_RECEIVED_SELECTOR))
Expand All @@ -376,9 +376,10 @@ mod erc1155 {
.push_arg(data),
)
.returns::<Vec<u8>>()
.params();
.params()
.invoke();

match ink::env::invoke_contract(&params) {
match result {
Ok(v) => {
ink::env::debug_println!(
"Received return value \"{:?}\" from contract {:?}",
Expand Down
10 changes: 3 additions & 7 deletions examples/erc20/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,14 +577,10 @@ mod erc20 {
// then
assert_eq!(
total_supply,
total_supply_res.return_value().unwrap(),
total_supply_res.return_value(),
"total_supply"
);
assert_eq!(
transfer_to_bob,
balance_of_res.return_value().unwrap(),
"balance_of"
);
assert_eq!(transfer_to_bob, balance_of_res.return_value(), "balance_of");

Ok(())
}
Expand Down Expand Up @@ -671,7 +667,7 @@ mod erc20 {

assert_eq!(
total_supply - approved_value,
balance_of_res.return_value().unwrap(),
balance_of_res.return_value(),
"balance_of"
);

Expand Down
Loading