Allow specifying `payable` constructors #1065

HCastano · 2021-12-02T16:30:25Z

Before this change ink! constructors were payable by default. This was because all
contracts were required to place an endowment in order to stay alive.

However, with the changes introduced in paritytech/substrate#10082 this is no
longer necessary. Instead we can allow users to specify whether or not they want their
constructors to be payable.

We'll need to wait for the Substrate PR to get merged before merging this one. This is
because if we deploy a contract without also initializing it with some value we get a
NewContractNotFunded error.

Closes #1027.

P.S, we'll also want to change endowment to value to be consistent with
pallet-contracts, but that can be done in a follow-up PR.

codecov-commenter · 2021-12-02T16:56:24Z

Codecov Report

Merging #1065 (aebeb39) into master (d9c17b6) will decrease coverage by 0.17%.
The diff coverage is 58.13%.

@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
- Coverage   78.99%   78.82%   -0.18%     
==========================================
  Files         248      251       +3     
  Lines        9346     9384      +38     
==========================================
+ Hits         7383     7397      +14     
- Misses       1963     1987      +24

Impacted Files	Coverage Δ
crates/lang/src/codegen/dispatch/execution.rs	`0.00% <0.00%> (ø)`
crates/lang/src/reflect/dispatch.rs	`0.00% <ø> (ø)`
.../tests/ui/contract/pass/constructor-non-payable.rs	`28.57% <28.57%> (ø)`
...lang/tests/ui/contract/pass/constructor-payable.rs	`28.57% <28.57%> (ø)`
...s/ui/contract/pass/constructor-payable-multiple.rs	`33.33% <33.33%> (ø)`
crates/lang/codegen/src/generator/dispatch.rs	`94.61% <100.00%> (+0.33%)`	⬆️
crates/lang/ir/src/ir/item_impl/constructor.rs	`93.84% <100.00%> (+1.53%)`	⬆️
...ates/storage/src/collections/hashmap/fuzz_tests.rs	`94.68% <0.00%> (-5.32%)`	⬇️
crates/lang/ir/src/ir/attrs.rs	`82.27% <0.00%> (-0.56%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9c17b6...aebeb39. Read the comment docs.

crates/lang/tests/ui/contract/fail/constructor-payable-invalid-1.rs

Reference: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned

Reference: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

HCastano · 2022-01-06T23:14:36Z

So UI support for this isn't quite there since polkadot-js/apps#6645 hasn't been tackled, but really the only "gotcha" here for users is that if they specify a value if deploying a contract with a non-payable constructor they get a ContractTrapped error. If we're okay with dealing with that for now I say we merge this.

athei · 2022-01-07T10:19:24Z

Does this PR also includes this information into the metadata? Currently, constructors don't have a payable field.

ascjones · 2022-01-07T10:26:33Z

It doesn't look like it has the extra metadata field, as Jaco says in that issue there should be a V3 metadata for this including the addition of this field.

It would go in this: https://github.com/paritytech/ink/blob/master/crates/metadata/src/specs.rs#L223

Some changes needed.

athei · 2022-01-07T10:31:38Z

So in this PR we need to:

Add the payable field to metadata
Bump the metadata version

HCastano · 2022-01-07T15:48:46Z

It doesn't look like it has the extra metadata field, as Jaco says in that issue there should be a V3 metadata for this including the addition of this field.

It would go in this: https://github.com/paritytech/ink/blob/master/crates/metadata/src/specs.rs#L223

Ah, okay I didn't realize Jaco was referring to the metadata on our end. Sure I can add this

cmichi · 2022-01-14T13:27:45Z

crates/lang/src/codegen/dispatch/execution.rs

+    ///
+    /// If no ink! constructor within the same ink! smart contract
+    /// is payable then this flag will be `true` since the check
+    /// then is moved before the constructor dispatch as an optimization.


Hmm, not happy about this. This possibility of payable being set to true despite no payable constructor seems to be destined for a bug. How about we reflect this information e.g. with an enum here as a wrapper around the bool?

I think there was already a small bug here actually. If there were indeed no payable messages for example, we'd never hit this check. Although to be fair, payment would have been denied in call() (maybe this was the optimization alluded to in the comment?).

Either way, this was confusing and I think it's unnecessary so I removed it.

It looks like this was meant to be part of an optimization, however it doesn't look like this is necessary anymore. If no constructor or message is payable we already do early deny payment checks in `deploy()` and `call()` respectively.

cmichi · 2022-01-17T09:43:10Z

Can you create a follow-up issue for ink-docs?

athei · 2022-01-17T09:52:53Z

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

@@ -706,7 +732,6 @@ impl Dispatch<'_> {
            );
            let accepts_payment = quote_spanned!(message_span=>
                false ||
-                !#any_message_accept_payment ||


Can we have a UI test for this: Multiple constructors which are all not payable?

HCastano · 2022-01-17T23:07:50Z

Can you create a follow-up issue for ink-docs?

Done: use-ink/ink-docs#41

* Add `payable` constructor support to IR * Add payable associated type to `DispatchableConstructorInfo` * Allow constructors to be payable in dispatcher codegen * Add UI tests * Remove test related to constructors being implicitly payable * Add some failing UI tests * Deny payments on `deploy` if there are no payable constructors * Appease Clippy * Deny payments when executing constructors * `s/message/constructor/g` * RustFmt * Address `unnecessary_to_owned` lint Reference: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned * Address `needless_borrow` lint Reference: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow * Make a note about this PR in the RELEASES doc * Allow `clippy::nonminimal_bool` for `deploy()` * Remove `any_payable_*` checks from individual Config creation It looks like this was meant to be part of an optimization, however it doesn't look like this is necessary anymore. If no constructor or message is payable we already do early deny payment checks in `deploy()` and `call()` respectively. * Add UI test for multiple non-payable constructors

HCastano added 9 commits December 2, 2021 13:34

Add payable constructor support to IR

f49184e

Add payable associated type to DispatchableConstructorInfo

325f1dc

Allow constructors to be payable in dispatcher codegen

c42e371

Add UI tests

2acf3c5

Remove test related to constructors being implicitly payable

8313263

Add some failing UI tests

fb4f136

Deny payments on deploy if there are no payable constructors

ebbff47

Appease Clippy

e07772d

Deny payments when executing constructors

aeaf383

HCastano requested review from ascjones, cmichi and Robbepop as code owners December 2, 2021 16:30

HCastano added 2 commits December 2, 2021 17:32

s/message/constructor/g

cb7188f

RustFmt

206bdac

HCastano requested a review from athei December 2, 2021 16:41

athei previously approved these changes Dec 6, 2021

View reviewed changes

cmichi reviewed Dec 6, 2021

View reviewed changes

crates/lang/tests/ui/contract/fail/constructor-payable-invalid-1.rs Show resolved Hide resolved

HCastano mentioned this pull request Dec 7, 2021

Add support for non-payable ink! constructors polkadot-js/apps#6645

Closed

HCastano added 4 commits January 6, 2022 14:35

Merge branch 'master' into hc-make-constructors-non-payable

d14cbd2

Address unnecessary_to_owned lint

d081127

Reference: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned

Address needless_borrow lint

1de66b4

Reference: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

Make a note about this PR in the RELEASES doc

9f0a8b8

Merge branch 'master' into hc-make-constructors-non-payable

4c648d0

Merge branch 'master' into hc-make-constructors-non-payable

c72a352

HCastano mentioned this pull request Jan 10, 2022

Update metadate to support payable constructors #1100

Merged

Allow clippy::nonminimal_bool for deploy()

aebeb39

This was referenced Jan 14, 2022

Cater for updated ink! metadata (w/ constructor payable) polkadot-js/api#4431

Closed

Support for ink! metadata v3 polkadot-js/api#4432

Merged

cmichi reviewed Jan 14, 2022

View reviewed changes

HCastano requested a review from cmichi January 17, 2022 02:03

cmichi approved these changes Jan 17, 2022

View reviewed changes

athei reviewed Jan 17, 2022

View reviewed changes

Add UI test for multiple non-payable constructors

d915ec0

HCastano mentioned this pull request Jan 17, 2022

Describe new constructor default payable behaviour use-ink/ink-docs#41

Closed

HCastano merged commit e646ddd into master Jan 18, 2022

HCastano deleted the hc-make-constructors-non-payable branch January 18, 2022 03:39

cmichi mentioned this pull request Jan 19, 2022

Make constructor of contract-transfer example payable #1104

Merged

xgreenx mentioned this pull request Jan 23, 2022

[rc8] examples/erc721 build upload/deployment fails with ContractTrapped #1108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying `payable` constructors #1065

Allow specifying `payable` constructors #1065

HCastano commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading

HCastano commented Jan 6, 2022

athei commented Jan 7, 2022

ascjones commented Jan 7, 2022

athei commented Jan 7, 2022

HCastano commented Jan 7, 2022

cmichi Jan 14, 2022

HCastano Jan 17, 2022

cmichi commented Jan 17, 2022

athei Jan 17, 2022

HCastano commented Jan 17, 2022

Allow specifying payable constructors #1065

Allow specifying payable constructors #1065

Conversation

HCastano commented Dec 2, 2021 • edited Loading

codecov-commenter commented Dec 2, 2021 • edited Loading

Codecov Report

HCastano commented Jan 6, 2022

athei commented Jan 7, 2022

ascjones commented Jan 7, 2022

athei commented Jan 7, 2022

HCastano commented Jan 7, 2022

cmichi Jan 14, 2022

Choose a reason for hiding this comment

HCastano Jan 17, 2022

Choose a reason for hiding this comment

cmichi commented Jan 17, 2022

athei Jan 17, 2022

Choose a reason for hiding this comment

HCastano commented Jan 17, 2022

Allow specifying `payable` constructors #1065

Allow specifying `payable` constructors #1065

HCastano commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading