Return LangErrors from constructors (#1504)

* Return `ConstructorResult` in `deploy()`

* Wrap return type with `Result` in metadata

* Check that constructor's return Results in tests

* Add test showing that `Result`s are decoded correctly

* Correctly generate metadata for constructors

* Fix some nitpicks from Andrew's PR

* Wrap dispatch return types with `Ok`

These can't return dispatch errors atm. I may want to clean up how this
is done later.

* Manually wrap metadata return with `ConstructorResult`

* Fix existing constructor integration tests

* Remove constructor related test from `integration-flipper`

* Fix compile tests

* Add `ident` to dictionary

* Simplify code

* Driveby: Also simplify call dispatch

* Small fixes to type paths

* Check that actual instantiate call fails

* Add some tests using the `CreateBuilder`

* Clean up the `create_builder` tests

* Remove unused method

* Format code for generating constructor return type

* Add `constructors-return-value` to CI

* Move shared imports out of messages

* Appease Clippy

* Appease Clippy

* Try flipping order of tests

* Change message name

Maybe it's a naming clash?

* Revert last two changes

* Try moving message related test to different module

* Revert "Try moving message related test to different module"

This reverts commit cab6c98.

* Try different type for account ID

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

.config/cargo_spellcheck.dic

@@ -46,6 +46,7 @@ evaluable
 fuzzer
 getter
 growable
+ident
 interoperate
 invariants
 kB

.gitlab-ci.yml

@@ -34,7 +34,7 @@ variables:
   ALL_CRATES:                      "${PURELY_STD_CRATES} ${ALSO_WASM_CRATES}"
   DELEGATOR_SUBCONTRACTS:          "accumulator adder subber"
   UPGRADEABLE_CONTRACTS:           "forward-calls set-code-hash"
-  LANG_ERR_INTEGRATION_CONTRACTS:  "integration-flipper call-builder contract-ref"
+  LANG_ERR_INTEGRATION_CONTRACTS:  "integration-flipper call-builder contract-ref constructors-return-value"
   # TODO `cargo clippy --verbose --all-targets --all-features` for this crate
   # currently fails on `stable`, but succeeds on `nightly`. This is due to
   # this fix not yet in stable: /~https://github.com/rust-lang/rust-clippy/issues/8895.

crates/e2e/src/client.rs

@@ -341,9 +341,9 @@ where
             .instantiate_with_code_dry_run(
                 value,
                 storage_deposit_limit,
-                code.clone(),
-                data.clone(),
-                salt.clone(),
+                code,
+                data,
+                salt,
                 signer,
             )
             .await

crates/ink/codegen/src/generator/dispatch.rs

@@ -270,8 +270,8 @@ impl Dispatch<'_> {
                         type Input = #input_tuple_type;
                         type Output = #output_type;
                         type Storage = #storage_ident;
-                        type Error = #constructor_return_type :: Error;
-                        const IS_RESULT: ::core::primitive::bool = #constructor_return_type :: IS_RESULT;
+                        type Error = #constructor_return_type::Error;
+                        const IS_RESULT: ::core::primitive::bool = #constructor_return_type::IS_RESULT;
 
                         const CALLABLE: fn(Self::Input) -> Self::Output = |#input_tuple_bindings| {
                             #storage_ident::#constructor_ident(#( #input_bindings ),* )
@@ -420,16 +420,33 @@ impl Dispatch<'_> {
                         .unwrap_or_else(|error| ::core::panic!("{}", error))
                 }
 
