From a83c937d41d16125260afdf5e72bdb8a00a053bf Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 20 Jan 2023 06:32:16 -0800 Subject: [PATCH] Make `CallBuilder` and `CreateBuilder` error handling optional (#1602) * Make default behaviour of `fire()` to return "raw" return value * Update delegate call codepath to give option to handle `EnvError` * Update `CreateBuilder::instantiate()`'s default cleaner as well * Add a breaking change note in the changelog * Fix delegator example * Drive-by: rename `fire()` method to `invoke()` This keeps this more consistent since we're wrapping a few methods with `invoke` in the name anyways. * Fix ERC-1155 example * Update `Multisig` example * Fix ContractRef tests * Revert "Drive-by: rename `fire()` method to `invoke()`" This reverts commit 72add05d0c6dcc703502e11c7e862a0a03222eac. Decided it's best to do this in a standalone PR. * Use instantiate instead of `try_instantiate` in `delegator` Co-authored-by: Andrew Jones --- CHANGELOG.md | 8 ++ crates/env/src/call/call_builder.rs | 107 +++++++++++++----- crates/env/src/call/create_builder.rs | 45 +++++++- .../src/generator/trait_def/call_forwarder.rs | 5 +- .../fail/message_input_non_codec.stderr | 100 ++++++++-------- .../fail/message_output_non_codec.stderr | 2 +- examples/delegator/lib.rs | 18 +-- examples/erc1155/lib.rs | 8 +- .../call-builder/lib.rs | 3 +- .../contract-ref/lib.rs | 5 +- examples/multisig/lib.rs | 24 ++-- .../forward-calls/lib.rs | 12 +- 12 files changed, 216 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 754fada5fe..e447c852d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - Add E2E testing framework MVP ‒ [#1395](https://github.com/paritytech/ink/pull/1395) - Add E2E tests for `Mapping` functions - [#1492](https://github.com/paritytech/ink/pull/1492) +- Make CallBuilder and CreateBuilder error handling optional - [#1602](https://github.com/paritytech/ink/pull/1602) + +### Breaking Changes +With the addition of [#1602](https://github.com/paritytech/ink/pull/1602), +the `CallBuilder::fire()`, `CallParams::invoke()`, and `CreateBuilder::instantiate()` +methods now unwrap the `Result` from `pallet-contracts` under the hood. + +If you wish to handle the error use the new `try_` variants of those methods instead. ## Version 4.0.0-beta diff --git a/crates/env/src/call/call_builder.rs b/crates/env/src/call/call_builder.rs index 3c208c5cdd..e88ec37df1 100644 --- a/crates/env/src/call/call_builder.rs +++ b/crates/env/src/call/call_builder.rs @@ -112,14 +112,17 @@ where /// /// # 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 { - crate::invoke_contract(self).map(|inner| { - inner.unwrap_or_else(|lang_error| { + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] or an + /// [`ink::primitives::LangError`][`ink_primitives::LangError`]. If you want to handle those + /// use the [`try_invoke`][`CallParams::try_invoke`] method instead. + pub fn invoke(&self) -> R { + crate::invoke_contract(self) + .unwrap_or_else(|env_error| { + panic!("Cross-contract call failed with {:?}", env_error) + }) + .unwrap_or_else(|lang_error| { panic!("Cross-contract call failed with {:?}", lang_error) }) - }) } /// Invokes the contract with the given built-up call parameters. @@ -128,7 +131,8 @@ where /// /// # Note /// - /// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. + /// On failure this returns an outer [`ink::env::Error`][`crate::Error`] or inner + /// [`ink_primitives::LangError`], both of which can be handled by the caller. pub fn try_invoke(&self) -> Result, crate::Error> { crate::invoke_contract(self) } @@ -140,11 +144,29 @@ where Args: scale::Encode, R: scale::Decode, { - /// Invokes the contract via delegated call with the given - /// built-up call parameters. + /// Invoke the contract using Delegate Call semantics with the given built-up call parameters. /// /// Returns the result of the contract execution. - pub fn invoke(&self) -> Result { + /// + /// # Panics + /// + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] If you want to + /// handle those use the [`try_invoke`][`CallParams::try_invoke`] method instead. + pub fn invoke(&self) -> R { + crate::invoke_contract_delegate(self).unwrap_or_else(|env_error| { + panic!("Cross-contract call failed with {:?}", env_error) + }) + } + + /// Invoke the contract using Delegate Call semantics with the given built-up call parameters. + /// + /// Returns the result of the contract execution. + /// + /// # Note + /// + /// On failure this returns an [`ink::env::Error`][`crate::Error`] which can be handled by the + /// caller. + pub fn try_invoke(&self) -> Result { crate::invoke_contract_delegate(self) } } @@ -192,8 +214,7 @@ where /// .push_arg(&[0x10u8; 32]) /// ) /// .returns::<()>() -/// .fire() -/// .expect("Got an error from the Contract's pallet."); +/// .fire(); /// ``` /// /// ## Example 2: With Return Value @@ -228,8 +249,7 @@ where /// .push_arg(&[0x10u8; 32]) /// ) /// .returns::() -/// .fire() -/// .expect("Got an error from the Contract's pallet."); +/// .fire(); /// ``` /// /// ## Example 3: Delegate call @@ -256,8 +276,7 @@ where /// .push_arg(&[0x10u8; 32]) /// ) /// .returns::() -/// .fire() -/// .expect("Got an error from the Contract's pallet."); +/// .fire(); /// ``` /// /// # Handling `LangError`s @@ -660,9 +679,10 @@ where /// /// # 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> { + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] or an + /// [`ink::primitives::LangError`][`ink_primitives::LangError`]. If you want to handle those + /// use the [`try_fire`][`CallBuilder::try_fire`] method instead. + pub fn fire(self) { self.params().invoke() } @@ -670,7 +690,8 @@ where /// /// # Note /// - /// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. + /// On failure this returns an outer [`ink::env::Error`][`crate::Error`] or inner + /// [`ink_primitives::LangError`], both of which can be handled by the caller. pub fn try_fire(self) -> Result, Error> { self.params().try_invoke() } @@ -686,10 +707,24 @@ impl where E: Environment, { - /// Invokes the cross-chain function call. - pub fn fire(self) -> Result<(), Error> { + /// Invokes the cross-chain function call using Delegate Call semantics. + /// + /// # Panics + /// + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] + /// If you want to handle those use the [`try_fire`][`CallBuilder::try_fire`] method instead. + pub fn fire(self) { self.params().invoke() } + + /// Invokes the cross-chain function call using Delegate Call semantics. + /// + /// # Note + /// + /// On failure this an [`ink::env::Error`][`crate::Error`] which can be handled by the caller. + pub fn try_fire(self) -> Result<(), Error> { + self.params().try_invoke() + } } impl @@ -703,9 +738,10 @@ where /// /// # 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 { + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] or an + /// [`ink::primitives::LangError`][`ink_primitives::LangError`]. If you want to handle those + /// use the [`try_fire`][`CallBuilder::try_fire`] method instead. + pub fn fire(self) -> R { self.params().invoke() } @@ -713,7 +749,8 @@ where /// /// # Note /// - /// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. + /// On failure this returns an outer [`ink::env::Error`][`crate::Error`] or inner + /// [`ink_primitives::LangError`], both of which can be handled by the caller. pub fn try_fire(self) -> Result, Error> { self.params().try_invoke() } @@ -726,8 +763,22 @@ where Args: scale::Encode, R: scale::Decode, { - /// Invokes the cross-chain function call and returns the result. - pub fn fire(self) -> Result { + /// Invokes the cross-chain function call using Delegate Call semantics and returns the result. + /// + /// # Panics + /// + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`] + /// If you want to handle those use the [`try_fire`][`CallBuilder::try_fire`] method instead. + pub fn fire(self) -> R { self.params().invoke() } + + /// Invokes the cross-chain function call using Delegate Call semantics and returns the result. + /// + /// # Note + /// + /// On failure this an [`ink::env::Error`][`crate::Error`] which can be handled by the caller. + pub fn try_fire(self) -> Result { + self.params().try_invoke() + } } diff --git a/crates/env/src/call/create_builder.rs b/crates/env/src/call/create_builder.rs index c065785837..527ede79dc 100644 --- a/crates/env/src/call/create_builder.rs +++ b/crates/env/src/call/create_builder.rs @@ -119,8 +119,28 @@ where R: FromAccountId, { /// Instantiates the contract and returns its account ID back to the caller. + /// + /// # Panics + /// + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`]. If you want to + /// handle those use the [`try_instantiate`][`CreateParams::try_instantiate`] method instead. #[inline] - pub fn instantiate(&self) -> Result { + pub fn instantiate(&self) -> R { + crate::instantiate_contract(self) + .map(FromAccountId::from_account_id) + .unwrap_or_else(|env_error| { + panic!("Cross-contract instantiation failed with {:?}", env_error) + }) + } + + /// Instantiates the contract and returns its account ID back to the caller. + /// + /// # Note + /// + /// On failure this returns an [`ink::env::Error`][`crate::Error`] which can be handled by the + /// caller. + #[inline] + pub fn try_instantiate(&self) -> Result { crate::instantiate_contract(self).map(FromAccountId::from_account_id) } } @@ -180,8 +200,7 @@ where /// ) /// .salt_bytes(&[0xDE, 0xAD, 0xBE, 0xEF]) /// .params() -/// .instantiate() -/// .unwrap(); +/// .instantiate(); /// ``` /// /// **Note:** The shown example panics because there is currently no cross-calling @@ -384,9 +403,25 @@ where Salt: AsRef<[u8]>, R: FromAccountId, { - /// Instantiates the contract using the given instantiation parameters. + /// Instantiates the contract and returns its account ID back to the caller. + /// + /// # Panics + /// + /// This method panics if it encounters an [`ink::env::Error`][`crate::Error`]. If you want to + /// handle those use the [`try_instantiate`][`CreateBuilder::try_instantiate`] method instead. #[inline] - pub fn instantiate(self) -> Result { + pub fn instantiate(self) -> R { self.params().instantiate() } + + /// Instantiates the contract and returns its account ID back to the caller. + /// + /// # Note + /// + /// On failure this returns an [`ink::env::Error`][`crate::Error`] which can be handled by the + /// caller. + #[inline] + pub fn try_instantiate(self) -> Result { + self.params().try_instantiate() + } } diff --git a/crates/ink/codegen/src/generator/trait_def/call_forwarder.rs b/crates/ink/codegen/src/generator/trait_def/call_forwarder.rs index d2828712aa..46017c5dff 100644 --- a/crates/ink/codegen/src/generator/trait_def/call_forwarder.rs +++ b/crates/ink/codegen/src/generator/trait_def/call_forwarder.rs @@ -355,8 +355,9 @@ impl CallForwarder<'_> { , #input_bindings )* ) - .fire() - .unwrap_or_else(|err| ::core::panic!("{}: {:?}", #panic_str, err)) + .try_fire() + .unwrap_or_else(|env_err| ::core::panic!("{}: {:?}", #panic_str, env_err)) + .unwrap_or_else(|lang_err| ::core::panic!("{}: {:?}", #panic_str, lang_err)) } ) } diff --git a/crates/ink/tests/ui/trait_def/fail/message_input_non_codec.stderr b/crates/ink/tests/ui/trait_def/fail/message_input_non_codec.stderr index 63b1a30a63..a4a7905251 100644 --- a/crates/ink/tests/ui/trait_def/fail/message_input_non_codec.stderr +++ b/crates/ink/tests/ui/trait_def/fail/message_input_non_codec.stderr @@ -1,56 +1,56 @@ error[E0277]: the trait bound `NonCodec: WrapperTypeDecode` is not satisfied - --> tests/ui/trait_def/fail/message_input_non_codec.rs:6:23 - | -6 | fn message(&self, input: NonCodec); - | ^^^^^ the trait `WrapperTypeDecode` is not implemented for `NonCodec` - | - = help: the following other types implement trait `WrapperTypeDecode`: - Arc - Box - Rc - = note: required for `NonCodec` to implement `parity_scale_codec::Decode` + --> tests/ui/trait_def/fail/message_input_non_codec.rs:6:23 + | +6 | fn message(&self, input: NonCodec); + | ^^^^^ the trait `WrapperTypeDecode` is not implemented for `NonCodec` + | + = help: the following other types implement trait `WrapperTypeDecode`: + Arc + Box + Rc + = note: required for `NonCodec` to implement `parity_scale_codec::Decode` note: required by a bound in `DispatchInput` - --> src/codegen/dispatch/type_check.rs - | - | T: scale::Decode + 'static; - | ^^^^^^^^^^^^^ required by this bound in `DispatchInput` + --> src/codegen/dispatch/type_check.rs + | + | T: scale::Decode + 'static; + | ^^^^^^^^^^^^^ required by this bound in `DispatchInput` error[E0277]: the trait bound `NonCodec: WrapperTypeEncode` is not satisfied - --> tests/ui/trait_def/fail/message_input_non_codec.rs:3:1 - | -3 | #[ink::trait_definition] - | ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `WrapperTypeEncode` is not implemented for `NonCodec` -4 | pub trait TraitDefinition { -5 | #[ink(message)] - | - required by a bound introduced by this call - | - = help: the following other types implement trait `WrapperTypeEncode`: - &T - &mut T - Arc - Box - Cow<'a, T> - Rc - String - Vec - parity_scale_codec::Ref<'a, T, U> - = note: required for `NonCodec` to implement `Encode` + --> tests/ui/trait_def/fail/message_input_non_codec.rs:3:1 + | +3 | #[ink::trait_definition] + | ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `WrapperTypeEncode` is not implemented for `NonCodec` +4 | pub trait TraitDefinition { +5 | #[ink(message)] + | - required by a bound introduced by this call + | + = help: the following other types implement trait `WrapperTypeEncode`: + &T + &mut T + Arc + Box + Cow<'a, T> + Rc + String + Vec + parity_scale_codec::Ref<'a, T, U> + = note: required for `NonCodec` to implement `Encode` note: required by a bound in `ExecutionInput::>::push_arg` - --> $WORKSPACE/crates/env/src/call/execution_input.rs - | - | T: scale::Encode, - | ^^^^^^^^^^^^^ required by this bound in `ExecutionInput::>::push_arg` + --> $WORKSPACE/crates/env/src/call/execution_input.rs + | + | T: scale::Encode, + | ^^^^^^^^^^^^^ required by this bound in `ExecutionInput::>::push_arg` -error[E0599]: the method `fire` exists for struct `CallBuilder>, Set, ArgumentList>>>, Set>>`, but its trait bounds were not satisfied - --> tests/ui/trait_def/fail/message_input_non_codec.rs:5:5 - | -5 | #[ink(message)] - | ^ method cannot be called on `CallBuilder>, Set, ArgumentList>>>, Set>>` due to unsatisfied trait bounds - | - ::: $WORKSPACE/crates/env/src/call/execution_input.rs - | - | pub struct ArgumentList { - | ----------------------------------- doesn't satisfy `_: Encode` - | - = note: the following trait bounds were not satisfied: - `ArgumentList, ArgumentList>: Encode` +error[E0599]: the method `try_fire` exists for struct `CallBuilder>, Set, ArgumentList>>>, Set>>`, but its trait bounds were not satisfied + --> tests/ui/trait_def/fail/message_input_non_codec.rs:5:5 + | +5 | #[ink(message)] + | ^ method cannot be called on `CallBuilder>, Set, ArgumentList>>>, Set>>` due to unsatisfied trait bounds + | + ::: $WORKSPACE/crates/env/src/call/execution_input.rs + | + | pub struct ArgumentList { + | ----------------------------------- doesn't satisfy `_: Encode` + | + = note: the following trait bounds were not satisfied: + `ArgumentList, ArgumentList>: Encode` diff --git a/crates/ink/tests/ui/trait_def/fail/message_output_non_codec.stderr b/crates/ink/tests/ui/trait_def/fail/message_output_non_codec.stderr index 2c487dcca6..2cbf788308 100644 --- a/crates/ink/tests/ui/trait_def/fail/message_output_non_codec.stderr +++ b/crates/ink/tests/ui/trait_def/fail/message_output_non_codec.stderr @@ -21,7 +21,7 @@ note: required by a bound in `DispatchOutput` | T: scale::Encode + 'static; | ^^^^^^^^^^^^^ required by this bound in `DispatchOutput` -error[E0599]: the method `fire` exists for struct `CallBuilder>, Set>>, Set>>`, but its trait bounds were not satisfied +error[E0599]: the method `try_fire` exists for struct `CallBuilder>, Set>>, Set>>`, but its trait bounds were not satisfied --> tests/ui/trait_def/fail/message_output_non_codec.rs:5:5 | 1 | pub struct NonCodec; diff --git a/examples/delegator/lib.rs b/examples/delegator/lib.rs index 074ac5cae7..69f5c5ab35 100644 --- a/examples/delegator/lib.rs +++ b/examples/delegator/lib.rs @@ -63,29 +63,17 @@ mod delegator { .endowment(total_balance / 4) .code_hash(accumulator_code_hash) .salt_bytes(salt) - .instantiate() - .unwrap_or_else(|error| { - panic!( - "failed at instantiating the Accumulator contract: {:?}", - error - ) - }); + .instantiate(); let adder = AdderRef::new(accumulator.clone()) .endowment(total_balance / 4) .code_hash(adder_code_hash) .salt_bytes(salt) - .instantiate() - .unwrap_or_else(|error| { - panic!("failed at instantiating the Adder contract: {:?}", error) - }); + .instantiate(); let subber = SubberRef::new(accumulator.clone()) .endowment(total_balance / 4) .code_hash(subber_code_hash) .salt_bytes(salt) - .instantiate() - .unwrap_or_else(|error| { - panic!("failed at instantiating the Subber contract: {:?}", error) - }); + .instantiate(); Self { which: Which::Adder, accumulator, diff --git a/examples/erc1155/lib.rs b/examples/erc1155/lib.rs index 55f86b16f5..0f86d98d48 100644 --- a/examples/erc1155/lib.rs +++ b/examples/erc1155/lib.rs @@ -377,17 +377,19 @@ mod erc1155 { ) .returns::>() .params() - .invoke(); + .try_invoke(); match result { Ok(v) => { ink::env::debug_println!( "Received return value \"{:?}\" from contract {:?}", - v, + v.clone().expect( + "Call should be valid, don't expect a `LangError`." + ), from ); assert_eq!( - v, + v.clone().expect("Call should be valid, don't expect a `LangError`."), &ON_ERC_1155_RECEIVED_SELECTOR[..], "The recipient contract at {:?} does not accept token transfers.\n Expected: {:?}, Got {:?}", to, ON_ERC_1155_RECEIVED_SELECTOR, v diff --git a/examples/lang-err-integration-tests/call-builder/lib.rs b/examples/lang-err-integration-tests/call-builder/lib.rs index 0a3de1e94e..454ecdbe82 100755 --- a/examples/lang-err-integration-tests/call-builder/lib.rs +++ b/examples/lang-err-integration-tests/call-builder/lib.rs @@ -72,7 +72,6 @@ mod call_builder { .exec_input(ExecutionInput::new(Selector::new(selector))) .returns::<()>() .fire() - .expect("Error from the Contracts pallet.") } #[ink(message)] @@ -94,7 +93,7 @@ mod call_builder { .exec_input(ExecutionInput::new(Selector::new(selector)).push_arg(init_value)) .salt_bytes(&[0xDE, 0xAD, 0xBE, 0xEF]) .params() - .instantiate(); + .try_instantiate(); // NOTE: Right now we can't handle any `LangError` from `instantiate`, we can only tell // that our contract reverted (i.e we see error from the Contracts pallet). diff --git a/examples/lang-err-integration-tests/contract-ref/lib.rs b/examples/lang-err-integration-tests/contract-ref/lib.rs index 64074d14fa..bd6e6108e1 100755 --- a/examples/lang-err-integration-tests/contract-ref/lib.rs +++ b/examples/lang-err-integration-tests/contract-ref/lib.rs @@ -17,10 +17,7 @@ mod contract_ref { .endowment(0) .code_hash(flipper_code_hash) .salt_bytes(salt) - .instantiate() - .unwrap_or_else(|error| { - panic!("failed at instantiating the Flipper contract: {:?}", error) - }); + .instantiate(); Self { flipper } } diff --git a/examples/multisig/lib.rs b/examples/multisig/lib.rs index 16a3543052..42fa16593d 100755 --- a/examples/multisig/lib.rs +++ b/examples/multisig/lib.rs @@ -348,8 +348,7 @@ mod multisig { /// .push_arg(&transaction_candidate) /// ) /// .returns::<(u32, ConfirmationStatus)>() - /// .fire() - /// .expect("submit_transaction won't panic"); + /// .fire(); /// /// // Wait until all required owners have confirmed and then execute the transaction /// // @@ -362,8 +361,7 @@ mod multisig { /// .push_arg(&id) /// ) /// .returns::<()>() - /// .fire() - /// .expect("invoke_transaction won't panic"); + /// .fire(); /// ``` #[ink(message)] pub fn add_owner(&mut self, new_owner: AccountId) { @@ -549,8 +547,13 @@ mod multisig { ExecutionInput::new(t.selector.into()).push_arg(CallInput(&t.input)), ) .returns::<()>() - .fire() - .map_err(|_| Error::TransactionFailed); + .try_fire(); + + let result = match result { + Ok(Ok(_)) => Ok(()), + _ => Err(Error::TransactionFailed), + }; + self.env().emit_event(Execution { transaction: trans_id, result: result.map(|_| None), @@ -582,8 +585,13 @@ mod multisig { ExecutionInput::new(t.selector.into()).push_arg(CallInput(&t.input)), ) .returns::>() - .fire() - .map_err(|_| Error::TransactionFailed); + .try_fire(); + + let result = match result { + Ok(Ok(v)) => Ok(v), + _ => Err(Error::TransactionFailed), + }; + self.env().emit_event(Execution { transaction: trans_id, result: result.clone().map(Some), diff --git a/examples/upgradeable-contracts/forward-calls/lib.rs b/examples/upgradeable-contracts/forward-calls/lib.rs index 09a47cd399..b4b073a512 100644 --- a/examples/upgradeable-contracts/forward-calls/lib.rs +++ b/examples/upgradeable-contracts/forward-calls/lib.rs @@ -81,11 +81,17 @@ pub mod proxy { .set_forward_input(true) .set_tail_call(true), ) - .fire() - .unwrap_or_else(|err| { + .try_fire() + .unwrap_or_else(|env_err| { panic!( "cross-contract call to {:?} failed due to {:?}", - self.forward_to, err + self.forward_to, env_err + ) + }) + .unwrap_or_else(|lang_err| { + panic!( + "cross-contract call to {:?} failed due to {:?}", + self.forward_to, lang_err ) }); unreachable!(