Changed the field `name` to `label` #923

xgreenx · 2021-09-16T22:17:17Z

Updated the code generation of ABI. Instead of "name" in messages, constructors, arguments, events we are using "label".
Changed the behavior of "label" for messages and constructors, now it is a string instead of an array.
For trait's messages, we will use the "TRAIT_IDENT::MESSAGE_IDENT" format of representation.

This PR is related to #900.

xgreenx · 2021-09-16T22:28:37Z

@cmichi What do you think about updating displayName to be a string instead of an array too?

cmichi · 2021-09-17T02:26:32Z

Hey nice, that you created a PR for this!

@cmichi What do you think about updating displayName to be a string instead of an array too?

:-) I actually had intended to create a ticket for this today! I agree that it's a good idea, but let's keep this to a separate issue. I'll create one where we track this.

cmichi

Looks good on first glance ‒ great that you implemented this!

Got some language remarks. I guess the PSP-22 should also be adapted at some point in the future.

cmichi · 2021-09-17T02:39:04Z

README.md

@@ -221,6 +221,7 @@ In a module annotated with `#[ink::contract]` these attributes are available:
 | `#[ink(constructor)]` | Applicable to method. | Flags a method for the ink! storage struct as constructor making it available to the API for instantiating the contract. |
 | `#[ink(payable)]` | Applicable to ink! messages. | Allows receiving value as part of the call of the ink! message. ink! constructors are implicitly payable. |
 | `#[ink(selector = "..")]` | Applicable to ink! messages and ink! constructors. | Specifies a concrete dispatch selector for the flagged entity. This allows a contract author to precisely control the selectors of their APIs making it possible to rename their API without breakage. |
+| `#[ink(label = "..")]` | Applicable to ink! messages, ink! constructors and ink! implementation blocks. | Specifies a concrete label of method/trait inside of metadata. This allows a contract author to precisely control the labeling of methods inside of ABI making it possible to rename their methods without breakage. |


Suggested change

| `#[ink(label = "..")]` | Applicable to ink! messages, ink! constructors and ink! implementation blocks. | Specifies a concrete label of method/trait inside of metadata. This allows a contract author to precisely control the labeling of methods inside of ABI making it possible to rename their methods without breakage. |

| `#[ink(label = "..")]` | Applicable to ink! messages, ink! constructors and ink! implementation blocks. | Specifies a concrete label of a method/trait inside the metadata. This allows a contract author to precisely control the labeling of methods inside of ABI making it possible to rename their methods without breakage. |

| #[ink(label = "..")] | Applicable to ink! messages, ink! constructors and ink! implementation blocks.

Why do we have the need to label ink! implementation blocks? What is the effect of it?

Marking the implementation block allows you to specify the prefix before the message or constructor.
The user can use it to remove the prefix of trait, or change the prefix of trait, or add a prefix for the impl section without the trait.

In this PR I described the case when we are using "change the prefix of trait" behavior.

I am not sure I like this feature for trait implementation blocks since I see a potential for a lot of confusion. What is an actual concrete use case there?

The actual use case is:
We have a trait with the name Erc20External. During generation of labels for methods ink! adds Erc20External:: prefix. With this prefix it generates Erc20External::transfer label. The idea is to label the impl section with the Erc20 label to generate the Erc20::transfer for the method.

We can remove the label attribute for the impl section if the label attribute of the method overrides the prefix(in the current implementation it only overrides the label of method and trait prefix affects it).

I am not sure I like this feature for trait implementation blocks since I see a potential for a lot of confusion. What is an actual concrete use case there?

@Robbepop Can you elaborate on where you see potential for confusion here? If you haven't seen yet, the issue for this PR states some more of the motivation:

For traits, ink! generates a field in the form of "name": ["BaseErc20", "new"]. The BaseErc20 hereby is a trait implemented by the contract. This exposes internal implementation details of the contract without the developer having any influence over it.

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

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

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

crates/lang/macro/src/lib.rs

