Custom signature topic - Githubissues

SkymanOne commented 10 months ago

Summary

Closes #1838 and follows up #1827

[n] y/n | Does it introduce breaking changes?
[n] y/n | Is it dependant on the specific version of cargo-contract or pallet-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 of Event, and the same argument can be used with inline events, but subject to discussion in #2046.

Example

#[derive(ink::Event, scale::Encode)]
#[ink(signature_topic = "1111111111111111111111111111111111111111111111111111111111111111")]
pub struct MyCustomSignatureEvent {
     pub field: u32,
     pub topic: [u8; 32],
}

assert_eq!(Some([17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17]),
     <MyCustomSignatureEvent as ink::env::Event>::SIGNATURE_TOPIC)

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 change~~

## As part of old discussion However, when we manually use `#[derive(Event, scale::Encode)]`, we must also implement `GetSignatureTopic` as implied above. Therefore, we must also use `[ink::signature_event]`. ```rust #[derive(::ink::Event, ::scale::Encode, ::scale::Decode)] #[cfg_attr(feature = "std", derive(::ink::EventMetadata))] #[ink::signature_topic] pub struct Flipped { .. } ``` However, if `#[ink(anonymous)]` is used, then `#[derive(event)` will derive a blank implementation of `GetSignatureTopic` that returns `None`, and `[ink::signature_event]` should not be used. ```rust #[derive(::ink::Event, ::scale::Encode, ::scale::Decode)] #[cfg_attr(feature = "std", derive(::ink::EventMetadata))] #[ink(anonymous)] pub struct Flipped { .. } ``` ### 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]`](https://github.com/paritytech/ink/blob/48cec1f4a4679e2f8e851c991dad744450b88071/crates/ink/codegen/src/generator/event.rs#L56) but this prevented from using `#[derive(Event)]` independently. Therefore, if use `signature_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 of `SIGNATURE_TOPIC` without the need to implement `Event` trait. ```rust impl GetSignatureTopic for Flipper { const SIGNATURE_TOPIC: Option<[u8; 32]> = } #[derive(::ink::Event, ::scale::Encode, ::scale::Decode)] #[cfg_attr(feature = "std", derive(::ink::EventMetadata))] pub struct Flipped { .. } ``` So signature topic can be defined in 6 ways: 1. Automatically calculated using `#[ink(event)` or `#[ink::event]` attributes 2. Automatically calculated with `#[ink::signature_topic]` when `#[derive(Event)` is used directly 3. Manually specified with `#[ink(event, signature_topic = s)` for local events 4. Manually specified with `#[ink::event(signature_topic = s)]` for shared events 5. Manually specified with `#[ink::signature_topic(hash = s)]` for shared events with `#[derive(Event)]` 6. Manually specified by implementing `GetSignatureTopic` for the event struct directly for shared events

No breaking changes have been introduced. The signature_topic argument is totally optional.

Checklist before requesting a review

[x] My code follows the style guidelines of this project
[x] I have added an entry to CHANGELOG.md
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have added tests that prove my fix is effective or that my feature works
[x] Any dependent changes have been merged and published in downstream modules

codecov-commenter commented 10 months ago

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Comparison is base (db5600e) 53.47% compared to head (c5e5e9f) 53.60%. Report is 11 commits behind head on master.

Files	Patch %	Lines
crates/ink/ir/src/ir/event/signature_topic.rs	36.11%	23 Missing :warning:
crates/ink/macro/src/event/mod.rs	74.35%	20 Missing :warning:
crates/ink/ir/src/ir/attrs.rs	79.16%	5 Missing :warning:
crates/ink/ir/src/ir/event/config.rs	75.00%	4 Missing :warning:
crates/ink/codegen/src/generator/event.rs	0.00%	3 Missing :warning:
.../ink/tests/ui/event/pass/custom_signature_works.rs	25.00%	3 Missing :warning:
...sts/ui/event/pass/inline_custom_signature_works.rs	25.00%	3 Missing :warning:
crates/ink/ir/src/ir/event/mod.rs	91.66%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 10 months ago

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract and comparing them to ink! master:

