Unify fallible and non fallible instantiate methods #1591

ascjones · 2023-01-14T22:11:26Z

Since #1512 CreateBuilder has 4 different instantiate methods:

instantiate
instantiate_fallible
try_instantiate
try_instantiate_faillble

The use case for this is when we have a "fallible" constructor which returns a Result e.g.

#[ink(constructor)]
pub fn new() -> Result<Self, Error> {
    Ok(Self {})
}

In order to invoke this from another contract, the following is required:

ContractRef::new()
            .code_hash(ink_primitives::Clear::CLEAR_HASH)
            .gas_limit(4000)
            .endowment(25)
            .salt_bytes([0xDE, 0xAD, 0xBE, 0xEF])
            .instantiate_fallible() // must call this because it returns a result.
            .unwrap()

Some issues with this approach:

Requires the user to know which variant of instantiate to call based on the return type of the constructor. Granted it should not compile with the wrong one, but still will cause some friction.
Code duplication all the way down, two versions of each instantiate method.

This PR unifies the instantaite and instantiate_fallible versions of each method call, such that the user only needs to call instantiate or instantiate_fallible, and the CreateBuilder method will return either Result<ContranctRef> or plain ContractRef, depending on the actual return type of the constructor.

See this UI test for the variations of constructor return types.

HCastano

Looks interesting 👀

crates/ink/codegen/src/generator/as_dependency/contract_ref.rs

HCastano · 2023-01-15T04:33:09Z

crates/env/src/api.rs

    Salt: AsRef<[u8]>,
-    ContractError: scale::Decode,
+    R: InstantiateResult<R>,
+    ContractRef: FromAccountId<E>,


Why not use the E::AccountId type directly?

I did start with that, but would have had to implement some map capability that does id e.g. AccountId -> AccountId and AccountId -> Result<(), AccountId> and it was slightly tricky. Maybe possible but thought it was easier just to call FromAccountId in place to get the thing working.

crates/env/src/engine/mod.rs

crates/ink/codegen/src/generator/as_dependency/contract_ref.rs

HCastano · 2023-01-17T01:05:04Z

crates/env/src/engine/mod.rs

+                                let contract_err = <<R as InstantiateResult<ContractStorage>>::Error
+                                    as scale::Decode>::decode(out_return_value)?;
+                                let err = <R as InstantiateResult<ContractStorage>>::err(contract_err);
+                                Ok(Ok(err))


nit: I'd use the typedefs here to make it more clear as to which Result is which

Suggested change

Ok(Ok(err))

EnvResult::Ok(ConstructorResult::Ok(err))

HCastano · 2023-01-17T01:05:34Z

crates/env/src/engine/mod.rs

