Implement wildcard selector, add `proxy` example #1020

cmichi · 2021-11-17T08:17:18Z

Closes #739.

This PR intentionally

…does not contain any wildcard selector trait support.
…does not contain an off-chain test for the new proxy example. Will be added later.

I'll add an E2E test for the ink-waterfall as a follow-up.

Pictures for the proxy/readme.md would also be nice, can also be done as a follow-up.

cmichi · 2021-11-17T08:46:55Z

examples/proxy/lib.rs

+                        .set_tail_call(true),
+                )
+                .gas_limit(0)
+                .transferred_value(self.env().transferred_balance())


We should unify this API to be just .transferred_balance everywhere.

also for constructors with their endowment?

I'd leave it there, but in this case I think it's clear that it should be streamlined.

codecov-commenter · 2021-11-17T09:00:55Z

Codecov Report

Merging #1020 (c297f50) into master (b2256d9) will increase coverage by 13.58%.
The diff coverage is 67.40%.

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

@@             Coverage Diff             @@
##           master    #1020       +/-   ##
===========================================
+ Coverage   63.52%   77.10%   +13.58%     
===========================================
  Files         246      248        +2     
  Lines        9260     9381      +121     
===========================================
+ Hits         5882     7233     +1351     
+ Misses       3378     2148     -1230

Impacted Files	Coverage Δ
crates/lang/ir/src/ir/trait_def/item/trait_item.rs	`89.06% <0.00%> (+43.03%)`	⬆️
...ests/ui/contract/pass/message-wildcard-selector.rs	`12.50% <12.50%> (ø)`
.../ui/contract/pass/constructor-wildcard-selector.rs	`16.66% <16.66%> (ø)`
crates/lang/codegen/src/generator/dispatch.rs	`94.28% <50.00%> (+94.28%)`	⬆️
crates/lang/ir/src/ir/attrs.rs	`82.59% <74.50%> (+3.59%)`	⬆️
crates/lang/ir/src/ir/item_mod.rs	`89.71% <76.92%> (+29.98%)`	⬆️
crates/lang/ir/src/ir/item_impl/callable.rs	`91.91% <100.00%> (+14.59%)`	⬆️
crates/lang/ir/src/ir/item_impl/constructor.rs	`92.30% <100.00%> (+7.56%)`	⬆️
crates/lang/ir/src/ir/item_impl/message.rs	`92.13% <100.00%> (+16.23%)`	⬆️
crates/lang/ir/src/ir/trait_def/item/mod.rs	`90.16% <100.00%> (+5.03%)`	⬆️
... and 58 more

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 b2256d9...21bb558. Read the comment docs.

paritytech-ci · 2021-11-17T09:14:16Z

🦑 📈 ink! Example Contracts ‒ Size Change Report 📉 🦑