crates/lang/macro/tests/ui/pass/12-attributes.rs

crates/metadata/src/specs.rs

Robbepop · 2021-09-21T06:35:02Z

@xgreenx please rebase this on the current master branch so we might be ready for another round of review and merge it.

Robbepop · 2021-09-21T07:29:05Z

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

@@ -764,6 +788,18 @@ impl TryFrom<syn::NestedMeta> for AttributeFrag {
                            }
                            return Err(format_err!(name_value, "expecteded 4-digit hexcode for `selector` argument, e.g. #[ink(selector = 0xC0FEBABE]"))
                        }
+                        if name_value.path.is_ident("label") {
+                            if let syn::Lit::Str(lit_str) = &name_value.lit {
+                                let name = lit_str.value();


This needs a check to be a valid Rust identifier.
You can check this by using

syn::parse_str::<syn::Ident>() .unwrap_or_else(|error| { panic!("encountered invalid non-identifier namespace argument: {}", error) })

Actually instead of panicking you should return a proper syn::Error via format_err! instead.

https://github.com/paritytech/ink/pull/931/files#diff-024136e95afaa59cacb0e87a4287e1e425de563db108032ba4b2d40df2c160eaR770

@Robbepop I'm waiting for your decision in the thread.
Because it will affect this part too=)

I cannot tell either what the better design is.
Let us summarize what you propose:

#[ink(label = "foo")] on an ink! implementation block is a prefix for #[ink(label = "bar")] on ink! messages in that same block. So both combined you end up with a label foo::bar.

Any #[ink(label = "bar")] on an ink! message or constructor in an ink! implementation block will overwrite the label of the ink! message or constructor entirely. Any #[ink(label = "foo")] on the ink! implementation block will be silently ignored.

I can so pros and cons and confusion with both points.
Furthermore I still cannot judge how important this feature really is. It seems to just alter some UI components.
I like the change to the metadata format with label instead of name and a simple string instead of a sequence of strings. However, I don't yet see the value behind the #[ink(label = ..)] property.

I am very unsure about #[ink(label = "foo")] on implementation blocks but I think on ink! dispatchables (constructors and messages) it is probably a good idea.
Semantically I'd prefer that if #[ink(label = "foo")] is put on some ink! constructor or message then only the label suffix is overwritten. Also we would apply the same semantics for trait impl blocks as with the other properties with the incoming PR #665 . It is no longer possible to alter the properties of ink! trait messages but applying those ink! properties like payable and selector to them will act as a guard in case the ink! trait definitions changes. So we could do the same for label. In fact the ink! trait definition then decides the labels for all implemented ink! messages. Is that a deal?

I like the idea of the #665 to not alter the properties in impl block, so I agree with the same rules for the label attribute=)
We need the ability to change the suffix of the method and the prefix of the method in the generated ABI.
Above you described how can be implemented the attribute to change the suffix, I agree with it. Now we need to decide how we can change the prefix in case of traits=)

We can try to move in this direction, but what do you think about next:
The label and selector attributes should have the same behavior.

If someone specified the label, it will use this label directly

Otherwise, it generates a label based on the path of the trait, namespace, and the name of the method(It means that the namespace attribute also will affect the label in the same way how it affects the selector).

Also, I think, that we need to introduce some mechanism to affect the name of the trait during the generation of the selectors and labels. It will allow the user to specify any part of the selector/label during the generation process.

Any #[ink(label = "bar")] on an ink! message or constructor in an ink! implementation block will overwrite the label of the ink! message or constructor entirely. Any #[ink(label = "foo")] on the ink! implementation block will be silently ignored.

This is the approach I prefer. My understanding is that the entire reason for this feature is to make it possible for a developer to control what label appears in a UI. If we still concatenate internal implementation details (like the name of the impl block) to the label it's pointless.

I agree, it is why I think that the behavior of labels and selectors must be the same(it is described above :)

@Robbepop So, what do you think?=) We only need to select the behavior and I will implement it. I can integrate it to #665 and partially review the change