-                ::ink::env::decode_input::<
-                        <#storage_ident as ::ink::reflect::ContractConstructorDecoder>::Type>()
-                    .map_err(|_| ::ink::reflect::DispatchError::CouldNotReadInput)
-                    .and_then(|decoder| {
-                        <<#storage_ident as ::ink::reflect::ContractConstructorDecoder>::Type
-                            as ::ink::reflect::ExecuteDispatchable>::execute_dispatchable(decoder)
-                    })
-                    .unwrap_or_else(|error| {
-                        ::core::panic!("dispatching ink! constructor failed: {}", error)
-                    })
+                let dispatchable = match ::ink::env::decode_input::<
+                    <#storage_ident as ::ink::reflect::ContractConstructorDecoder>::Type,
+                >() {
+                    ::core::result::Result::Ok(decoded_dispatchable) => {
+                        decoded_dispatchable
+                    }
+                    ::core::result::Result::Err(_decoding_error) => {
+                        let error = ::ink::ConstructorResult::Err(::ink::LangError::CouldNotReadInput);
+
+                        // At this point we're unable to set the `Ok` variant to be the any "real"
+                        // constructor output since we were unable to figure out what the caller wanted
+                        // to dispatch in the first place, so we set it to `()`.
+                        //
+                        // This is okay since we're going to only be encoding the `Err` variant
+                        // into the output buffer anyways.
+                        ::ink::env::return_value::<::ink::ConstructorResult<()>>(
+                            ::ink::env::ReturnFlags::new_with_reverted(true),
+                            &error,
+                        );
+                    }
+                };
+
+                <<#storage_ident as ::ink::reflect::ContractConstructorDecoder>::Type
+                    as ::ink::reflect::ExecuteDispatchable>::execute_dispatchable(dispatchable)
+                .unwrap_or_else(|error| {
+                    ::core::panic!("dispatching ink! message failed: {}", error)
+                })
             }
 
             #[cfg(not(test))]
@@ -448,7 +465,7 @@ impl Dispatch<'_> {
                         decoded_dispatchable
                     }
                     ::core::result::Result::Err(_decoding_error) => {
-                        let error = ::core::result::Result::Err(::ink::LangError::CouldNotReadInput);
+                        let error = ::ink::MessageResult::Err(::ink::LangError::CouldNotReadInput);
 
                         // At this point we're unable to set the `Ok` variant to be the any "real"
                         // message output since we were unable to figure out what the caller wanted
@@ -606,30 +623,25 @@ impl Dispatch<'_> {
 
                     let result: #constructor_output = #constructor_callable(input);
                     let output_value = ::ink::reflect::ConstructorOutputValue::new(result);
+                    let output_result = #constructor_value::as_result(&output_value);
 
-                    match #constructor_value :: as_result(&output_value) {
-                        ::core::result::Result::Ok(contract) => {
-                            ::ink::env::set_contract_storage::<::ink::primitives::Key, #storage_ident>(
-                                &<#storage_ident as ::ink::storage::traits::StorageKey>::KEY,
-                                contract,
-                            );
-                            // only fallible constructors return success `Ok` back to the caller.
-                            if #constructor_value :: IS_RESULT {
-                                ::ink::env::return_value::<::core::result::Result<&(), ()>>(
-                                    ::ink::env::ReturnFlags::new_with_reverted(false),
-                                    &::core::result::Result::Ok(&())
-                                )
-                            } else {
-                                ::core::result::Result::Ok(())
-                            }
-                        },
-                        ::core::result::Result::Err(err) => {
-                           ::ink::env::return_value::<::core::result::Result<(), & #constructor_value :: Error>>(
-                                ::ink::env::ReturnFlags::new_with_reverted(true),
-                                &::core::result::Result::Err(err)
-                            )
-                        }
+                    if let ::core::result::Result::Ok(contract) = output_result.as_ref() {
+                        ::ink::env::set_contract_storage::<::ink::primitives::Key, #storage_ident>(
+                            &<#storage_ident as ::ink::storage::traits::StorageKey>::KEY,
+                            contract,
+                        );
                     }
+
+                    ::ink::env::return_value::<
+                        ::ink::ConstructorResult<
+                            ::core::result::Result<(), &#constructor_value::Error>
+                        >,
+                    >(
+                        ::ink::env::ReturnFlags::new_with_reverted(output_result.is_err()),
+                        // Currently no `LangError`s are raised at this level of the
+                        // dispatch logic so `Ok` is always returned to the caller.
+                        &::ink::ConstructorResult::Ok(output_result.map(|_| ())),
+                    );
                 }
             )
         });
@@ -814,27 +826,19 @@ impl Dispatch<'_> {
                     }
 
                     let result: #message_output = #message_callable(&mut contract, input);