Contract	Upstream Size (kB)	PR Size (kB)	Change
basic-contract-caller	3.207	3.207	:heavy_minus_sign:
basic-contract-caller/other-contract	1.581	1.581	:heavy_minus_sign:
call-builder-return-value	8.904	8.904	:heavy_minus_sign:
call-runtime	2.013	2.013	:heavy_minus_sign:
conditional-compilation	1.453	1.453	:heavy_minus_sign:
contract-storage	7.336	7.336	:heavy_minus_sign:
contract-terminate	1.336	1.336	:heavy_minus_sign:
contract-transfer	1.693	1.693	:heavy_minus_sign:
custom-allocator	7.652	7.652	:heavy_minus_sign:
dns	7.321	7.321	:heavy_minus_sign:
e2e-call-runtime	1.302	1.302	:heavy_minus_sign:
e2e-runtime-only-backend	1.879	1.879	:heavy_minus_sign:
erc1155	14.123	14.123	:heavy_minus_sign:
erc20	6.918	6.918	:heavy_minus_sign:
erc721	9.816	9.816	:heavy_minus_sign:
events	5.264	5.264	:heavy_minus_sign:
flipper	1.637	1.637	:heavy_minus_sign:
incrementer	1.504	1.504	:heavy_minus_sign:
lang-err-integration-tests/call-builder-delegate	2.561	2.561	:heavy_minus_sign:
lang-err-integration-tests/call-builder	5.087	5.087	:heavy_minus_sign:
lang-err-integration-tests/constructors-return-value	1.987	1.987	:heavy_minus_sign:
lang-err-integration-tests/contract-ref	4.568	4.568	:heavy_minus_sign:
lang-err-integration-tests/integration-flipper	1.815	1.815	:heavy_minus_sign:
lazyvec-integration-test	4.553	4.553	:heavy_minus_sign:
mapping-integration-tests	7.919	7.919	:heavy_minus_sign:
mother	12.756	12.756	:heavy_minus_sign:
multi-contract-caller	6.155	6.155	:heavy_minus_sign:
multi-contract-caller/accumulator	1.378	1.378	:heavy_minus_sign:
multi-contract-caller/adder	1.908	1.908	:heavy_minus_sign:
multi-contract-caller/subber	1.928	1.928	:heavy_minus_sign:
multisig	21.621	21.621	:heavy_minus_sign:
payment-channel	5.653	5.653	:heavy_minus_sign:
sr25519-verification	1.148	1.148	:heavy_minus_sign:
static-buffer	1.649	1.649	:heavy_minus_sign:
trait-dyn-cross-contract-calls	2.706	2.706	:heavy_minus_sign:
trait-dyn-cross-contract-calls/contracts/incrementer	1.549	1.549	:heavy_minus_sign:
trait-erc20	7.294	7.294	:heavy_minus_sign:
trait-flipper	1.453	1.453	:heavy_minus_sign:
trait-incrementer	1.614	1.614	:heavy_minus_sign:
upgradeable-contracts/delegator	3.152	3.152	:heavy_minus_sign:
upgradeable-contracts/delegator/delegatee	1.613	1.613	:heavy_minus_sign:
upgradeable-contracts/set-code-hash	1.747	1.747	:heavy_minus_sign:
upgradeable-contracts/set-code-hash/updated-incrementer	1.726	1.726	:heavy_minus_sign:
wildcard-selector	2.852	2.852	:heavy_minus_sign:

Link to the run | Last update: Thu Jan 11 06:13:55 CET 2024

ascjones commented 10 months ago

So signature topic can be defined in 6 ways:

I think it should be:just 3 ways:

Automatically calculated using #[ink(event) or #[ink::event] attributes ~~Automatically calculated with #[ink::signature_topic] when #[derive(Event) is used directly~~ ~~Manually specified with #[ink(event, signature_topic = s) for local events~~ Manually specified with #[ink::event(signature_topic = s)] for shared events ~~Manually specified with #[ink::signature_topic(hash = s)] for shared events with #[derive(Event)]~~ ^^ Instead: #[derive(Event)] with #[ink(signature_topic = 0x)] ~~Manually specified by implementing GetSignatureTopic for the event struct directly for shared events~~

So in essence we have the

#[ink::event(signature_topic = 0x...)]]

Which expands to

#[derive(Event)]
#[ink(signature_topic = 0x...)]

The derive then picks up the ink attribute, so exactly how anonymous currently works (IIRC).

deuszx commented 10 months ago

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 PSP22::Transfer events with signature XYZ and there are contracts using custom signature topic XYZ even though they're not PSP22 contracts or not emitting PSP22::Transfer events.