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

Events 2.0 #1827

Merged
merged 152 commits into from
Jul 24, 2023
Merged

Events 2.0 #1827

merged 152 commits into from
Jul 24, 2023

Conversation

ascjones
Copy link
Collaborator

@ascjones ascjones commented Jun 26, 2023

Rework of event definitions:

  • decouples events from the #[ink::contract] macro and changes the topic calculation, allowing events to be shared between contracts.
  • changes how topics are calculated.

Closes #809 and supersedes #1243.

New macros

Events can now be defined independently of contracts using the new event attribute macro:

#[ink::event]
pub struct Flipped {
    pub value: bool,
}

This event can now be imported and emitted by any contract e.g.

self.env().emit_event(event_def::Flipped { value: self.value })

Derives

The ink::event attribute macro simply expands to underlying derive macros for the following traits:

  • Event (formerly Topics): captures the topics of the event when it is emitted at runtime.
  • EventMetadata: returns the metadata for the event definition.

e.g.

#[ink::event]
pub struct Flipped { .. }

expands to

#[derive(::ink::Event, ::scale::Encode, ::scale::Decode)]
#[cfg_attr(feature = "std", derive(::ink::EventMetadata))]
pub struct Flipped { .. }

Backwards compatibility

The legacy syntax of "inline" event definitions using the #[ink(event)] syntax will continue to work, no code changes in existing contracts are required to update to the new syntax.

Topics changes

How topics are calculated has changed, so this is a breaking change for any client code which uses topics to filter events.

Signature topic

Currently, the signature topic of an event was calculated by <contract_name>::<event_name> e.g. "Erc20::Transfer", which in turn was hashed if encoded len > 32 bytes.

Now, we mimic the approach of Solidity events of hashing the name of the event and the concatenated field types e.g. given

    #[ink::event]
    pub struct Transfer {
        #[ink(topic)]
        from: Option<AccountId>,
        #[ink(topic)]
        to: Option<AccountId>,
        value: Balance,
    }

Produces the signature of:

Transfer(Option<AccountId>, Option<AccountId>, Balance)

Which is then hashed with blake2x256 to produce the signature topic. This is calculated by the macro at compile time, and can be accessed statically with <Transfer as Event>::SIGNATURE_TOPIC. If the event is annotated with #[ink(anonymous)] attribute then the value of this will be None and no signature topic is published.

Field type name variation

In Rust, the same concrete type can be represented in different ways via type aliases and associated types e.g. u32, MyAlias, T::AssocType can all represent the same concrete type but with different names in the source code. This can result in two events with the same name and the same field types having different signature topics. This issue is discussed further with @xgreenx #1243 (comment).

In the end I decided that it is an acceptable compromise that two binary compatible events would have different signature topics. It will be important to document this fact, where e.g. for block explorers which want to index on common events such as Transfer that there is a canonical definition of the event which must have matching field type names.

Field topics

Previously field topics were calculated by concatenating the path to the field e.g. Erc20::Transfer::from with the value of the field, then hashing it (if > 32 bytes).

However with this encoding topic values are often opaque, i.e. from a topic value on it's own there is no way to know the actual value of the field in the topic because it has been hashed. Even if it is < 32 bytes and not been hashed, the path of the field prefixed the value, so the value itself can not easily be separated.

Specifically it is a problem for topics with the AccountId type, which is often indexed. Indexers such as block explorers might want a view which shows all events affecting a given AccountId. With the current topics encoding, this would be difficult, requiring the metadata for all known events and having to precalculate hashes for topics with known account ids.

With the new topics, the value of the event field itself (if <= 32 bytes) is the topic. In the case of 32 byte account ids this allows subscribing to a topic for a given account id and then it is guaranteed to receive all events which relate to that account.

It is still possible to filter those events which are from a specific event type, by combining with the filter with the signature topic. The topics data structure in a block is effectively a map of topics to event indices i.e. HashSet<Hash, Vec<u32>>>. So a filter would apply a union to the event indices of the combined topics.

This is all similar to how topics/logs work on EVM chains.

Fields with the Option type

Option fields are a special case. If a topic field value is None then it pushes the 0x0000... topic, the same as if None was encoded and converted to [u8; 32].

However, the difference is if there is a value present, instead of encoding the Option<T> it encodes the value of T and uses that as a topic. This is a special case for fields of type T and Option<T> to have matching topic values. Otherwise a suffix of 1u8 for the encoded Some value would prevent the topics being unified. And in the case of the AccountId example above it would result in unnecessary hashing because the length of the encoded value would now be 33 bytes.

Metadata

When events were all defined within the ink::contract macro it was easy to gather the metadata for all possible events being raised by a contract.

However with "shared" events now able to be defined and raised anywhere inside or outside of the ink::contract definition, the challenge was how to gather all those possible events together and put them in the metadata.

In the end I have used https://github.com/dtolnay/linkme for link time concatenation of event metadata into the EVENTS global static. See https://github.com/paritytech/ink/blob/ca861ae7eab19625e3e212be329b185a6219521f/crates/metadata/src/lib.rs#L164

See original discussion #1243 (comment)

Topics Validation

Max environment topics.

Previously there were compile time checks to ensure that any given event did not have more topics than the max allowed for the given environment. This prevents a possible runtime error at compile time.

However with the event codegen being separated from the contract itself, this compile time check became more difficult, primarily because of the limitation of not being able to assert trait constant values of type parameters (i.e. ::MAX_EVENT_TOPICS)`.

I was able to find a solution to that by using a const type parameter and some codegen, but I found the tradeoffs unsatisfactory. So I moved the validation to the metadata generation stage, where it became a simple check at metadata generation time to see whether the number of topics exceeded the environment specification.

Signature topic collisions

The metadata generation step also ensures that in the scope of the contract, signature topics are unique. This is required (see below) to decide the type of the event to decode when dealing with contract events.

Consuming Events

Previously the ink::contract macro stitched all the #[ink(event)] definitions into a top level generated enum. The way to decode any events originating from the contract would be to use the enum encoding of the first byte to determine which event variant to decode to.

Now, the signature topics must be used to determine which event a contract has emitted. See use-ink/cargo-contract#1184.

Contract Sizes

The simplification of topics codegen should reduce the contract sizes.

With ink-waterfall currently not working, the automatic checking of contract size differences is not available. Here are some examples to demonstrate some size reductions:

erc20 7.5K -> 7.1K = -5.3%
erc1155 16.9K -> 15.8K = -6.5%

Naturally reductions depend on how many events are being defined in each contract, so would be good to take that into account to.

@ascjones
Copy link
Collaborator Author

ascjones commented Jul 13, 2023

I want to highlight that the current implementation also includes all events defined in the crate if we import something from this crate. #1842 shows it by failing the collects_specs_for_all_linked_and_used_events test.

We discussed the potential solution to this problem here.

In the first iteration I implemented this solution, see the ink side and the cargo-contract side.

However I decided that this approach was introducing some non obvious coupling between the ink and cargo-contract implementations, and was slightly too convoluted.

The implementation now is at least "encapsulated" in ink (i.e. we could change internally e.g. to life-before-main inventory, similar to your original idea).

Of course the tradeoff is that if you reference a crate with an event but don't emit it, it still gets used in the metadata, as you demonstrate in #1842. However I would say that having an event in the metadata that could possibly be raised but is never raised is not such a big issue. It is even possible with any solution to have event which is emitted in a logically unreachable code branch it if balance > MAX_SUPPLY { self.env().emit_event(UnreachableEvent) } which would be included in the metadata anyway.

So on balance I have chosen the simpler solution.

@xgreenx
Copy link
Collaborator

xgreenx commented Jul 13, 2023

Of course the tradeoff is that if you reference a crate with an event but don't emit it, it still gets used in the metadata, as you demonstrate in #1842. However I would say that having an event in the metadata that could possibly be raised but is never raised is not such a big issue. It is even possible with any solution to have event which is emitted in a logically unreachable code branch it if balance > MAX_SUPPLY { self.env().emit_event(UnreachableEvent) } which would be included in the metadata anyway.

So on balance I have chosen the simpler solution.

I was happy with this approach from the beginning #1243 (comment) =D I only highlighted that the problem is not solved(we discussed it during the previous iteration, and I thought is our final goal to solve it)=)

* Usage of the crate includes defined there events

* Inclusion of the event from `event_def_unused` is expected
@ascjones
Copy link
Collaborator Author

I was happy with this approach from the beginning #1243 (comment) =D I only highlighted that the problem is not solved(we discussed it during the previous iteration, and I thought is our final goal to solve it)=)

Yes you were right all along :)

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

@cmichi @SkymanOne The pull request is ready for a final review=) It looks good to me.

@ascjones
Copy link
Collaborator Author

ascjones commented Jul 21, 2023

While we wait, here is the comparison of contract sizes.

Used script from #1851, run on master and on this branch and then pass the resulting files through awk:

awk 'NR==FNR {a[$1]=$2; next} $1 in a {print "|" $1 "|" a[$1] "|" $2 "|" (($2 - a[$1]) / a[$1]) * 100 "|"}' master shared-events-redux
Contract master this PR diff %
trait_erc20 7887 7407 -6.08596
call_builder_return_value 8952 8952 0
wildcard_selector 2496 2496 0
psp22_extension 7012 7012 0
rand_extension 2935 2734 -6.84838
e2e_call_runtime 1000 1000 0
multisig 23387 22074 -5.61423
dns 8797 7879 -10.4354
erc20 7487 7072 -5.54294
conditional_compilation 1176 1176 0
erc721 11277 10427 -7.53747
call_runtime 1781 1781 0
custom_allocator 8216 8216 0
erc1155 16893 15804 -6.44646
integration_flipper 1599 1599 0
call_builder 5179 5179 0
call_builder_delegate 2436 2436 0
constructors_return_value 1779 1779 0
contract_ref 4710 4710 0
updated_incrementer 1458 1458 0
trait_flipper 1176 1176 0
mapping_integration_tests 3346 3346 0
accumulator 1052 1052 0
adder 1710 1710 0
multi_contract_caller 6103 6103 0
subber 1727 1727 0
contract_transfer 1441 1441 0
contract_terminate 1149 1149 0
flipper 1403 1403 0
payment_channel 5942 5905 -0.622686
other_contract 1295 1295 0
basic_contract_caller 3117 3117 0
trait_incrementer 1274 1274 0
trait_incrementer_caller 2569 2569 0
custom_environment 2548 1876 -26.3736
incrementer 1215 1215 0
mother 10410 10390 -0.192123

Copy link
Contributor

@SkymanOne SkymanOne 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 to me! Congrats on the PR. Just one last small nitpick

crates/ink/macro/src/event/mod.rs Outdated Show resolved Hide resolved
@ascjones ascjones merged commit 9bbeffd into master Jul 24, 2023
22 checks passed
@ascjones ascjones deleted the aj/shared-events-redux branch July 24, 2023 14:12
jubnzv added a commit to jubnzv/ink that referenced this pull request Jul 30, 2023
jubnzv added a commit that referenced this pull request Aug 15, 2023
…1837)

* The initial structure of the linting crate and tests

Needed for #1436

* Implement the lint that checks primitive numeric types in expanded code

* The initial implementation of the `primitive_topic` lint

* chore: Run clippy

* fix(fmt)

* fix: Lint suppressions

The problem with suppressions is that `rustc` saves the state of the
last visited node and checks its attributes to decide if the lint
warning should be suppressed there (see:
[LateContext::last_node_with_lint_attrs](https://github.com/rust-lang/rust/blob/c44324a4fe8e96f9d6473255df6c3a481caca76f/compiler/rustc_lint/src/context.rs#L1105)).

This commit adds a call to the utility function from clippy that checks
if the lint is suppressed for the field definition node, not for the
last node (which is `Topics` impl).

* chore: cleanup

* chore: make warning messages more consistent

* feat: support events 2.0

Rework the lint after #1827 is merged

* chore: Fix comments; remove needless empty file

* chore: Update `clippy_utils` version

* fix: Cargo.toml

---------

Co-authored-by: Georgiy Komarov <georgiy@parity.io>
@SkymanOne SkymanOne mentioned this pull request Dec 22, 2023
5 tasks
@SkymanOne SkymanOne mentioned this pull request Feb 26, 2024
5 tasks
@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
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.

Shared external event definitions
4 participants