Support Result<Self, E> returning ink! constructors

[Robbepop]

Currently it is not possible to return anything but Self in ink! constructors.
This is an artificial limitation created to make things a bit easier, however, technically as well as from a design perspective nothing speaks against introducing fallible ink! constructors.
They will behave in the same way to fallible ink! messages in that they will revert state in case Result::Err is returned.

Mainly ink_lang_ir as well as ink_lang_codegen crates are required to be modified in order to implement this feature.

After this change ink! constructors can either return Self or Result<Self, E> where E is some error type:

use ink_lang as ink;

#[ink::contract]
mod contract {
    #[ink(storage)]
    pub struct Contract {
        value: Balance,
    }

    enum Error {
        TooMuchInitValue,
    }

    impl Contract {
        #[ink(constructor)]
        fn new() -> Self {
            self.value = 0;
        }

        #[ink(constructor)]
        fn with_balance(balance: Balance) -> Result<Self, Error> {
            if balance >= 1000 {
                return Err(Error::TooMuchInitValue)
            }
            Self { value: balance }
        }
    }
}

[SkymanOne]

After some fiddling, we can use the same approach for failable constructors as one for messages, but it would require including [derive(scale::Encode)] for the storage struct, not sure what implications it will have.

Otherwise, we need a different approach where we deduce whether the constructor has a Result return type and have different token parsing for each variant

[cmichi]

it would require including [derive(scale::Encode)] for the storage struct, not sure what implications it will have.

I'm just wondering why we don't require this bound for #[ink(storage)] as of now or derive scale::Encode by default for it. All storage items need to be encoded to storage anyway. So I can't see a negative implication. @xgreenx I'm interested in your thoughts?

[xgreenx]

I guess [derive(scale::Encode)] helped you because now you store in the storage Result<Contract>, instead of Contract =)

Why just not use the approach that we did for messages?
is_result_type and is_result_err works well, we also need to add one more macro to extract Self from the Result<Self>, but you can do it in the same way as we did it for is_result_err.

[SkymanOne]

@xgreenx - This is exactly the approach I am following. As you correctly mentioned, there is already partial support for failable constructors, it wasn't just used at the time.

@cmichi - I also realised that, it absolutely makes sense for storage to also derive scale::Encode since the Result output should be encoded whether it is the error or storage itself.

[xgreenx]

I also realised that, it absolutely makes sense for storage to also derive scale::Encode since the Result output should be encoded whether it is the error or storage itself.

If the type implements scale::Encode, it means that it can be transferred across contracts. We can't transfer the storage itself, only {Contract}Ref so contract ref should implement scale::Encode and scale::Decode. If the result is Ok(Contract) we should use Storable methods to push the contract into the storage.

[SkymanOne]

I also realised that, it absolutely makes sense for storage to also derive scale::Encode since the Result output should be encoded whether it is the error or storage itself.

If the type implements scale::Encode, it means that it can be transferred across contracts. We can't transfer the storage itself, only {Contract}Ref so contract ref should implement scale::Encode and scale::Decode. If the result is Ok(Contract) we should use Storable methods to push the contract into the storage.

I was thinking of manually checking the result output. If the constructor fails => return encoded Error, otherwise push the contract to storage.

[xgreenx]

I was thinking of manually checking the result output. If the constructor fails => return encoded Error, otherwise push the contract to storage.

Yeah, it is what we need to do, but to push the contract into the storage we don't need to impl scale::Encode

[xgreenx]

The usage of Result in the constructor means that the CallBuilder part({Contract}Ref implementation) also should support it.

We have two possible solutions to do it:

• Encode Result<{Contract}Ref> and decode it. It means that we need to pass encoded AccountId between contracts.
• Encode only error, decode this error in the {Contract}Ref implementation and if it is not an error, then return Ok(AccountId.into()), if error, return Err(ContractsError)

[SkymanOne]

messages

While this is true, is it necessary bad? The reason why the storage derives scale::Encode is because there is a trait bound in ::ink::env::return_value()

Event existing implementation of execute_constructor() support failable constructors and enforces the return type to derive scale::Encode