These are the results of building the examples/* contracts from this branch with cargo-contract 0.15.0:

Link to the run | Last update: Wed Nov 24 07:31:18 CET 2021

examples/proxy/lib.rs

HCastano

I only took a look at the example contract and UI tests. Will take a look at the codegen tomorrow. Looking good though 👍

examples/proxy/lib.rs

HCastano · 2021-11-17T14:56:40Z

crates/lang/tests/ui/contract/fail/message-selector-invalid-type-01.stderr

@@ -1,4 +1,4 @@
-error: #[ink(selector = ..)] attributes with string inputs are deprecated. use an integer instead, e.g. #[ink(selector = 1)] or #[ink(selector = 0xC0DECAFE)].
+error: #[ink(selector = ..)] attributes with string inputs are deprecated (except for the wildcard selector "_"). use an integer instead, e.g. #[ink(selector = 1)] or #[ink(selector = 0xC0DECAFE)].


This is a shame, would've be nice to have consistent syntax. Can you expand on the issue you mentioned in the PR description as to why we weren't able to have selector = _?

Yeah, so the issue is here. We would need access to _ there.

But the crate which we use for parsing attributes a la #[ink(key = "value")] does not support parsing verbatims such as _. Specifically the type syn::Meta::NameValue which we use as part of the syn::parse_meta return value does not support this.

I briefly looked into syn::parse_meta, but it was nothing trivial. Maybe we could also do some pre-processing, so that selector = _ is transformed into selector = "_" before passing it to syn::parse_meta, but I haven't looked into that.

Or you can implement your own parse_meta=) You can copy the main logic from syn + add support of _=)

README.md

RELEASES.md

Co-authored-by: Hernando Castano <[email protected]>

examples/proxy/lib.rs

Robbepop · 2021-11-18T14:48:38Z

This PR intentionally

* …does not contain any wildcard selector trait support. I will create a follow-up issue for this.

This feature is highly unwanted since it would heavily interfere with trait modularity. We should make sure that traits never allow for wildcard selectors.

Robbepop

Haven't reviewed the entire thing, yet, however, I have to say that this PR goes a very long way in making sure that a wildcard selector is something completely different from a selector whereas the beauty in the original design actually made a lot of sense from the fact that we can treat both as different kinds of selectors.
So instead of putting is_wildcard_selector everywhere it would probably a more elegant design to make ir::Selector an enum that can represent a concrete selector or a wildcard selector.

RELEASES.md

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

crates/lang/ir/src/ir/item_impl/callable.rs

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

crates/lang/ir/src/ir/item_impl/message.rs

HCastano

Nice!

crates/lang/ir/src/ir/attrs.rs

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

crates/lang/ir/src/ir/item_impl/message.rs

Co-authored-by: Robin Freyler <[email protected]> Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>

Co-authored-by: Robin Freyler <[email protected]>

HCastano

I haven't looked at all the new changes just yet, will finish this up tomorrow

examples/proxy/lib.rs

HCastano · 2021-11-23T16:26:40Z

crates/lang/tests/ui/contract/pass/constructor-wildcard-selector.rs

+    pub struct Contract {}
+
+    impl Contract {
+        #[ink(constructor, selector = _)]


Are there any invariants around constructors only being able to be run once? If so, then we shouldn't allow this

Can you elaborate on this please? I cannot follow.

crates/lang/tests/ui/contract/fail/trait-message-wildcard-selector.rs

crates/lang/ir/src/ir/attrs.rs

Co-authored-by: Hernando Castano <[email protected]>

This reverts commit 00f360d.

cmichi · 2021-11-24T06:03:54Z

examples/proxy/lib.rs

+        pub fn change_forward_address(
+            &mut self,
+            new_address: AccountId,
+        ) -> Result<(), NotAdminErr> {


@Robbepop While running the waterfall tests for this example, I found that since I switched from assert! to returning Result::Err the transaction is no longer reverted ‒ it always results in ExtrinsicFailed.

If you don't see anything that I missed I'll consider this a bug, revert to assert! and create a follow-up issue.

Hmm, I tested that on Europa 0.3.6 and it is reverted.

Another question, how the user can forward some data via polkadot-js?=) Because I can only read that=D

Hmm, could be that substrate-contracts-node is somewhat outdated.

Found it, the UI doesn't reflect this yet: polkadot-js/apps#6465.

Another question, how the user can forward some data via polkadot-js?=) Because I can only read that=D

@xgreenx You have to switch the metadata of this contract to some other contract. This can be done by clicking the "Add an existing contract" button, entering the proxy smart contract address and upload a different .contract file there. The UI will then use the ABI of this contract to interact with the proxy.

This reverts commit 7698ad5. This reverts commit fbc6097. This reverts commit d55c84b.

cmichi · 2021-11-24T13:54:21Z

I've reverted returning an Err to the assert! for now, since the UI doesn't support displaying this yet and hence ink-waterfall can't test for it. Will add it back once the UI exposes it.

* Implement wildcard selector, add `proxy` example * Minor code and text improvements * Remove code * Fix function name * Add unit tests for lang parsing * Clarify where state resides * Apply suggestions from code review Co-authored-by: Hernando Castano <[email protected]> * Implement comments * Apply suggestions from code review Co-authored-by: Robin Freyler <[email protected]> Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> * Pre-process `_` to `"_"` * Update String error message * Rename `is_wildcard_selector` to `has_wildcard_selector` * Move `_` to `"_"` transformation to lower layer * Update test fixtures * Update error message * Update test fixtures * Migrate to enum `SelectorOrWildcard` * Allow wildcard selectors for constructors * Fix tests * Include clippy suggestion * Fix error output matching * Apply suggestions from code review Co-authored-by: Robin Freyler <[email protected]> * Fix suggestions * Implement reviewer suggestions * Improve matching * Include clippy suggestion * Update test fixture * Detect multiple wildcard selectors in constructors * Apply suggestions from code review Co-authored-by: Hernando Castano <[email protected]> * Rename enum variant `Selector` to `UserProvided` * Return `Result:Err` instead of asserting * Implement suggestions * Revert "Return `Result:Err` instead of asserting" This reverts commit 00f360d. * Apply `cargo fmt` * Disable overflow checks for `proxy` example * Return `Result:Err` instead of asserting * Fix import path * Fix import path * Fix import path * Revert "Return `Result:Err` instead of asserting" This reverts commit 7698ad5. This reverts commit fbc6097. This reverts commit d55c84b. Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Robin Freyler <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>

Implement wildcard selector, add proxy example

64901a9

cmichi requested review from ascjones and HCastano November 17, 2021 08:17

cmichi added 2 commits November 17, 2021 09:24

Minor code and text improvements

2eaf655

Remove code

bd7eea1

cmichi requested a review from athei November 17, 2021 08:38

Fix function name

7b7c43b

cmichi commented Nov 17, 2021

View reviewed changes

xgreenx reviewed Nov 17, 2021

View reviewed changes

examples/proxy/lib.rs Outdated Show resolved Hide resolved

cmichi added 2 commits November 17, 2021 10:51

Add unit tests for lang parsing

4677a59

Clarify where state resides

fbe82d2

HCastano reviewed Nov 17, 2021

View reviewed changes

Michael Müller and others added 2 commits November 17, 2021 16:15

Apply suggestions from code review

2e633e3

Co-authored-by: Hernando Castano <[email protected]>

Implement comments

23e2baf

athei approved these changes Nov 18, 2021

View reviewed changes

examples/proxy/lib.rs Outdated Show resolved Hide resolved

Robbepop reviewed Nov 18, 2021

View reviewed changes

HCastano approved these changes Nov 18, 2021

View reviewed changes

crates/lang/ir/src/ir/attrs.rs Outdated Show resolved Hide resolved

crates/lang/ir/src/ir/item_impl/constructor.rs Outdated Show resolved Hide resolved

crates/lang/ir/src/ir/item_impl/message.rs Outdated Show resolved Hide resolved

Michael Müller and others added 10 commits November 22, 2021 16:01

Apply suggestions from code review

75886af

Co-authored-by: Robin Freyler <[email protected]> Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>

Pre-process _ to "_"

0299f4d

Update String error message

1d102b0

Merge branch 'master' into cmichi-implement-wildcard-selector

7f1a7ea

Rename is_wildcard_selector to has_wildcard_selector

7dba798

Move _ to "_" transformation to lower layer

66a3455

Update test fixtures

f643a30

Update error message

1b0c396

Update test fixtures

5cb1f3c

Migrate to enum SelectorOrWildcard

ee3d140

Michael Müller and others added 8 commits November 23, 2021 11:45

Apply suggestions from code review

17067e7

Co-authored-by: Robin Freyler <[email protected]>

Fix suggestions

a7e624a

Implement reviewer suggestions

f1429cf

Merge branch 'master' into cmichi-implement-wildcard-selector

0b7cf6e

Improve matching

cc078ea

Include clippy suggestion

c2a2697

Update test fixture

99a4ff8

Detect multiple wildcard selectors in constructors

a2ba30a

HCastano reviewed Nov 23, 2021

View reviewed changes

Michael Müller and others added 9 commits November 23, 2021 18:20

Apply suggestions from code review

9f58738

Co-authored-by: Hernando Castano <[email protected]>

Rename enum variant Selector to UserProvided

1a7b33c

Return Result:Err instead of asserting

00f360d

Implement suggestions

1bc3bb6

Revert "Return Result:Err instead of asserting"

98da359

This reverts commit 00f360d.

Apply cargo fmt

b498308

Disable overflow checks for proxy example

36a374c

Return Result:Err instead of asserting

d55c84b

Fix import path

fbc6097

cmichi commented Nov 24, 2021

View reviewed changes

cmichi added 2 commits November 24, 2021 07:04

Fix import path

7698ad5

Fix import path

c297f50

cmichi mentioned this pull request Nov 24, 2021

Add handling for entries which exist only in old or new CSV HCastano/csv-comparator#1

Merged

Revert "Return Result:Err instead of asserting"

21bb558

This reverts commit 7698ad5. This reverts commit fbc6097. This reverts commit d55c84b.

cmichi merged commit d2f19d9 into master Nov 24, 2021

cmichi deleted the cmichi-implement-wildcard-selector branch November 24, 2021 13:54

This was referenced Nov 25, 2021

Specialize codegen for case of exactly one ink! message/constructor + that one being marked as wildcard #1046

Open

Make fire() return ! in case of tail-call flag being set #1047

Open

cmichi mentioned this pull request Sep 27, 2022

Member Request polkadot-fellows/seeding#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement wildcard selector, add `proxy` example #1020

Implement wildcard selector, add `proxy` example #1020

cmichi commented Nov 17, 2021 •

edited

Loading

cmichi Nov 17, 2021

Robbepop Nov 23, 2021

cmichi Nov 23, 2021

codecov-commenter commented Nov 17, 2021 •

edited

Loading

paritytech-ci commented Nov 17, 2021 •

edited

Loading

HCastano left a comment

HCastano Nov 17, 2021

cmichi Nov 17, 2021 •

edited

Loading

xgreenx Nov 17, 2021

Robbepop commented Nov 18, 2021

Robbepop left a comment

HCastano left a comment

HCastano left a comment

HCastano Nov 23, 2021

Robbepop Nov 23, 2021

cmichi Nov 24, 2021

xgreenx Nov 24, 2021

cmichi Nov 24, 2021

cmichi Nov 24, 2021

cmichi commented Nov 24, 2021

		@@ -1,4 +1,4 @@
		error: #[ink(selector = ..)] attributes with string inputs are deprecated. use an integer instead, e.g. #[ink(selector = 1)] or #[ink(selector = 0xC0DECAFE)].
		error: #[ink(selector = ..)] attributes with string inputs are deprecated (except for the wildcard selector "_"). use an integer instead, e.g. #[ink(selector = 1)] or #[ink(selector = 0xC0DECAFE)].

Implement wildcard selector, add proxy example #1020

Implement wildcard selector, add proxy example #1020

Conversation

cmichi commented Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2021 • edited Loading

Codecov Report

paritytech-ci commented Nov 17, 2021 • edited Loading

🦑 📈 ink! Example Contracts ‒ Size Change Report 📉 🦑

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop commented Nov 18, 2021

Robbepop left a comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi commented Nov 24, 2021

Implement wildcard selector, add `proxy` example #1020

Implement wildcard selector, add `proxy` example #1020

cmichi commented Nov 17, 2021 •

edited

Loading

codecov-commenter commented Nov 17, 2021 •

edited

Loading

paritytech-ci commented Nov 17, 2021 •

edited

Loading

cmichi Nov 17, 2021 •

edited

Loading