cmichi · 2021-11-09T06:48:42Z

I've lost a bit track of where we are with this PR and where the discussion went.

@xgreenx Could you maybe quickly summarize what decision needs to be made for you to be able to finish the PR?

xgreenx · 2021-11-09T08:31:03Z

We need to decide do we need the label attribute or not. You think that better to add, @Robbepop thinks it is better to not add additional attributes=) My thoughts

Robbepop · 2021-11-09T08:33:05Z

We need to decide do we need the label attribute or not. You think that better to add, @Robbepop thinks it is better to not add additional attributes=) My thoughts

I am strictly against adding the label attribute. the metadata changes are ok. i see no reason in adding a feature where there is no strict requirement and where we are not 100% sure about its design.

cmichi · 2021-11-09T10:33:47Z

i see no reason in adding a feature where there is no strict requirement and where we are not 100% sure about its design.

@Robbepop Could you please reply to the points brought up in the discussion here?

cmichi · 2021-11-09T15:07:33Z

@xgreenx I've discussed it with @Robbepop and the outcome is that we'll only include the metadata changes for now and revisit the #[ink(label)] discussion at a later point. Would you be so kind to adapt your PR?

xgreenx · 2021-11-09T15:09:27Z

Sure, will adapt it for the master branch and will remove label related code=)

…For trait it is string in format "TRAIT_IDENT::METHOD_IDENT"

xgreenx · 2021-11-09T21:40:28Z

I've updated the PR. In ABI we still use "name" for storage layout and for types. Do I need to update it for storage layout? (because in the case of types we need to update scale-info)

xgreenx · 2021-11-10T07:42:44Z

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

@@ -275,11 +275,9 @@ impl Metadata<'_> {
                        as ::ink_lang::reflect::TraitMessageInfo<#local_id>>::SELECTOR
                }};
                let ret_ty = Self::generate_return_type(message.output());
+                let label = [trait_ident.to_string(), message_ident.to_string()].join("::");


Do you prefer this logic here or in from_trait_and_label on "spec" level?

What do you think about the idea that the namespace will affect the label too(not only selector)? If you agree then I will implement it like this https://github.com/paritytech/ink/blob/master/crates/lang/ir/src/ir/selector.rs#L108

It's a good point, I think for consistency reasons it would be good.

What are you referring to with from_trait_and_label?

I meant, that I removed from_trait_and_label from spec and implement the logic of concatenating(joining of trait name and method name by "::") here, on generator level.

Alright, got it. It's fine here.

Do you want to implement the namespace affecting the label as well? Could also be done as a follow-up.

I can implement that in follow-up PR=) @Robbepop What do you think about the namespace affecting the label?

cmichi · 2021-11-23T17:38:17Z

@xgreenx Could you merge master into this PR?

cmichi · 2021-11-23T17:40:30Z

In ABI we still use "name" for storage layout and for types. Do I need to update it for storage layout? (because in the case of types we need to update scale-info)

Nope, no need to adapt those.

codecov-commenter · 2021-11-23T18:01:39Z

Codecov Report

Merging #923 (1920769) into master (2ee203d) will decrease coverage by 10.57%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           master     #923       +/-   ##
===========================================
- Coverage   74.15%   63.57%   -10.58%     
===========================================
  Files         246      246               
  Lines        9258     9249        -9     
===========================================
- Hits         6865     5880      -985     
- Misses       2393     3369      +976

Impacted Files	Coverage Δ
crates/lang/codegen/src/generator/metadata.rs	`0.00% <0.00%> (-97.35%)`	⬇️
crates/metadata/src/lib.rs	`0.00% <0.00%> (ø)`
crates/metadata/src/specs.rs	`73.33% <60.00%> (+0.60%)`	⬆️
crates/lang/codegen/src/traits.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/env.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/mod.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/enforced_error.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/blake2b.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/storage.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/codegen/src/generator/arg_list.rs	`0.00% <0.00%> (-100.00%)`	⬇️
... and 34 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 2ee203d...1920769. Read the comment docs.

