Handle `LangError` from instantiate #1512

HCastano · 2022-11-19T01:07:38Z

This is a follow up to #1504, in which LangErrors were returned from constructors. In
that PR we had no way to read the output buffer from seal_instantiate and bubble any
information back up to the contract caller.

This PR adds support for reading this output buffer for both infallible and fallible
constructors.

Because of type checking problems I've had to add a new set of APIs for handling fallible
constructors. I'm still looking for a way to unify them, as mentioned in this comment.
If you have any ideas for improvements here I'd be happy to hear them.

I've changed the create_builder API slightly to be more in-line with the call_builder
one (i.e removing the generic in favour of a returns method).

For infallible constructors, the main changes are:

The CreateBuilder now returns a ConstructorResult (introduced in Return LangErrors from constructors #1504)
- This can optionally be handled using the new try_instantiate method

For fallible constructors:

We've added intantiate_fallible and try_instantiate_fallible CreateBuilder methods
- Like above, ConstructorErrors are only bubbled up in the try_* variant
- Edit: This API is clunky, but Andrew's proposing a nicer solution in
  Unify fallible and non fallible instantiate methods #1591
We've added a new environment API, ink_env::instantiate_fallible_contract

I've also updated the codegen so that ContractRefs can be used to instantiate contracts,
as an alternative to the "raw" CreateBuilder API.

Notes for reviewers:

Keep in mind that there are two ways that the output buffer of seal_instantiate can
be written to:
- By the ink! codegen
- Manually by the callee contract using ink_env::return_value
  - This second one is the one we need to worry about since people can (on purpose
    or by accident) encode erroneous values in there
Related to the above, make sure to take a close look at the different output buffer
decoding scenarios
Take a look at the E2E tests to see if they make sense
Right now the E2E test fail in the CI, not because they're actually broken but because
of E2E test "panicked at 'error on call sign_and_submit_then_watch_default" #1498
(run them locally if you don't believe me 😆)

This commit lets us grab what's in the output buffer after our call to `instantiate`, however we are unable to succesfully decode the `AccountId` from the success case.

I accidently introduced this not knowing that the generic `C` was for the return type

Is supposed to help give better error messages if somebody uses a wrong type with the builder

codecov-commenter · 2022-11-29T00:43:56Z

Codecov Report

Merging #1512 (aa4f897) into master (bf2de8f) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

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

@@            Coverage Diff             @@
##           master    #1512      +/-   ##
==========================================
- Coverage   71.67%   71.57%   -0.11%     
==========================================
  Files         204      204              
  Lines        6320     6321       +1     
==========================================
- Hits         4530     4524       -6     
- Misses       1790     1797       +7

Impacted Files	Coverage Δ
crates/env/src/api.rs	`36.92% <ø> (ø)`
crates/env/src/backend.rs	`78.12% <ø> (ø)`
crates/env/src/call/create_builder.rs	`0.00% <0.00%> (ø)`
crates/env/src/engine/off_chain/impls.rs	`47.64% <ø> (ø)`
...odegen/src/generator/as_dependency/contract_ref.rs	`100.00% <ø> (ø)`
crates/ink/src/env_access.rs	`9.09% <ø> (ø)`
crates/allocator/src/bump.rs	`85.95% <0.00%> (-3.97%)`	⬇️
crates/metadata/src/layout/mod.rs	`75.83% <0.00%> (-1.67%)`	⬇️
crates/ink/ir/src/ir/item/storage.rs	`80.00% <0.00%> (-0.65%)`	⬇️
crates/ink/ir/src/ir/item/event.rs	`86.56% <0.00%> (-0.20%)`	⬇️
... and 14 more

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

ascjones

Just the question mark about the handling of the decoding of the LangError, other than that it looks good.

crates/env/src/call/create_builder.rs

ascjones · 2022-11-29T16:41:05Z

crates/env/src/engine/on_chain/impls.rs

+            Err(ext::Error::CalleeReverted) => {
+                // We don't wrap manually with an extra `Err` like we do in the `Ok` case since the
+                // buffer already comes back in the form of `Err(LangError)`
+                let out = scale::Decode::decode(&mut &out_return_value[..])?;


Couldn't there exist a contract that would revert the contract but return Ok(()) e.g.

::ink::env::return_value::<::ink::ConstructorResult<()>>( ::ink::env::ReturnFlags::new_with_reverted(true), Ok(()), );

This could mislead the caller into handling the Ok case?

Hmm yeah this is a good observation. I've added a test that shows that you can indeed revert from a constructor and continue execution by returning Ok(AccountId).

I've also added a quick patch which asserts that the content in the buffer is an error.

I'm not entirely sure what the implications of either case are though...

In the Revert + Ok case, misleading the caller seems like the main reason to do this, but maybe there are legit reasons to do encode an Ok back? I can't think of any though.

In the Revert + Err case, the callee can encode the "wrong" LangError into the output buffer, potentially misleading the caller too, and there's nothing we can do to help there.

Either way it'll be up to the contract developer to make sure they understand that there are risks to calling other contracts.

@athei would like to hear your opinion here

The assumption is that the user is not calling return_value directly, and if they are they are using it in an ABI compatible way which is either Revert + Err(LangError) or Revert + Ok(Err(ContracErr)).

So perhaps we could return an Err(LangError) here if this ABI is not satisfied, e.g. as mentioned in #1512 (comment)

Okay maybe what we want is something along the lines of

// note changed from `AccountId` to `()` let out = ink_primitives::ConstructorResult::<()>::decode( &mut &out_return_value[..], )?; match out { // This branch when a fallible constructor returns `Err` Ok(()) => Err(ext::Error::CalleeReverted), /// This branch handles an actual `LangErr` Err(lang_err) => Ok(Err(lang_err)) }

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

Co-authored-by: Andrew Jones <[email protected]>

ascjones · 2023-01-03T12:43:08Z

Ping me when you're back and we can catch up on this and get it merged

ascjones · 2023-01-04T17:02:44Z

See use-ink/ink-workshop#31

Edit, sorry I realize this is for a call and not an instantiate. Still, it highlights a potential footgun if we are requiring the user to explicitly specify the Result<_, LangError>

HCastano · 2023-01-14T18:01:59Z

See paritytech/ink-workshop#31

Edit, sorry I realize this is for a call and not an instantiate. Still, it highlights a potential footgun if we are requiring the user to explicitly specify the Result<_, LangError>

Yeah, this is something I wanted to address with #1525

ascjones

LGTM!

The dual method API with fallible alternatives is a bit clunky, however #1591 provides a solution to that.

HCastano mentioned this pull request Nov 19, 2022

Return LangErrors from constructors #1504

Merged

2 tasks

Base automatically changed from hc-impl-lang-error-for-constructors to master November 21, 2022 21:48

Handle LangError from instantiate (fails for success case)

8796a62

This commit lets us grab what's in the output buffer after our call to `instantiate`, however we are unable to succesfully decode the `AccountId` from the success case.

HCastano force-pushed the hc-get-lang-error-from-create-builder branch from acc3f48 to 8796a62 Compare November 25, 2022 14:56

HCastano added 16 commits November 25, 2022 11:19

Change generic in CreateBuilder to be more consistent

95b80c4

Remove extra generic parameter

a49fcb7

I accidently introduced this not knowing that the generic `C` was for the return type

Remove generic return type parameter from CreateBuidler codegen

8312fa8

Hardcode assumption that instantiate returns a ConstructorResult

6e70be0

Update CreateBuilder codegen to just return Self

2fd4a82

Remove generic usage to fix formatting

4d1f2bf

Unwrap ConstructorResult in contract-ref E2E test

f107cb1

Clean up some comments

b6de9fc

Bring back the assumption that we expect an AccountId

5d221c3

Is supposed to help give better error messages if somebody uses a wrong type with the builder

Remove unused method

7d94f75

Update doc tests for new builder pattern

798f946

Clean up some comments

18d0922

Fix Clippy warning

06bba81

Fix typo

612537e

Add try_instantiate method to CreateBuilder

2175174

Remove unneeded unwrap

44a8324

HCastano changed the title ~~Handle LangError from instantiate (fails for success case)~~ Handle LangError from instantiate Nov 29, 2022

HCastano added 2 commits November 28, 2022 19:13

Remove debug logging

84d257d

Update doc test

ffcca2c

HCastano marked this pull request as ready for review November 29, 2022 00:20

HCastano requested review from cmichi and ascjones as code owners November 29, 2022 00:20

ascjones reviewed Nov 29, 2022

View reviewed changes

HCastano and others added 2 commits November 29, 2022 15:13

Fix some typos

91cd98f

Co-authored-by: Andrew Jones <[email protected]>

Mention panicking behaviour of instantiate methods

e701230

HCastano added 3 commits December 12, 2022 13:53

Add some links to env docs

0d40825

Add more docs to call-builder E2E tests

c8e5cfa

Use correct path in call-builder docs

1e323f4

HCastano requested review from xermicus and athei December 15, 2022 20:50

ascjones mentioned this pull request Jan 4, 2023

Account for LangError use-ink/ink-workshop#31

Merged

ascjones mentioned this pull request Jan 12, 2023

Release 4.0.0-rc #1590

Closed

HCastano mentioned this pull request Jan 17, 2023

Unify fallible and non fallible instantiate methods #1591

Merged

ascjones added 2 commits January 18, 2023 09:41

Merge branch 'master' into hc-get-lang-error-from-create-builder

0b31392

Use return_value() method in e2e test

9a14e21

ascjones force-pushed the hc-get-lang-error-from-create-builder branch from 09af6a2 to 9a14e21 Compare January 20, 2023 15:30

HCastano added 10 commits January 20, 2023 08:57

Merge branch 'master' into hc-get-lang-error-from-create-builder

69946de

Fix call-builder E2E test compilation

37d8e80

Fix contract-ref E2E test compilation

0c568b5

Fix some of the comment links

7d20051

Unwrap errors from default instantiate_fallible codepath

eeb7b85

Fix contract-ref E2E test

658fed1

Wrap long line

aac62db

Remove TODO

619cc71

Fix instatiation doc test

dd41485

Merge branch 'master' into hc-get-lang-error-from-create-builder

b41ca87

ascjones approved these changes Jan 23, 2023

View reviewed changes

HCastano merged commit cdfb7fe into master Jan 23, 2023

HCastano deleted the hc-get-lang-error-from-create-builder branch January 23, 2023 22:02

HCastano mentioned this pull request Jan 24, 2023

Release v4.0.0-beta.1 #1617

Merged

ascjones mentioned this pull request Jan 25, 2023

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

Merged

deuszx mentioned this pull request Jan 31, 2023

Merge main + migrate to ink4 Cardinal-Cryptography/aleph-node#898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `LangError` from instantiate #1512

Handle `LangError` from instantiate #1512

HCastano commented Nov 19, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

ascjones left a comment

ascjones Nov 29, 2022

HCastano Nov 29, 2022 •

edited

Loading

HCastano Nov 30, 2022

ascjones Dec 1, 2022 •

edited

Loading

ascjones Dec 1, 2022 •

edited

Loading

ascjones commented Jan 3, 2023

ascjones commented Jan 4, 2023 •

edited

Loading

HCastano commented Jan 14, 2023

ascjones left a comment •

edited

Loading

Handle LangError from instantiate #1512

Handle LangError from instantiate #1512

Conversation

HCastano commented Nov 19, 2022 • edited Loading

codecov-commenter commented Nov 29, 2022 • edited Loading

Codecov Report

ascjones left a comment

Choose a reason for hiding this comment

ascjones Nov 29, 2022

Choose a reason for hiding this comment

HCastano Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

HCastano Nov 30, 2022

Choose a reason for hiding this comment

ascjones Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

ascjones Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

ascjones commented Jan 3, 2023

ascjones commented Jan 4, 2023 • edited Loading

HCastano commented Jan 14, 2023

ascjones left a comment • edited Loading

Choose a reason for hiding this comment

Handle `LangError` from instantiate #1512

Handle `LangError` from instantiate #1512

HCastano commented Nov 19, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

HCastano Nov 29, 2022 •

edited

Loading

ascjones Dec 1, 2022 •

edited

Loading

ascjones Dec 1, 2022 •

edited

Loading

ascjones commented Jan 4, 2023 •

edited

Loading

ascjones left a comment •

edited

Loading