Shared external event definitions

[ascjones]

WIP Closes #809

• events must now be defined as an enum annotated with #[ink::event_definition]

• events can now be defined outside the #[ink::contract] definition and can be shared across contracts

• contracts can emit externally defined events.

• contracts can still define an event inline, but this can now be referenced and used by other contracts.

• contracts can now define multiple inline event enums (though probably should not be encouraged)

• each variant has its own signature topic which should be calculated at compile time

  • decide the composition of the signature topic see Shared external event definitions #1243 (comment)

• field topics will no longer have a prefix but will be just the raw value, see Shared external event definitions #1243 (comment)

  • confirm this is the correct approach and there will be enough information for clients to filter on a specific field of a specific event
  • figure out what to do about Option types, possibly not write out the topic at all in case of None. Don't want a bunch of 0x0000.. topics for e.g. Option<AccountId>
  • remove check in substrate for duplicate topics @xgreenx

• resolve how to deal with compile time enforcement of the environments MAX_EVENT_TOPICS.

/// Event defined outside of `#[ink::contract]`, can be shared across contracts
#[ink::event_definition]
pub enum Erc20Event {
    /// Event emitted when a token transfer occurs.
    Transfer {
        #[ink(topic)]
        from: Option<AccountId>,
        #[ink(topic)]
        to: Option<AccountId>,
        value: Balance,
    },
    /// Event emitted when an approval occurs that `spender` is allowed to withdraw
    /// up to the amount of `value` tokens from `owner`.
    Approval {
        #[ink(topic)]
        owner: AccountId,
        #[ink(topic)]
        spender: AccountId,
        value: Balance,
    },
}

#[ink::contract]
mod contract {
    #[ink(storage)]
    pub struct Contract {}

    impl Contract {
        #[ink(constructor)]
        pub fn constructor() -> Self {
            Self {}
        }

        #[ink(message)]
        pub fn message(&self) {
            self.env().emit_event(super::Erc20Event::Transfer { from: None, to: None });
        }
    }
}

[xgreenx]

Fixing that issue is part of our Grant. If we can agree on the implementation the Superoclony will handle the implementation=)

In the comment we highlighted two problems that we need to solve.

1. The identifier of the event is generated based on the name of the contract's storage.

A created issue during retreat solves that very well. The name of the events enum + the name of the variant defines a unique identifier of the event.

2. We can't collect metadata from other crates. So we can't generate metadata for those events because we don't know about them.

The approach with explicit aliases duplicates the code. But it still is better than before(you don't need to define all events in the contract's body).

    // import event as a type alias and mark as an ink(event)
    // Possibly could do this as a `use` instead or in addition.
    #[ink(event)]
    type Event0 = super::Event0;

In the comment, we suggested another approach with lazy loading of all events from the crates. This repository contains an example of how we can use the on_startup macro to collect events from different crates. It will automate the generation of metadata and simplify the usage of events. The problem with the solution is that we will include all events that were defined in sub-crates(if you import some crate you also import events from that crate into metadata). But it is not a problem at all, because it doesn't make the contract heaver. It only adds maybe not fully useful information to the metadata but now it is automated.

[ascjones]

1. The identifier of the event is generated based on the name of the contract's storage.

#1222 during retreat solves that very well. The name of the events enum + the name of the variant defines a unique identifier of the event.

Yes that would indeed allow us to generate a useful event identifier for the contract. However it is worth noting that my current solution would also allow that since the codegen is already generating a top level Event enum from the individual specified events. e.g.

mod shared {
    #[ink::event_definition]
    pub struct SharedEvent;
}

#[ink(event)]
struct EventLocal;

#[ink(event)]
type EventB = shared::SharedEvent;

generates approximately...

enum __ink_EventBase {
    EventLocal(EventLocal),
    SharedEvent(SharedEvent),
}

So, regardless of whether we implement #1222, this solution is available to us.

2. We can't collect metadata from other crates. So we can't generate metadata for those events because we don't know about them.

The approach with explicit aliases duplicates the code. But it still is better than before(you don't need to define all events in the contract's body).

By code duplication do you mean the fact that it is required to annotate the imported shared event with #[ink(event)]?

In the comment, we suggested another approach with lazy loading of all events from the crates. This repository contains an example of how we can use the on_startup macro to collect events from different crates. It will automate the generation of metadata and simplify the usage of events. The problem with the solution is that we will include all events that were defined in sub-crates(if you import some crate you also import events from that crate into metadata).

I'm not sure about this approach, it is a big change to the metadata generation philosophy and introduces some extra complexity.

But it is not a problem at all, because it doesn't make the contract heaver. It only adds maybe not fully useful information to the metadata but now it is automated.

Currently the metadata lists all possible events defined in a contract, and with this approach the metadata would possibly contain events that are not used by the contract at all. IMO it is preferable that the metadata should be an accurate representation as to the API of the contract.

Overall the philosophy I have approached my solution is: how can we introduce shared events into the existing model of event definitions, not aiming for a complete overhaul. It should make it possible to release this as a 3.x non breaking change.

Not to say that the event codegen doesn't need some attention and thought and possibly a complete overhaul, especially optimizing codesize of topics etc.

It's just for this I want to focus on only adding shared events into the existing framework.

[xgreenx]

I guess you meant:

enum __ink_EventBase {
    EventLocal(EventLocal),
    EventB(shared::SharedEvent),
}

What benefits do we have in grouping all events into one __ink_EventBase enum? I see it helps to work with events during codegen in a more convenient way but it doesn't affect the developer anyhow. Correct me if I'm wrong=)

Do we need really combine all events into one enum if we can refactor the events collection and in that case, each event definition can be independent?

We hoped that resolving the #809 also will resolve the problem with event identifiers. So it is why #1222 looks very promising. Yes, it breaks the previous API but it allows contracts to be production-ready. Because all contracts can emit the same events(the name of the contract will not affect the identifier).

mod events {
   enum PSP22 {
       Transfer(AccountId, AccountId, Balance),
       Approve(AccountId, AccountId, Balance),
       ...
   }
}

That kind of event can be standardized and everyone can emit it.

We can extract that to a separate issue but I think all of it is related to each other and should be solved during one breaking change refactoring=)

By code duplication do you mean the fact that it is required to annotate the imported shared event with #[ink(event)]?

Yes!=)
Let's imagine that you split your contract into 4 big parts(AccessControl, PSP22, Lending, Router). Each part emits 4 events. You implemented the logic of each part in a separate crate. Also, you implemented emitting of those events in that crate.

Now you want to combine all parts together to create a final contract.

#[ink(storage)]
pub struct ComplexContract {
    access: AccessControlData,
    psp22: PSP22Data,
    lending: LendingData,
    router: RouterData,
}

impl AccessControl for ComplexContract {}
impl PSP22 for ComplexContract {}
impl Lending for ComplexContract {}
impl Router for ComplexContract {}

#[ink(event)]
type AccessControlEventChangeRole = access_contol::ChangeRole;
#[ink(event)]
type AccessControlEventRoleGranted = access_contol::RoleGranted;
#[ink(event)]
type AccessControlEventRoleRevoked = access_contol::RoleRevoked;
#[ink(event)]
type AccessControlEventCucumber = access_contol::Cucumber;

#[ink(event)]
type PSP22EventTransfer = psp22::Tranfer;
#[ink(event)]
type PSP22EventApprove = psp22::Approve;
#[ink(event)]
type PSP22EventHello = psp22::Hello;
#[ink(event)]
type PSP22EventWorld = psp22::World;

#[ink(event)]
type LendingEventBorrow = lending::Borrow;
#[ink(event)]
type LendingEventDeposit = lending::Deposit;
#[ink(event)]
type LendingEventLiquidated = lending::Liquidated;
#[ink(event)]
type LendingEventOracleChange = lending::OracleChange;

#[ink(event)]
type RouterEventCreated = router::Created;
#[ink(event)]
type RouterEventNewProposal = router::NewProposal;
#[ink(event)]
type RouterEventNewLending = router::NewLending;
#[ink(event)]
type RouterEventSwap = router::Swap;

And you scrolled here=D It is really a lot of boilerplate code and you should create a unique name for each event. Overwise it will conflict with other evens like PSP34Event::Transfer with PSP22Event::Transfer. It is better than the previous solution where you defined all events but we still can improve it=)

I'm not sure about this approach, it is a big change to the metadata generation philosophy and introduces some extra complexity.

The philosophy changed only a little bit. Instead of one place to collect metadata, we will collect it from each crate and the final metadata will be generated in the same way. In simple form: we have a static array with events. During a load of each crate, we add events to that array. When all crates are loaded it runs __ink_generate_metadata that uses that array to create the final metadata with all events. It is not hard to implement and I'm sure that it will simplify codegen=) Because we don't need to parse anything in ink_lang::contract, each ink_lang::event_definition is independent.

Currently the metadata lists all possible events defined in a contract, and with this approach, the metadata would possibly contain events that are not used by the contract at all. IMO it is preferable that the metadata should be an accurate representation as to the API of the contract.

Better to have several useless events than to forget to add some events at all=) If all events are collected automatically, it doesn't add any problem. But if we missed some events it can cause some issues.

With the introduction of upgradable contracts, we can't have accurate representation at all=) Because any contract can be upgradable or be Diamond and in that case, it is a combination of different ABIs.

Another plus for collecting all events(Example from our integration testing):
For example, you create a DEX. That means you do transfers between PSP22. Each PSP22 emits an event on the transfer. So during the execution of the Dex::swap, it will emit the known Dex::Swap event and the bunch on the unknown PSP22Event::Tranfer. With a lazy approach, collecting all events: if someone imported the trait definition of the PSP22 then he also imported the definition of the events. In that case, the indexer, UI, or another application will know how to decode PSP22Event::Tranfer. Adding more events doesn't cause any problem=)

BTW, we also can use event::PSP22::Tranfer instead of PSP22Event::Tranfer with a lazy approach and the identifier can be generated based on PSP22::Transfer=)

P. S. Sorry for long comment=D

[ascjones]

Also, you implemented emitting of those events in that crate.

Okay I had not considered this scenario, my assumption was that the contract implementation itself would always raise the events, therefore it would have to import them either with a use or a type alias which could be decorated.

Your examples got me thinking, that shared events are tied to their corresponding traits, since they are effectively part of the contract's interface. i.e. any UI for ERCX contracts requires both standard messages and events. Indeed if you look at the standards e.g. https://eips.ethereum.org/EIPS/eip-1155 they include the event definitions. So we could have something like:

#[ink::interface]
pub mod access_control {

    #[ink(trait_definition)]
    pub trait AccessControl {
        // messages
    }
    
    #[ink(event)]
    pub enum AccessControlEvent {
        ChangeRole,
        RoleGranted,
        RoleRevoked,
        Cucumber,
    }
}

So now the trait and the events are tied together, we could even use the impl AccessControl block within the contract definition to wire up the event metadata.

The philosophy changed only a little bit. Instead of one place to collect metadata, we will collect it from each crate and the final metadata will be generated in the same way. In simple form: we have a static array with events

It is changing from explicit "pulling" to implicit "pushing" of event metadata via global state, so we need to be sure it is the correct thing to do, not just the easiest way to solve the problem.

[xgreenx]

Your examples got me thinking, that shared events are tied to their corresponding traits, since they are effectively part of the contract's interface. i.e. any UI for ERCX contracts requires both standard messages and events. Indeed if you look at the standards e.g. https://eips.ethereum.org/EIPS/eip-1155 they include the event definitions.

Yep, that idea was a reason to create that issue=)

So now the trait and the events are tied together, we could even use the impl AccessControl block within the contract definition to wire up the event metadata.

Yep, we also come up with that solution and it already fits all requirements. The trait can have some hidden __ink_events() method that returns the whole list of events(or we can generate some stuff like TraitInfo. I think we can try to experiment here), we can call it based on the impl block. And if you don't like the solution with lazy loading, then I hope we will choose that solution=)

That solution has 3 cons:

1. It makes the definition of the trait harder and introduces a new entity as #[ink_lang::interface]. Previously the definition of the trait and events was intuitive in the native Rust way. With that approach, it requires the usage of mod access_controll { ... } and some restricted constructions. It makes codegen more complex.
2. It doesn't solve the issue described in the example with Dex::swap and emitted PSP22::Transfer events. It is because we didn't pull the events that can be emitted. (Of course, it is optional problem for solving, but still a problem)
3. We can't use the same naming during the event definition. PSP22 already is used by the trait and it is not allowed to use the same name in enum PSP22 { Transfer, Approve }. We can have a separate mod events { ... } but in that case, it is a very huge definition=)

It is changing from explicit "pulling" to implicit "pushing" of event metadata via global state, so we need to be sure it is the correct thing to do, not just the easiest way to solve the problem.

You are right, it changes it in that way=)
Of course, it requires discussion and I hope we will discuss that approach. The main idea is that events are used in most cases by UI, indexers, and other aggregation stuff. ABI is used to decode the events right. It is a problem if we don't know how to decode the event. But it is not a problem if we know how to decode events that can't be emitted. It, maybe, can be a problem in the case of an indexer because the indexer will listen for events that can't be emitted by the contract. But in most cases the handler that should process the event is empty, so the indexer can not listen for events without a handler.

The main pros:

1. We don't affect the definition of the trait. The definition of the event is independent and native. The codegen is simple.
2. We can't lose any event that can be emitted, because we include all events imported into the contract.
3. The events can be defined anywhere and anyhow without any restrictions.

cons:

1. We include all events, useless and useful.

I hope that we will find and select the best approach. I'm describing only the ideas and thoughts of our team and the use cases that we faced.

[ascjones]

Will respond properly tomorrow but regarding:

2. It doesn't solve the issue described in the example with Dex::swap and emitted PSP22::Transfer events. It is because we didn't pull the events that can be emitted. (Of course, it is optional problem for solving, but still a problem)

I did some googling about the state of that in Solidity, and looks like that is not currently supported but this PR looks like it might be a solution ethereum/solidity#10996?

[xgreenx]

I did some googling about the state of that in Solidity, and looks like that is ethereum/solidity#9765 but this PR looks like it might be a solution ethereum/solidity#10996?

The best solution is to include the event if it was somewhere emitted. But ink! is not its own language, so it is hard to identify that event was emitted. Yesterday I tried to play with rust ABI and linking attributes and with ctor but didn't find how to do that.

[xgreenx]

The anchor for discussing prefixes and topics(I use it only as a reference).

[ascjones]

As you mentioned in our chat, we don't need this const fn if we are just using Enum::Variant, we can do the hashing in the proc_macro itself. Currently I need this because I am using module_path!, but if we can do without this then we can choose our hashing algorithm.

The question remains then whether it is sufficient to use Enum::Variant or do we add the field names too e.g. Enum::Variant { field_1, field_2, field_3 } similar to Ethereum (except without type names).

With our without the field names, need to consider the impact of signature topic collisions.

[xgreenx]

With #1533, tests fail because of the same topics.

The caller and from accounts are the same. It is clearly a problem with the test because the caller and from should be different, but it raises discussion about this part.

In the ethereum, the first topic is almost always a signature of the event(except manual LOG* opcode). And you highlighted it here. So the first topic is affected by the event's name and used types. But other topics are not prefixed and contain raw/hashed(if the value is more than 32 bytes) values.

I think we need to follow the same pattern. The first topic helps to subscribe to a specific event of contracts. With raw/hashed values, we can do searches across all events, only knowing value without knowing the event structure. It is also very helpful.

Prefixing each topic with the field's name allows us not to mix different events or fields with the same type. And for that, we need to know the definition of the event to do searches. But, we can achieve the same by filtering supported by ethereum nodes(we can do the same). We can use the event's signature if we need only specific events. If we need a specific field, we can use the (null, null, 0x...) format where we want the third topic.

So comparing those two options, the Ethereum approach looks more flexible and performant(we don't need to hash prefixes and values each time).

It seems you follow the same thoughts(based on your comment), so I only highlighted conclusions here(if not, let's discuss =) ).
I also created a discussion here because following the Ethereum approach creates several issues we need to solve:

1. How do we want to calculate a unique signature of the event? It should be documented to be clear for developers of tools. We discussed it in this PR before but didn't finalize it. My approach was to use the hash({Name of the enum}::{Name of the variant}) or hash({Name of the enum}::{Name of the variant}::{Number of fields}). It is a bad idea to use types names because, in Rust, we can create a wrapper that behaves as an original type but has a new definition=( We can use only names, as we do for traits. The number of fields is only an idea that we can discuss or maybe that can give you more ideas=) Also we need to define which hash algorithm we want to use. Because we can calculate the hash during event definition, we are not limited.

2. How do we want to process two same topics? Based on the Ethereum approach, we need to allow them, but on the contract-pallet side, we don't let it(and it is an error that I got in the tests).

3. Right now, we sort topics, but it breaks the filtering strategy because the expected topic can be in an unexpected position.

[ascjones]

We are on the same page regarding the event topics, I am also leaning towards using non-prefixed field topics. Clients would then need to filter on both the signature topic and the field topic if they wanted only a specific field. It also opens up the possibility to susbscribe to all events with a topic of a given account id, which would be very useful for explorers.

[ascjones]

Superseded by #1827