-                    let failure = ::ink::is_result_type!(#message_output)
+                    let is_reverted = ::ink::is_result_type!(#message_output)
                         && ::ink::is_result_err!(result);
 
-                    // Currently no `LangError`s are raised at this level of the dispatch logic
-                    // so `Ok` is always returned to the caller.
-                    let return_value = ::core::result::Result::Ok(result);
-
-                    if failure {
-                        // We return early here since there is no need to push back the
-                        // intermediate results of the contract - the transaction is going to be
-                        // reverted anyways.
-                        ::ink::env::return_value::<::ink::MessageResult::<#message_output>>(
-                            ::ink::env::ReturnFlags::new_with_reverted(true),
-                            &return_value
-                        )
+                    // no need to push back results: transaction gets reverted anyways
+                    if !is_reverted {
+                        push_contract(contract, #mutates_storage);
                     }
 
-                    push_contract(contract, #mutates_storage);
-
                     ::ink::env::return_value::<::ink::MessageResult::<#message_output>>(
-                        ::ink::env::ReturnFlags::new_with_reverted(false), &return_value
+                        ::ink::env::ReturnFlags::new_with_reverted(is_reverted),
+                        // Currently no `LangError`s are raised at this level of the
+                        // dispatch logic so `Ok` is always returned to the caller.
+                        &::ink::MessageResult::Ok(result),
                     )
                 }
             )

crates/ink/codegen/src/generator/metadata.rs

@@ -334,7 +334,7 @@ impl Metadata<'_> {
         }
     }
 
-    /// Generates ink! metadata for the storage with given selector and indentation.
+    /// Generates ink! metadata for the storage with given selector and ident.
     fn generate_constructor_return_type(
         storage_ident: &Ident,
         selector_id: u32,
@@ -343,22 +343,17 @@ impl Metadata<'_> {
         let constructor_info = quote_spanned!(span =>
             < #storage_ident as ::ink::reflect::DispatchableConstructorInfo<#selector_id>>
         );
+
         quote_spanned!(span=>
-            ::ink::metadata::ReturnTypeSpec::new(
-                if #constructor_info ::IS_RESULT {
-                    ::core::option::Option::Some(::ink::metadata::TypeSpec::with_name_str::<
-                        ::core::result::Result<
-                            (),
-                            #constructor_info ::Error
-                        >
-                    >(
-                        "core::result::Result"
-                    )
-                )
-                } else {
-                    ::core::option::Option::None
-                }
-            )
+            ::ink::metadata::ReturnTypeSpec::new(if #constructor_info::IS_RESULT {
+                ::core::option::Option::Some(::ink::metadata::TypeSpec::with_name_str::<
+                    ::ink::ConstructorResult<::core::result::Result<(), #constructor_info::Error>>,
+                >("ink_primitives::ConstructorResult"))
+            } else {
+                ::core::option::Option::Some(::ink::metadata::TypeSpec::with_name_str::<
+                    ::ink::ConstructorResult<()>,
+                >("ink_primitives::ConstructorResult"))
+            })
         )
     }
 

crates/ink/src/lib.rs

@@ -72,6 +72,7 @@ pub use ink_macro::{
     trait_definition,
 };
 pub use ink_primitives::{
+    ConstructorResult,
     LangError,
     MessageResult,
 };

crates/ink/tests/ui/contract/fail/constructor-return-result-non-codec-error.stderr

