Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make #[ink(selector = ..)] take a u32 parameter instead of a string #928

Merged
merged 7 commits into from
Sep 20, 2021

Conversation

Robbepop
Copy link
Collaborator

This PR is the result of work in #665.

Currently ink! requires a user to input a string parameter for a message or constructor selector like the following:

#[ink(selector = "0xC0DECAFE")]

This is not necessary and the implementation requires the huge regex crate that bloats the compilation times.
The PR changes the selector property to take a u32 integer instead. This allows for inputs like the following:

#[ink(selector = C0DECAFE)]
#[ink(selector = 1)]

README, example contracts and tests are updated as well.

Also warn about deprecation if a user still uses the old string parameter.
Also remove unnecessary regex dependency from ink_lang_codegen
Also update namespace parameter description.
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2021

Codecov Report

Merging #928 (26894d6) into master (4155ebe) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
- Coverage   84.24%   84.22%   -0.02%     
==========================================
  Files         172      172              
  Lines        7945     7924      -21     
==========================================
- Hits         6693     6674      -19     
+ Misses       1252     1250       -2     
Impacted Files Coverage Δ
crates/lang/ir/src/ir/item_impl/callable.rs 93.47% <ø> (ø)
crates/lang/ir/src/ir/item_impl/message.rs 96.29% <ø> (ø)
crates/lang/ir/src/ir/item_mod.rs 91.94% <ø> (ø)
crates/lang/macro/src/lib.rs 100.00% <ø> (ø)
crates/lang/ir/src/ir/attrs.rs 82.37% <87.50%> (-0.23%) ⬇️
...ates/storage/src/collections/hashmap/fuzz_tests.rs 98.93% <0.00%> (-1.07%) ⬇️

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 4155ebe...26894d6. Read the comment docs.

@Robbepop Robbepop requested review from cmichi and HCastano and removed request for cmichi September 20, 2021 06:58
| `#[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(namespace = "..")]` | Applicable to ink! trait implementation blocks. | Changes the resulting selectors of all the ink! messages and ink! constructors within the trait implementation. Allows to disambiguate between trait implementations with overlapping message or constructor names. Use only with great care and consideration! |
| `#[ink(selector = S:u32)]` | 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(namespace = N:string)]` | Applicable to ink! trait implementation blocks. | Changes the resulting selectors of all the ink! messages and ink! constructors within the trait implementation. Allows to disambiguate between trait implementations with overlapping message or constructor names. Use only with great care and consideration! |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do S and N refer to here? We should add some context above the table to clarify.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S and N are the parameter names with respect to normal Rust syntax respectively.
S is the input selector of type u32 and N is the input namespace of type string.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Could you add the change to the Unreleased section in our release notes as well?

With the next RC we then have to update the workshop and ink-docs as well. Maybe already provide PR's for this change there?

Another thing I was just wondering about: Won't there be any issue with the UI's? Because they SCALE-encode the selector as String when submitting the transaction, but now we use u32 internally.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Sep 20, 2021

Looks good!

Could you add the change to the Unreleased section in our release notes as well?

With the next RC we then have to update the workshop and ink-docs as well. Maybe already provide PR's for this change there?

Another thing I was just wondering about: Won't there be any issue with the UI's? Because they SCALE-encode the selector as String when submitting the transaction, but now we use u32 internally.

This PR does not really change how metadata is serialized in the end. It is just inspectable from a developer looking into the expanded smart contract code. The #927 PR actually makes a visible change to the produced metadata that leads to UIs having to adjust their decode (not encoding) of ink! metadata selectors if they do not already support hex-encoded and non hex-encoded integer inputs.

edit: What I said about the other PR is actually not true and UIs don't have to adjust at all.

@Robbepop
Copy link
Collaborator Author

Maybe already provide PR's for this change there?

Updating of docs will be needed with this PR. I will update the preliminary RELEASE notes.
Although we should update docs once we do an actual release imo. What do you think?

@cmichi
Copy link
Collaborator

cmichi commented Sep 20, 2021

Maybe already provide PR's for this change there?

Updating of docs will be needed with this PR. I will update the preliminary RELEASE notes.
Although we should update docs once we do an actual release imo. What do you think?

Yup, I wrote

With the next RC we then have to update the workshop and ink-docs as well. Maybe already provide PR's for this change there?

:-), I think it would be good to already have the PR's which do these changes in the pipeline, so that we can merge them right after the next release, so that there'll be no discrepancy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants