Return `LangError`s from constructors by HCastano · Pull Request #1504 · use-ink/ink

HCastano · 2022-11-16T23:34:40Z

Follow-up to #1450, and closes the constructor part of #1207.

As in #1450, constructors now return a Result<Constructor::Output, LangError>
which can be handled by callers.

If you're curious, click here for a snippet of the new metadata.

    "constructors": [
  {
    "args": [
      {
        "label": "init_value",
        "type": {
          "displayName": [
            "bool"
          ],
          "type": 0
        }
      }
    ],
    "docs": [
      "Infallible constructor"
    ],
    "label": "new",
    "payable": false,
    "returnType": {
      "displayName": [
        "ink_primitives",
        "ConstructorResult"
      ],
      "type": 1
    },
    "selector": "0x9bae9d5e"
  },
                                        
{
  "id": 1,
  "type": {
    "def": {
      "variant": {
        "variants": [
          {
            "fields": [
              {
                "type": 2
              }
            ],
            "index": 0,
            "name": "Ok"
          },
          {
            "fields": [
              {
                "type": 3
              }
            ],
            "index": 1,
            "name": "Err"
          }
        ]
      }
    },
    "params": [
      {
        "name": "T",
        "type": 2
      },
      {
        "name": "E",
        "type": 3
      }
    ],
    "path": [
      "Result"
    ]
  }
},
{
  "id": 2,
  "type": {
    "def": {
      "tuple": []
    }
  }
},
{
  "id": 3,
  "type": {
    "def": {
      "variant": {
        "variants": [
          {
            "index": 1,
            "name": "CouldNotReadInput"
          }
        ]
      }
    },
    "path": [
      "ink_primitives",
      "LangError"
    ]
  }
}

TODOs (Edit: These are gonna be done in #1512)

~~Add test showing how to handle LangError~~
~~ContractRef implementations~~

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

ascjones

Looks good, we may want to resolve the type of the ReturnValueSpec to be non optional before merging.

ascjones · 2022-11-18T09:44:19Z

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

+                };
+
+                <<#storage_ident as ::ink::reflect::ContractConstructorDecoder>::Type
+                    as ::ink::reflect::ExecuteDispatchable>::execute_dispatchable(dispatchable)


Looking at this control flow again: from a readability perspective, I wonder whether it would be better to have this inside the Ok arm of the match. It seems unusual because of the semantics of return_value returning immediately !. A casual reader might expect this statement to always be executed. Just a note for something to consider when we get to refactoring this dispatch logic.

Yeah that seems fair enough. I'll leave it for a follow-up since we'll want to change it for messages too

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

ascjones · 2022-11-18T09:59:13Z

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

+                        >
+                    >(
+                        "ink_primitives::ConstructorResult"
+                    )


I bet we could fit that all on one line to avoid the ugly indentation e.g.
::ink::ConstructorResult<()>>("ink_primitives::ConstructorResult")

ascjones · 2022-11-18T10:09:36Z

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

                )
                } else {
-                    ::core::option::Option::None
+                    ::core::option::Option::Some(::ink::metadata::TypeSpec::with_name_str::<


ReturnTypeSpec should now always expect a TypeSpec instead of an Option<TypeSpec>. There is one other usage to take care of for trait output types.

I don't know enough about all the metadata types to be able to remove this and still get things compiling, so in the interest of time I'm gonna leave it a a follow-up

Opened #1515

crates/ink/ir/src/ir/item_impl/constructor.rs

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

ascjones · 2022-11-18T11:48:53Z

Another thing to note is that the contract sizes have gone up very slightly again here: so something to investigate for future betas. e.g. erc20 has gone from 7.6 to 7.7

athei

Metadata and return_value invocations look good.

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

athei

I think we forgot the reversion

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

athei · 2022-11-18T13:06:29Z

I saw a change to unify those cases and get away with a single return_value. Revert if you don't like.

ascjones · 2022-11-18T13:31:19Z

I saw a change to unify those cases and get away with a single return_value. Revert if you don't like.

Side effect of this refactoring has been the cancelling out of this size increase:

Another thing to note is that the contract sizes have gone up very slightly again here: so something to investigate for future betas. e.g. erc20 has gone from 7.6 to 7.7

athei · 2022-11-18T13:45:45Z

Okay then let's be nice to the compiler and also unify it for the call case.

Fixed the bug myself

HCastano · 2022-11-19T01:21:45Z

The change related to handling LangErrors from constructors ended up being a bit more
annoying that I expected. I've opened up a separate PR for those (#1512).

The current behaviour of this PR is as follows:

Instantiations through normal dispatch:
- These are fine, we never raise LangErrors
CreateBuilder or ContractRef instantiations:
- These work, but we are not able to handle the LangError
- Instead, we get a ContractReverted error from the Contracts pallet (so kinda like
  the old behaviour)

Since the most important thing here are the metadata changes, we could in principle
merge this. The changes from #1512 would "just" be code level changes.

athei · 2022-11-19T08:42:38Z

Instantiations through normal dispatch:
* These are fine, we never raise `LangError`s

I don't get it. When I use the call builder I define myself what I expect to receive. And I would need to define that I get Result<(), LangError> or the decoding fails because that is what the callee copies into the output buffer for a fallible constructor.

CreateBuilder or ContractRef instantiations:

* These work, but we are not able to handle the `LangError`

* Instead, we get a `ContractReverted` error from the Contracts pallet (so kinda like
  the old behaviour)

Shouldn't it be ContractTrapped because you basically unwrap the outer Result? ContractReverted implies that you called seal_return which doesn't make sense given that you would need to choose an output buffer to return that conforms with the callers return type.

HCastano · 2022-11-19T15:08:02Z

I don't get it. When I use the call builder I define myself what I expect to receive. And I would need to define that I get Result<(), LangError> or the decoding fails because that is what the callee copies into the output buffer for a fallible constructor.

What we want to talk about here isn't the CallBuilder, but the CreateBuilder. At
the moment the CreateBuilder only supports a return type of an AccountId, which is
comes from seal_instantiate. In #1512 I try and add support for reading the LangError
from the output buffer of seal_instatiate.

When I talk about "normal dispatch" I'm referring to calls that go through something like
cargo-contract or the E2E tests . By default neither is able to use an invalid selector
(I guess you could hack the metadata or fiddle at the call site to do this) so these
should not return a LangError.

Shouldn't it be ContractTrapped because you basically unwrap the outer Result? ContractReverted implies that you called seal_return which doesn't make sense given that you would need to choose an output buffer to return that conforms with the callers return type.

If I log the return of the CreateBuilder::instantiate() call I get CalleeReverted. We're failing
to decode the selector and so we hit this:

https://github.com/paritytech/ink/blob/b63ef910b6c02a563cd70b081adbe90ba24ffb12/crates/ink/codegen/src/generator/dispatch.rs#L438-L441

which calls seal_return.

athei · 2022-11-19T15:53:20Z

What we want to talk about here isn't the CallBuilder, but the CreateBuilder. At
the moment the CreateBuilder only supports a return type of an AccountId, which is
comes from seal_instantiate. In #1512 I try and add support for reading the LangError
from the output buffer of seal_instatiate.

Okay I got it. Yes this is a bit more of a high level stuff then. Yes we can leave it that way for the beta.

When I talk about "normal dispatch" I'm referring to calls that go through something like
cargo-contract or the E2E tests . By default neither is able to use an invalid selector
(I guess you could hack the metadata or fiddle at the call site to do this) so these
should not return a LangError.

Yeah those off-chain callers are also not that interesting for ABI. As a matter of fact none of the UIs even displays data coming back from constructors (cause we didn't return anything up until now). We are good here.

If I log the return of the CreateBuilder::instantiate() call I get CalleeReverted. We're failing to decode the selector and so we hit this:

https://github.com/paritytech/ink/blob/b63ef910b6c02a563cd70b081adbe90ba24ffb12/crates/ink/codegen/src/generator/dispatch.rs#L438-L441

This is the callee side. But I thought we are discussing the caller here. How does the caller handle getting a Err(LangError)?

ascjones

LGTM. Please check my remaining unresolved comments

ascjones · 2022-11-21T13:22:11Z

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

+                .await;
+
+            assert!(
+                matches!(result, Err(ink_e2e::Error::InstantiateExtrinsic(_))),


Awesome ❤️

ascjones

I'm fine to delay the CreateBuilder changes in #1512 to a subsequent beta release, based on the philosophy of stablizing the ABI but allowing breaking code changes.

HCastano · 2022-11-21T18:54:11Z

This is the callee side. But I thought we are discussing the caller here. How does the caller handle getting a Err(LangError)?

Sorry, maybe I'm missing something - but right now they don't. There are two scenarios:

Calling an invalid selector with cargo-contract: ContractReverted, nothing to do
Calling an invalid selector via CreateBuilder: ContractReverted, but can continue its own execution

athei · 2022-11-21T19:46:04Z

Sorry, maybe I'm missing something - but right now they don't. There are two scenarios:

Checked the code. Looks alright. We don't even decode the return buffer for instantiate. So we are safe here. So from my side we can merge this and release the first beta. #1512 is not needed for the beta.

Maybe it's a naming clash?

This reverts commit cab6c98.

codecov-commenter · 2022-11-21T21:45:52Z

Codecov Report

Merging #1504 (ad1cab7) into master (adf8f26) will decrease coverage by 5.85%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
- Coverage   71.70%   65.85%   -5.86%     
==========================================
  Files         204      204              
  Lines        6306     6314       +8     
==========================================
- Hits         4522     4158     -364     
- Misses       1784     2156     +372

Impacted Files	Coverage Δ
crates/e2e/src/client.rs	`66.66% <ø> (ø)`
crates/ink/codegen/src/generator/dispatch.rs	`94.61% <ø> (ø)`
crates/ink/codegen/src/generator/metadata.rs	`95.20% <ø> (ø)`
...i/contract/pass/constructor-return-result-alias.rs	`93.87% <93.33%> (-1.00%)`	⬇️
crates/ink/ir/src/ir/contract.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/ink/ir/src/ir/ink_test.rs	`0.00% <0.00%> (-87.50%)`	⬇️
crates/ink/ir/src/ir/trait_def/mod.rs	`0.00% <0.00%> (-81.82%)`	⬇️
crates/ink/ir/src/ir/storage_item/mod.rs	`0.00% <0.00%> (-73.92%)`	⬇️
crates/ink/ir/src/ir/storage_item/config.rs	`0.00% <0.00%> (-73.34%)`	⬇️
crates/ink/ir/src/ir/blake2.rs	`20.83% <0.00%> (-58.34%)`	⬇️
... and 20 more

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

deuszx · 2023-01-03T17:08:49Z

examples/lang-err-integration-tests/call-builder/lib.rs

+            // 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).
+            result.ok().map(|id| ink::ToAccountId::to_account_id(&id))


How do you recommend handling instantiating other contracts inside a constructor?

HCastano added 13 commits November 16, 2022 16:43

Return ConstructorResult in deploy()

536b500

Wrap return type with Result in metadata

1ab55d9

Check that constructor's return Results in tests

79dff48

Add test showing that Results are decoded correctly

228604c

Correctly generate metadata for constructors

bb4c1a2

Merge branch 'master' into hc-impl-lang-error-for-constructors

15439a4

Fix some nitpicks from Andrew's PR

27b6b97

Wrap dispatch return types with Ok

eb10da6

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

Manually wrap metadata return with ConstructorResult

8b1aa66

Fix existing constructor integration tests

36556e7

Remove constructor related test from integration-flipper

e61576e

Fix compile tests

a3e0b24

Add ident to dictionary

fb8d293

HCastano marked this pull request as ready for review November 17, 2022 23:56

HCastano requested review from ascjones and cmichi as code owners November 17, 2022 23:56

HCastano requested a review from athei November 18, 2022 00:01

ascjones reviewed Nov 18, 2022

View reviewed changes

ascjones mentioned this pull request Nov 18, 2022

Release v4.0.0-beta #1508

Merged

1 task

athei approved these changes Nov 18, 2022

View reviewed changes

crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved

crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved

crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved

athei previously requested changes Nov 18, 2022

View reviewed changes

crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved

Simplify code

394ffca

Driveby: Also simplify call dispatch

2f026a2

HCastano added 2 commits November 18, 2022 12:28

Small fixes to type paths

c501d84

Check that actual instantiate call fails

c307516

Clean up the create_builder tests

b63ef91

ascjones reviewed Nov 21, 2022

View reviewed changes

ascjones approved these changes Nov 21, 2022

View reviewed changes

HCastano added 3 commits November 21, 2022 09:08

Merge branch 'master' into hc-impl-lang-error-for-constructors

0c37262

Remove unused method

ec6253d

Format code for generating constructor return type

d56b728

Add constructors-return-value to CI

053f570

HCastano requested a review from a team as a code owner November 21, 2022 19:17

HCastano added 3 commits November 21, 2022 14:17

Move shared imports out of messages

7305887

Appease Clippy

0ef75f9

Appease Clippy

4607a67

HCastano added 6 commits November 21, 2022 15:15

Try flipping order of tests

f791bfb

Change message name

5d0f164

Maybe it's a naming clash?

Revert last two changes

e8b4e75

Try moving message related test to different module

cab6c98

Revert "Try moving message related test to different module"

4dc9973

This reverts commit cab6c98.

Try different type for account ID

ad1cab7

HCastano merged commit 507d01d into master Nov 21, 2022

HCastano deleted the hc-impl-lang-error-for-constructors branch November 21, 2022 21:48

This was referenced Nov 23, 2022

ReturnTypeSpec should take a TypeSpec directly #1515

Closed

Handle LangError from instantiate #1512

Merged

deuszx reviewed Jan 3, 2023

View reviewed changes

ascjones mentioned this pull request Feb 15, 2023

Release v4.0.0 #1653

Merged

Conversation

HCastano commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ascjones commented Nov 18, 2022

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

athei commented Nov 18, 2022

Uh oh!

ascjones commented Nov 18, 2022

Uh oh!

athei commented Nov 18, 2022

Uh oh!

HCastano commented Nov 19, 2022

Uh oh!

athei commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HCastano commented Nov 19, 2022

Uh oh!

athei commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

HCastano commented Nov 21, 2022

Uh oh!

athei commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 21, 2022

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HCastano commented Nov 16, 2022 •

edited

Loading

athei commented Nov 19, 2022 •

edited

Loading

athei commented Nov 19, 2022 •

edited

Loading

athei commented Nov 21, 2022 •

edited

Loading