-
Notifications
You must be signed in to change notification settings - Fork 480
Custom signature topic #2031
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
Custom signature topic #2031
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2031 +/- ##
==========================================
+ Coverage 53.47% 53.60% +0.12%
==========================================
Files 221 224 +3
Lines 6984 7107 +123
Branches 3082 3146 +64
==========================================
+ Hits 3735 3810 +75
- Misses 3249 3297 +48 ☔ View full report in Codecov by Sentry. |
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑These are the results when building the
Link to the run | Last update: Thu Jan 11 06:13:55 CET 2024 |
ascjones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new attribute macro #[::ink::signature_topic], we should look at extending the mechanism we already have for configuring the signature topic.
At the moment we have #[ink(anonymous)] which allows configuring the #[derive(Event)]. This is an idiomatic way to configure a derive macro, so imagine we introduce a new derive configuration attribute #[ink(signature_topic = 0x...)]. To follow this idea further, #[ink(anonymous)] is just an alias for #[ink(signature_topic = None)] (or whatever syntax we choose).
By combining these we might be able to simplify the implementation here, certainly we won't need the additional trait.
I think it should be:just 3 ways: Automatically calculated using #[ink(event) or #[ink::event] attributes So in essence we have the
Which expands to The derive then picks up the |
|
A general question - why do we need it? I'm not convinced original argument "we have this for selectors so we can have it for events" . For selectors it makes sense for backwards compatibility - imagine upgrading a contract and want to maintain compatibility with the rest of the ecosystem. For events, signature is based on the event's structure only, right? So there's no problem with backwards compatibility - if event has the same structure, old deserializers will still work. The way I see it, this feature can actually trick clients to observe, fetch, deserialize, etc. events that are not the ones they're interested in - example I'm observing |
This is useful for two reasons:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
(see my final nitpick above, also conflict needs resolving)
Summary
Closes #1838 and follows up #1827
cargo-contractorpallet-contracts?Introduces optional attribute
signature_topicto the#[ink::event]and#[ink(event)]that takes 32-byte hash string to specify a signature topic for the specific event manually.Description
It introduces a new macro attribute
#[ink(signature_topic = _)], part ofEvent, and the same argument can be used with inline events, but subject to discussion in #2046.Example
If
signature_topicis not specified, then topic will be calculated automatically, otherwise the hash string will be used.This attribute is appended automatically in
#[ink::event].Breaking changeAs part of old discussion
However, when we manually use
#[derive(Event, scale::Encode)], we must also implementGetSignatureTopicas implied above. Therefore, we must also use[ink::signature_event].However, if
#[ink(anonymous)]is used, then#[derive(event)will derive a blank implementation ofGetSignatureTopicthat returnsNone, and[ink::signature_event]should not be used.Rationale
This approach was followed due to the philosophy behind the
[ink::event]macro that simply appends necessary proc macro attributes. Therefore, the user should achieve the same result manually.Earlier, I have attempted to generate an implementation for
GetSignatureTopicinside[ink::event]but this prevented from using#[derive(Event)]independently. Therefore, if usesignature_topic = Sarg inside#[ink::event]macro that simply passes the string arg into#[ink::signature_topic(hash = S)]Specifically,
GetSignatureTopicprovides the flexibility to provide custom calculation ofSIGNATURE_TOPICwithout the need to implementEventtrait.So signature topic can be defined in 6 ways:
#[ink(event)or#[ink::event]attributes#[ink::signature_topic]when#[derive(Event)is used directly#[ink(event, signature_topic = s)for local events#[ink::event(signature_topic = s)]for shared events#[ink::signature_topic(hash = s)]for shared events with#[derive(Event)]GetSignatureTopicfor the event struct directly for shared eventsNo breaking changes have been introduced. The
signature_topicargument is totally optional.Checklist before requesting a review
CHANGELOG.md