cmichi · 2021-11-23T18:59:07Z

@xgreenx The ink-waterfall CI stage is expected to fail, since it builds our example contracts and tries to upload them via polkadot-js. Since that UI doesn't understand the new metadata format yet the upload fails. Could you bump the metadata version of this PR to V2 in metadata/src/lib.rs?

* Changed "name" to "label". "label" field is not array, it is string. For trait it is string in format "TRAIT_IDENT::METHOD_IDENT" * Fixed test * Bump version of metadata * Fix metadata variant

cmichi requested changes Sep 17, 2021

View reviewed changes

Robbepop reviewed Sep 21, 2021

View reviewed changes

xgreenx mentioned this pull request Oct 21, 2021

Rename name in metadata to label #900

Closed

4 tasks

Changed "name" to "label". "label" field is not array, it is string. …

6356f80

…For trait it is string in format "TRAIT_IDENT::METHOD_IDENT"

xgreenx force-pushed the feature/ink-label-attribute branch from 436a62f to 6356f80 Compare November 9, 2021 21:21

xgreenx changed the title ~~New label attribute and update of ABI generation process~~ Changed the field name to label Nov 9, 2021

Fixed test

b8b862b

xgreenx commented Nov 10, 2021

View reviewed changes

Merge branch 'paritytech:master' into feature/ink-label-attribute

154e546

cmichi mentioned this pull request Nov 23, 2021

Metadata format for messages/constructors in a .contract bundle changed polkadot-js/api#4255

Closed

xgreenx added 2 commits November 23, 2021 22:11

Bump version of metadata

e99663e

Fix metadata variant

1920769

cmichi approved these changes Dec 10, 2021

View reviewed changes

cmichi merged commit a96cf87 into use-ink:master Dec 10, 2021

cmichi mentioned this pull request Dec 10, 2021

Document metadata format #940

Open

xgreenx mentioned this pull request Dec 10, 2021

Remove Multisig example contract #1075

Closed

	\| `#[ink(label = "..")]` \| Applicable to ink! messages, ink! constructors and ink! implementation blocks. \| Specifies a concrete label of method/trait inside of metadata. This allows a contract author to precisely control the labeling of methods inside of ABI making it possible to rename their methods without breakage. \|
	\| `#[ink(label = "..")]` \| Applicable to ink! messages, ink! constructors and ink! implementation blocks. \| Specifies a concrete label of a method/trait inside the metadata. This allows a contract author to precisely control the labeling of methods inside of ABI making it possible to rename their methods without breakage. \|

Changed the field name to label #923

Changed the field name to label #923

Conversation

xgreenx commented Sep 16, 2021 • edited Loading

xgreenx commented Sep 16, 2021

cmichi commented Sep 17, 2021

cmichi 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

xgreenx Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop commented Sep 21, 2021

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

Robbepop Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmichi commented Nov 9, 2021

xgreenx commented Nov 9, 2021

Robbepop commented Nov 9, 2021

cmichi commented Nov 9, 2021

cmichi commented Nov 9, 2021

xgreenx commented Nov 9, 2021

xgreenx commented Nov 9, 2021

Choose a reason for hiding this comment

cmichi Nov 23, 2021 • edited Loading

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 23, 2021

cmichi commented Nov 23, 2021

codecov-commenter commented Nov 23, 2021 • edited Loading

Codecov Report

cmichi commented Nov 23, 2021 • edited Loading

Changed the field `name` to `label` #923

Changed the field `name` to `label` #923

xgreenx commented Sep 16, 2021 •

edited

Loading

xgreenx Sep 22, 2021 •

edited

Loading

Robbepop Sep 28, 2021 •

edited

Loading

cmichi Oct 14, 2021 •

edited

Loading

cmichi Nov 23, 2021 •

edited

Loading

codecov-commenter commented Nov 23, 2021 •

edited

Loading

cmichi commented Nov 23, 2021 •

edited

Loading