use-ink
Shared external event definitions
Early WIP Closes #809
E.g.
// Define event to be shared across contracts
#[ink::event_definition]
pub struct SharedEvent {
arg_1: u8,
#[ink(topic)]
arg_2: u16,
}
#[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::SharedEvent { arg_1: 1, arg_2: 2 });
}
}
}
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.
- 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.
- 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.
- The identifier of the event is generated based on the name of the contract's storage.
https://github.com/paritytech/ink/issues/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.
- 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.
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 https://github.com/paritytech/ink/issues/809 also will resolve the problem with event identifiers. So it is why https://github.com/paritytech/ink/issues/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
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.
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:
- 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 ofmod access_controll { ... }and some restricted constructions. It makes codegen more complex. - It doesn't solve the issue described in the example with
Dex::swapand emittedPSP22::Transferevents. It is because we didn'tpullthe events that can be emitted. (Of course, it is optional problem for solving, but still a problem) - We can't use the same naming during the event definition.
PSP22already is used by the trait and it is not allowed to use the same name inenum PSP22 { Transfer, Approve }. We can have a separatemod 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:
- We don't affect the definition of the trait. The definition of the event is independent and native. The codegen is simple.
- We can't lose any event that can be emitted, because we include all events imported into the contract.
- The events can be defined anywhere and anyhow without any restrictions.
cons:
- 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.
Will respond properly tomorrow but regarding:
- 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 https://github.com/ethereum/solidity/pull/10996?
I did some googling about the state of that in Solidity, and looks like that is https://github.com/ethereum/solidity/issues/9765 but this PR looks like it might be a solution https://github.com/ethereum/solidity/pull/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.
- 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 is in keeping with the existing philosophy of ink! where we have #[ink::contract] mod erc20 already as a container, and also in Substrate the #[frame_support::pallet] pub mod pallet construct.
It is slightly more code and more restrictive, but it does crystalize the idea that the contract messages and its events are all part of its interface and should therefore be defined together as per Solidity interfaces.
Also if we want to we could still support standalone #[ink::trait_definition] and just generate the equivalent code as if embedded in #[ink::interface] just with a () event. Could do similar for standalone event_definition but that makes less sense I think.
It makes codegen more complex
Indeed we should weigh the added complexity when considering the solution
- 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)
That's true and it's not an easy problem to solve (without resorting to the onstartup! global static mutex). And we have to consider whether solving this problem is worth defining the approach to solving this.
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.
I can definitely see the benefits of having it happen automatically. But after all the Ethereum ecosystem has done pretty well so far without this, presumably it requires a secondary lookup for the metadata of known imported events.
- 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=)
That is not unsolvable, since the trait definition will be available to the macro we can just use the name from that by convention, and also additionally provide an attribute to apply a custom name.
cons: We include all events, useless and useful.
I would add the fact that by nature it is a hacky solution because we have to use the global Mutex for the shared state and rely on the onstartup! macro. I'm not against it per se but I believe we should rigorously explore alternatives, and if that turns out to be the best approach then all good.
Yesterday I tried to play with rust ABI and linking attributes but didn't find how to do that.
Yes this might be one possible area of exploration, if we can use the cargo-contract tooling as we do with calling #[no_mangle] _ink_generate_metadata, but first scan the binary for some known method prefix or somehow use #[link_section]. Arguably this is also hacky but I prefer all the hacks to be in cargo-contract and not in ink! :see_no_evil:
It is slightly more code and more restrictive, but it does crystalize the idea that the contract messages and its events are all part of its interface and should therefore be defined together as per Solidity interfaces.
It is=) But from my perspective as the user of ink! I prefer to have a native rust definition of the enum instead of creating heavy constructions and hidden relations between trait definition and events.
I can definitely see the benefits of having it happen automatically. But after all the Ethereum ecosystem has done pretty well so far without this, presumably it requires a secondary lookup for the metadata of known imported events.
But they've already implemented automated collection of events because Solidity is an independent language=) If we would use our own version of rust we also can implement it in a native way. The issue that you linked exists because Solidity developers missed the collection of events in libraries.
That is not unsolvable, since the trait definition will be available to the macro we can just use the name from that by convention, and also additionally provide an attribute to apply a custom name.
Yes, we can, but it all adds hidden behaviors and new entities.
I would add the fact that by nature it is a hacky solution because we have to use the global Mutex for the shared state and rely on the onstartup! macro. I'm not against it per se but I believe we should rigorously explore alternatives, and if that turns out to be the best approach then all good.
Yes this might be one possible area of exploration, if we can use the cargo-contract tooling as we do with calling #[no_mangle] _ink_generate_metadata, but first scan the binary for some known method prefix or somehow use #[link_section]. Arguably this is also hacky but I prefer all the hacks to be in cargo-contract and not in ink!
If you are okay with hacks on the cargo-contract level, then I can suggest a suitable solution=)
We already have a secure mechanism to catch the not allowed usage of some methods via __ink_enforce_error_. Right now on the cargo-contract side, we are checking that the WASM file doesn't contain any method with __ink_enforce_error_ prefix.
We can use the same mechanism to understand that events are used in the contract. We can use prefix __ink_event_usage_{} where {} is identifier of the event. Based on that artifact information we can filter events from lazy solutions OR we can update the generation of metadata. Instead of importing only __ink_generate_metadata we can import each __ink_event_usage_{}. Before the generation of metadata, we can collect all events via calling __ink_event_usage_{}.
extern "Rust" {
fn __ink_generate_metadata() -> ink_metadata::MetadataVersioned;
fn __ink_event_usage_1();
fn __ink_event_usage_2();
fn __ink_event_usage_3();
}
fn main() -> Result<(), std::io::Error> {
unsafe { __ink_event_usage_1() };
unsafe { __ink_event_usage_2() };
unsafe { __ink_event_usage_3() };
let metadata = unsafe { __ink_generate_metadata() };
let contents = serde_json::to_string_pretty(&metadata)?;
print!("{}", contents);
Ok(())
}
In that case, we will collect only events that really were used by the contract.
We can use the same mechanism to understand that events are used in the contract. We can use prefix _ink_event_usage{} where {} is identifier of the event
Seems like a good idea, I'll have a think about it over the weekend!
@xgreenx see WIP https://github.com/paritytech/cargo-contract/pull/794 for gathering the shared events metadata.