@@ -1,8 +1,8 @@
-error[E0277]: the trait bound `Result<(), &contract::Error>: Encode` is not satisfied
+error[E0277]: the trait bound `Result<Result<(), &contract::Error>, LangError>: Encode` is not satisfied
   --> tests/ui/contract/fail/constructor-return-result-non-codec-error.rs:13:9
    |
 13 |         pub fn constructor() -> Result<Self, Error> {
-   |         ^^^ the trait `Encode` is not implemented for `Result<(), &contract::Error>`
+   |         ^^^ the trait `Encode` is not implemented for `Result<Result<(), &contract::Error>, LangError>`
    |
    = help: the trait `Encode` is implemented for `Result<T, E>`
 note: required by a bound in `return_value`
@@ -28,6 +28,8 @@ error[E0277]: the trait bound `contract::Error: TypeInfo` is not satisfied
             (A, B, C, D, E, F)
           and $N others
   = note: required for `Result<(), contract::Error>` to implement `TypeInfo`
+  = note: 1 redundant requirement hidden
+  = note: required for `Result<Result<(), contract::Error>, LangError>` to implement `TypeInfo`
 note: required by a bound in `TypeSpec::with_name_str`
  --> $WORKSPACE/crates/metadata/src/specs.rs
   |

crates/ink/tests/ui/contract/pass/constructor-return-result-alias.rs

@@ -58,7 +58,7 @@ fn main() {
     assert_eq!("constructor", constructor.label());
     let type_spec = constructor.return_type().opt_type().unwrap();
     assert_eq!(
-        "core::result::Result",
+        "ink_primitives::ConstructorResult",
         format!("{}", type_spec.display_name())
     );
     let ty = metadata.registry().resolve(type_spec.ty().id()).unwrap();
@@ -68,16 +68,39 @@ fn main() {
         scale_info::TypeDef::Variant(variant) => {
             assert_eq!(2, variant.variants().len());
 
-            let ok_variant = &variant.variants()[0];
-            let ok_field = &ok_variant.fields()[0];
-            let ok_ty = metadata.registry().resolve(ok_field.ty().id()).unwrap();
+            // Outer Result
+            let outer_ok_variant = &variant.variants()[0];
+            let outer_ok_field = &outer_ok_variant.fields()[0];
+            let outer_ok_ty = metadata
+                .registry()
+                .resolve(outer_ok_field.ty().id())
+                .unwrap();
+            assert_eq!("Ok", outer_ok_variant.name());
+
+            // Inner Result
+            let inner_ok_ty = match outer_ok_ty.type_def() {
+                scale_info::TypeDef::Variant(variant) => {
+                    assert_eq!(2, variant.variants().len());
+
+                    let inner_ok_variant = &variant.variants()[0];
+                    assert_eq!("Ok", inner_ok_variant.name());
+
+                    let inner_ok_field = &inner_ok_variant.fields()[0];
+                    metadata
+                        .registry()
+                        .resolve(inner_ok_field.ty().id())
+                        .unwrap()
+                }
+                td => panic!("Expected a Variant type def enum, got {:?}", td),
+            };
+
             let unit_ty = scale_info::TypeDef::Tuple(
                 scale_info::TypeDefTuple::new_portable(vec![]),
             );
-            assert_eq!("Ok", ok_variant.name());
+
             assert_eq!(
                 &unit_ty,
-                ok_ty.type_def(),
+                inner_ok_ty.type_def(),
                 "Ok variant should be a unit `()` type"
             );
 

crates/primitives/src/lib.rs

@@ -59,3 +59,7 @@ pub enum LangError {
 /// The `Result` type for ink! messages.
 #[doc(hidden)]
 pub type MessageResult<T> = ::core::result::Result<T, LangError>;
+
+/// The `Result` type for ink! constructors.
+#[doc(hidden)]
+pub type ConstructorResult<T> = ::core::result::Result<T, LangError>;

examples/lang-err-integration-tests/call-builder/Cargo.toml

@@ -11,6 +11,9 @@ ink = { path = "../../../crates/ink", default-features = false }
 scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
 scale-info = { version = "2.3", default-features = false, features = ["derive"], optional = true }
 
+constructors_return_value = { path = "../constructors-return-value", default-features = false, features = ["ink-as-dependency"] }
+integration_flipper = { path = "../integration-flipper", default-features = false, features = ["ink-as-dependency"] }
+
 [dev-dependencies]
 ink_e2e = { path = "../../../crates/e2e" }
 
@@ -28,6 +31,9 @@ std = [
     "ink/std",
     "scale/std",
     "scale-info/std",
+
+    "constructors_return_value/std",
+    "integration_flipper/std",
 ]
 ink-as-dependency = []
 e2e-tests = []