+                            1 => {
+                                let contract_err = <<R as InstantiateResult<ContractStorage>>::Error
+                                    as scale::Decode>::decode(out_return_value)?;
+                                let err = <R as InstantiateResult<ContractStorage>>::err(contract_err);


Suggested change

let err = <R as InstantiateResult<ContractStorage>>::err(contract_err);

let contract_err = <R as InstantiateResult<ContractStorage>>::err(contract_err);

HCastano · 2023-01-17T23:17:57Z

@ascjones merging master into this messed up the commit history since #1512 hasn't been updated yet

codecov-commenter · 2023-01-18T05:43:24Z

Codecov Report

Merging #1591 (8c3f65c) into hc-get-lang-error-from-create-builder (1e323f4) will decrease coverage by 0.41%.
The diff coverage is 43.33%.

❗ Current head 8c3f65c differs from pull request most recent head ccf61eb. Consider uploading reports for the commit ccf61eb to get more accurate results

@@                            Coverage Diff                            @@
##           hc-get-lang-error-from-create-builder    #1591      +/-   ##
=========================================================================
- Coverage                                  65.86%   65.45%   -0.41%     
=========================================================================
  Files                                        205      205              
  Lines                                       6330     6285      -45     
=========================================================================
- Hits                                        4169     4114      -55     
- Misses                                      2161     2171      +10

Impacted Files	Coverage Δ
crates/e2e/macro/src/codegen.rs	`0.00% <0.00%> (ø)`
crates/e2e/src/client.rs	`36.36% <0.00%> (-3.64%)`	⬇️
crates/e2e/src/lib.rs	`25.00% <0.00%> (-8.34%)`	⬇️
crates/e2e/src/xts.rs	`0.00% <ø> (ø)`
crates/env/src/call/call_builder.rs	`0.00% <ø> (ø)`
crates/env/src/engine/off_chain/test_api.rs	`64.70% <ø> (ø)`
crates/ink/ir/src/ir/storage_item/mod.rs	`0.00% <0.00%> (ø)`
crates/ink/macro/src/lib.rs	`100.00% <ø> (ø)`
crates/ink/src/env_access.rs	`9.09% <ø> (ø)`
...i/contract/pass/constructor-return-result-alias.rs	`50.00% <ø> (-43.88%)`	⬇️
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ascjones · 2023-01-18T09:23:38Z

@ascjones merging master into this messed up the commit history since #1512 hasn't been updated yet

Sorry I will fix it.

ascjones · 2023-01-20T18:05:59Z

crates/env/src/contract.rs

+///
+/// fn main() {}
+/// ```
+pub trait ContractEnv {


These traits have just been copied from ink::reflect so we can use them here in ink_env

HCastano

There's still a couple things I want to review, but in general it's looking good 👍

HCastano · 2023-01-21T17:48:40Z

examples/lang-err-integration-tests/constructors-return-value/lib.rs

        }

-        /// A constructor which reverts and fills the output buffer with an erronenously encoded
+        /// A constructor which reverts and fills the output buffer with an erroneously encoded


Lol, awkward that this was spelt erroneously 😅

crates/env/src/call/create_builder.rs

HCastano · 2023-01-21T18:16:02Z

crates/env/src/contract.rs

+///
+/// fn main() {}
+/// ```
+pub trait ContractEnv {


Not related to this PR, but I don't really understand why we don't use the Environment trait directly. E.g why not have T: Environment instead of going through this extra wrapper?

Fixes some inconsistent errors between Clippy and `rustc`

HCastano

Thanks for cleaning up the API, looks much better now!

ascjones mentioned this pull request Jan 14, 2023

Handle LangError from instantiate #1512

Merged

HCastano reviewed Jan 15, 2023

View reviewed changes

HCastano reviewed Jan 16, 2023

View reviewed changes

crates/env/src/engine/mod.rs Outdated Show resolved Hide resolved

crates/ink/codegen/src/generator/as_dependency/contract_ref.rs Show resolved Hide resolved

HCastano reviewed Jan 17, 2023

View reviewed changes

ascjones force-pushed the aj/get-lang-error-from-create-builder branch from ccf61eb to a7a2f7a Compare January 18, 2023 09:53

ascjones marked this pull request as ready for review January 20, 2023 16:37

ascjones requested a review from cmichi as a code owner January 20, 2023 16:37

ascjones changed the title ~~Attempt to unify fallible and non fallible instantiate methods~~ Unify fallible and non fallible instantiate methods Jan 20, 2023

ascjones commented Jan 20, 2023

View reviewed changes

HCastano reviewed Jan 21, 2023

View reviewed changes

Base automatically changed from hc-get-lang-error-from-create-builder to master January 23, 2023 22:02

ascjones added 16 commits January 23, 2023 17:20

Remove fallible create_builder.rs methods

3758e09

WIP experiment

cdcc502

InstantiateResult blanket impl for T and Result

6f59077

Introduce ContractRef type parameter

baecba9

Fix up env access

af69cd5

WIP...

e134a8c

Make it compile

115dd9f

Add ContractStorage parameter

718d475

Remove commented out instantiate_fallible_contract

bb5042e

Convert to env Error in helper

528cfe0

Return Decode errors in case of invalid Result first byte

7b4c5e9

Fix impls::instantiate_contract

4de38f5

Remove ContractStorage generic parameter

09545fe

Fix env access

2ef624d

Use generated constructor ref, introduces update_selector

02f97b1

Fix e2e

39b45b0

ascjones and others added 22 commits January 23, 2023 17:29

Fix ui test

f1f4930

Fmt

12f51dc

Remove unused assoc type

1bceab3

Change error fn to return Option<Result>

8e0a9ef

Remove commented out code

aea1238

Fmt

163fa6b

ConstructorReturnType comments

1dd00d6

Fix contract-ref E2E test compilation

327967d

Fix up return types after merge

0bb7a8d

Fmt

75ab4e6

Clippy

77a5bab

Fix create_builder tests

7d03708

Fix cross-contract compile test

ca47caf

Clean up some comments

2cc8a22

Remove outdated doc

fa0436a

Update comment

016b102

Another comment fix

9655072

Wrap long line

7aecb5f

Remove TODO

6f92033

Bump contract-metadata

498c640

Fixes some inconsistent errors between Clippy and `rustc`

Fix CreateBuilder compilation

8e60ec2

Fix one of the doc tests

a9c0223

HCastano force-pushed the aj/get-lang-error-from-create-builder branch from 4dd4a3f to a9c0223 Compare January 24, 2023 01:47

Clean up doc tests a bit

db1a808

HCastano approved these changes Jan 24, 2023

View reviewed changes

ascjones mentioned this pull request Jan 24, 2023

[ink_e2e] method to generate and fund unique accounts #1615

Merged

ascjones merged commit 59bbd36 into master Jan 24, 2023

ascjones deleted the aj/get-lang-error-from-create-builder branch January 24, 2023 19:24

HCastano mentioned this pull request Jan 24, 2023

Release v4.0.0-beta.1 #1617

Merged

ascjones mentioned this pull request Feb 15, 2023

Release v4.0.0 #1653

Merged

	let err = <R as InstantiateResult<ContractStorage>>::err(contract_err);
	let contract_err = <R as InstantiateResult<ContractStorage>>::err(contract_err);

Unify fallible and non fallible instantiate methods #1591

Unify fallible and non fallible instantiate methods #1591

Uh oh!

Conversation

ascjones commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HCastano Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

ascjones Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HCastano Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

HCastano Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

HCastano commented Jan 17, 2023

Uh oh!

codecov-commenter commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ascjones commented Jan 18, 2023

Uh oh!

ascjones Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

HCastano Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HCastano Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ascjones commented Jan 14, 2023 •

edited

Loading

ascjones Jan 16, 2023 •

edited

Loading

codecov-commenter commented Jan 18, 2023 •

edited

Loading