-
Notifications
You must be signed in to change notification settings - Fork 432
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
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 |
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-contract
orpallet-contracts
?Introduces optional attribute
signature_topic
to 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_topic
is 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 implementGetSignatureTopic
as implied above. Therefore, we must also use[ink::signature_event]
.However, if
#[ink(anonymous)]
is used, then#[derive(event)
will derive a blank implementation ofGetSignatureTopic
that 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
GetSignatureTopic
inside[ink::event]
but this prevented from using#[derive(Event)]
independently. Therefore, if usesignature_topic = S
arg inside#[ink::event]
macro that simply passes the string arg into#[ink::signature_topic(hash = S)]
Specifically,
GetSignatureTopic
provides the flexibility to provide custom calculation ofSIGNATURE_TOPIC
without the need to implementEvent
trait.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)]
GetSignatureTopic
for the event struct directly for shared eventsNo breaking changes have been introduced. The
signature_topic
argument is totally optional.Checklist before requesting a review
CHANGELOG.md