-
Notifications
You must be signed in to change notification settings - Fork 431
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
Shared external event definitions #1243
Closed
Closed
Changes from all commits
Commits
Show all changes
163 commits
Select commit
Hold shift + click to select a range
2c42a21
Add ui test for shared event
ascjones 21fd910
WIP adding ink::event_definition
ascjones 27948f1
Merge branch 'master' into aj/shared-events
ascjones ba6734d
More event definition wiring
ascjones 54fbe58
WIP building shared event def
ascjones 400ea92
Merge branch 'master' into aj/shared-events
ascjones 88f3722
Make event_definition attribute compile
ascjones 63a30c6
Parse ink(event) attribute on type alias
ascjones 90c2eb5
Combining IR for external and inline event definitions
ascjones ee9e74b
WIP generating Topics impls for inline events
ascjones 0edba13
Remove duplicate topics impls
ascjones c1f7d30
Merge branch 'master' into aj/shared-events
ascjones 2d2541d
WIP implementing event spec metadata generation
ascjones 05d35b5
Add missing semicolon
ascjones e73e17c
Remove unused fields iterator
ascjones a1a04f0
Fix imported event ident
ascjones 98c7f49
Update trybuild to run ui tests in parallel (#1220)
ascjones c931a38
Fix test errors
ascjones a2c60ea
Add todo
ascjones f5882ed
Merge branch 'master' into aj/shared-events
ascjones 3f1978a
Fix test errors
ascjones 8b393f3
Merge remote-tracking branch 'origin/aj/shared-events' into aj/shared…
ascjones 0081544
Test out new interface api
ascjones 13a1e62
Merge branch 'master' into aj/shared-events
ascjones 7f35189
Revert "Test out new interface api"
ascjones 5f3f2f0
Update example to latest syntax
ascjones 35d8184
Fmt
ascjones 6763b74
Remove EmitEvent impl
ascjones 7063bd3
Remove EmitEvent impl and ContractBaseEvent
ascjones 7e115ec
Add args to shared event test
ascjones 100e53b
Convert inline event to use event_def codegen
ascjones 71db312
Remove ink item Event
ascjones c73ea8d
Fmt
ascjones 7628786
Merge branch 'master' into aj/shared-events
ascjones 7e59e74
Rewire contract internal event definitions
ascjones c6a8cec
add todos
ascjones fd8fef2
Merge branch 'master' into aj/shared-events
ascjones 0dfc495
WIP changing event def to be an enum
ascjones 34da8a6
WIP adding EventVariantInfo trait for EVENT_SIGNATURE
ascjones 14d6e30
WIP adapting to synstructure
ascjones babda7d
Merge branch 'master' into aj/shared-events
ascjones 99bb6bd
WIP generating event signature topics impls
ascjones 1e5dae3
Revert previous work attempting to use synstructure
ascjones 65a8822
Restore wiring for inline contract events
ascjones 8735978
WIP generating event enum impls
ascjones 17609fe
Merge branch 'master' into aj/shared-events
ascjones 6663383
Fix event def after merge
ascjones ca6247e
Merge branch 'master' into aj/shared-events
ascjones 090219d
Fmt
ascjones 2189719
Recover changes from master
ascjones 42dc296
Fix up metadata compilation
ascjones 3be3025
Fmt
ascjones a1ac0b4
Convert more events to enums
ascjones 10ad8e5
Fix some crate paths
ascjones 9383b40
Fix event def tests
ascjones 5253a31
Start to rebuild topics impl
ascjones 8e8af63
Fix some IR tests
ascjones d2ce784
WIP impl variant topics
ascjones dd2c398
Merge branch 'master' into aj/shared-events
ascjones 504e1fe
WIP building push_topics impl
ascjones 35561a7
WIP use const fn to generate event_variant_signature
ascjones a9d0559
WIP generate event variant match arms with remaining topics
ascjones 23fe12e
Fix some errors
ascjones 8db474f
Split InkEventDefinition constructors for inline and external events
ascjones 1c4d382
Merge branch 'master' into aj/shared-events
ascjones 5068804
Read anonymous from variant attributs, also fmt
ascjones 9914bf4
Just allow only ink::event_definition
ascjones 3e83f6e
WIP attempt to make trait-erc20 compile
ascjones a076821
Fix up field def gen error
ascjones 2f81f67
Fix up trait-erc20 events emit
ascjones 797ea95
Fix up ink_env refs in event_definition
ascjones 2309dd3
Wire up event signature topic
ascjones d776c3f
Fix variant field bindings
ascjones a720d30
Generate EventVariantInfo SIGNATURE
ascjones 9d2e334
WIP impl compile time signature topic calculation
ascjones 9246664
Remove field topic prefix
ascjones 343ae04
Resolve some warnings
ascjones 4238fbd
Restore topics guard
ascjones 216d684
Merge branch 'master' into aj/shared-events
ascjones 87ac91c
WIP generate event metadata, introduce EventInfo trait
ascjones ec01087
Generate event variants metadata, wire it up
ascjones 97cd765
Allow passing in of event metadata vec
ascjones 354f81d
Generate event metadata custom sections and externs
ascjones ff3b2ed
Remove todo comment from event-shared-external.rs
ascjones 6c94cb0
Filter topic fields and combine bindings
ascjones dbe237e
Merge branch 'master' into aj/shared-events
ascjones 3beb5a7
Migrating erc721 to new event def
ascjones 06bf240
Put event definition to the top of the contract file
ascjones e1c97de
Add ink event metadata custom section marker
ascjones 2356ffa
Fix errors after merge
ascjones 53189d5
Number of topics builder per variant
ascjones 3a23e06
Fmt erc721
ascjones ca68223
Convert dns to event_definition enum
ascjones d922172
Fix up erc20 events
ascjones 16aa615
Migrate rand-extension example
ascjones 2ba841e
Migrate erc1155 example
ascjones 55884db
Migrate payment-channel example
ascjones 7d42db0
Migrate mother example
ascjones c91171a
Migrate multisig example
ascjones b4ddcac
WIP fixing topic attr validation
ascjones 0343989
Merge branch 'master' into aj/shared-events
ascjones 9af23c6
WIP use event def ids to call into metadata
ascjones 82cff3d
Fix up topic field attribute validation, fix warnings
ascjones 2261e9d
Revert using event definition ids in custom section
ascjones e8a74c2
Merge branch 'master' into aj/shared-events
ascjones a25b763
Fix compilation errors after merge
ascjones 87a014a
Comment out TypeSpec::default for now
ascjones ce28c3c
Merge branch 'master' into aj/shared-events
ascjones abdb624
Fix clippy errors
ascjones 1d6fbaf
Update `scale-info` requirement to 2.3
ascjones 1e89578
Merge branch 'master' into aj/upgrade-scale-info-requirement
ascjones 905b943
Merge branch 'aj/upgrade-scale-info-requirement' into aj/shared-events
ascjones 495e830
Merge branch 'master' into aj/shared-events
ascjones 22739eb
Fix up erc20 ui test
ascjones 39da2d7
Fix and remove some tests
ascjones 2aef9d5
Fix non pub test
ascjones 2d16ec1
Fix up metadata specs
ascjones 5862d55
Merge branch 'master' into aj/shared-events
ascjones 64915ad
Merge branch 'master' into aj/shared-events
ascjones cdee107
Remove method after merge
ascjones 148f43f
E2E: utilize `contract-build` crate
ascjones 50e88c1
Merge branch 'master' into aj/contract-build-lib
ascjones 52f2024
Build as debug so we can see the debug messages
ascjones 8481f8b
Merge branch 'master' into aj/shared-events
ascjones 8aebcf2
Merge branch 'master' into aj/shared-events
ascjones 21c37f5
Merge branch 'master' into aj/contract-build-lib
ascjones 9d84bbe
Merge branch 'master' into aj/shared-events
ascjones 963cfee
Switch to using `contract-build` master branch
ascjones e9701ea
Add missing flag
ascjones cdbcbe0
Use released `contract-build` crate
ascjones 98dfb4a
Merge branch 'aj/contract-build-lib' into aj/shared-events
ascjones 4775cf3
Use custom `contract-build` branch
ascjones 3777274
Remove duplicate generate_type_spec method
ascjones e9fe3e1
Update generate_metadata signature in ui test
ascjones 9d9ef5e
Fix anonymous event ui test
ascjones 5ef3525
Update event defs in some UI tests
ascjones ddd3559
More UI tests migration
ascjones e5d8163
Check for event definitions with no variants
ascjones b5a961a
Check for event definitions are not structs
ascjones 38d74f9
Fix up single definition event UI test
ascjones f849e8e
Fix up too-many-topics UI test, succeeds but should fail...
ascjones 7cb6c74
Merge branch 'master' into aj/shared-events
ascjones 5d80691
Fix up generate_metadata extern in test
ascjones 55813c6
Restore max topics len guard for environment
ascjones e96f9d6
Remove extra topic, max topics now includes signature topic
ascjones bba0e88
Update event-conflicting-storage.rs for event_definition
ascjones c999880
Fixed compilation errors events (#1533)
xgreenx dc32a9c
Merge branch 'master' into aj/shared-events
ascjones 9155178
Merge branch 'master' into aj/shared-events
ascjones b883634
Merge branch 'master' into aj/shared-events
ascjones aae9fcb
Merge branch 'master' into aj/shared-events
ascjones 706c600
Constrain emit_event environment
ascjones dc5d2eb
Merge branch 'master' into aj/shared-events
ascjones 482b610
Merge branch 'master' into aj/shared-events
ascjones ea73683
Merge branch 'master' into aj/shared-events
ascjones 7a95e98
Merge branch 'master' into aj/shared-events
ascjones e1674a9
Merge branch 'master' into aj/shared-events
ascjones d9ec19b
Merge branch 'master' into aj/shared-events
ascjones 7ae9b85
Merge branch 'master' into aj/shared-events
ascjones 468b226
Calculate signature topic in macro
ascjones 203fe8b
Fix lifetime from merge
ascjones 3854c49
Generate signature topic
ascjones 06c7933
Add back method lost in merge
ascjones File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,223 @@ | ||
// Copyright 2018-2022 Parity Technologies (UK) Ltd. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
use crate::GenerateCode; | ||
|
||
use derive_more::From; | ||
use proc_macro2::TokenStream as TokenStream2; | ||
use quote::{ | ||
quote, | ||
quote_spanned, | ||
}; | ||
|
||
/// Generates code for an event definition. | ||
#[derive(From)] | ||
pub struct EventDefinition<'a> { | ||
event_def: &'a ir::InkEventDefinition, | ||
} | ||
|
||
impl GenerateCode for EventDefinition<'_> { | ||
fn generate_code(&self) -> TokenStream2 { | ||
let event_enum = self.generate_event_enum(); | ||
let event_info_impls = self.generate_event_info_impl(); | ||
let event_variant_info_impls = self.generate_event_variant_info_impls(); | ||
let event_metadata_impl = self.generate_event_metadata_impl(); | ||
let topics_impl = self.generate_topics_impl(); | ||
let topics_guard = self.generate_topics_guard(); | ||
quote! { | ||
#event_enum | ||
#event_info_impls | ||
#event_variant_info_impls | ||
#event_metadata_impl | ||
#topics_impl | ||
#topics_guard | ||
} | ||
} | ||
} | ||
|
||
impl<'a> EventDefinition<'a> { | ||
fn generate_event_enum(&'a self) -> TokenStream2 { | ||
let span = self.event_def.span(); | ||
let event_enum = &self.event_def.item; | ||
quote_spanned!(span => | ||
#[derive(::scale::Encode, ::scale::Decode)] | ||
#event_enum | ||
) | ||
} | ||
|
||
fn generate_event_info_impl(&self) -> TokenStream2 { | ||
let span = self.event_def.span(); | ||
let event_ident = self.event_def.ident(); | ||
|
||
quote_spanned!(span=> | ||
impl ::ink::reflect::EventInfo for #event_ident { | ||
const PATH: &'static str = ::core::concat!( | ||
::core::module_path!(), | ||
"::", | ||
::core::stringify!(#event_ident) | ||
); | ||
} | ||
) | ||
} | ||
|
||
fn generate_event_variant_info_impls(&self) -> TokenStream2 { | ||
let span = self.event_def.span(); | ||
let event_ident = self.event_def.ident(); | ||
|
||
let impls = self.event_def.variants().map(|ev| { | ||
let event_variant_ident = ev.ident(); | ||
let signature_topic = ev.signature_topic_hex_lits(event_ident); | ||
let index = ev.index(); | ||
quote_spanned!(span=> | ||
impl ::ink::reflect::EventVariantInfo<#index> for #event_ident { | ||
const NAME: &'static str = ::core::stringify!(#event_variant_ident); | ||
const SIGNATURE_TOPIC: [u8; 32] = [ #( #signature_topic ),* ]; | ||
} | ||
) | ||
}); | ||
quote_spanned!(span=> | ||
#( | ||
#impls | ||
)* | ||
) | ||
} | ||
|
||
fn generate_event_metadata_impl(&self) -> TokenStream2 { | ||
let event_metadata = super::metadata::EventMetadata::from(self.event_def); | ||
event_metadata.generate_code() | ||
} | ||
|
||
/// Generate checks to guard against too many topics in event definitions. | ||
fn generate_topics_guard(&self) -> TokenStream2 { | ||
let span = self.event_def.span(); | ||
let event_ident = self.event_def.ident(); | ||
// todo: [AJ] check if event signature topic should be included here (it is now, | ||
// wasn't before) | ||
let len_topics = self.event_def.max_len_topics(); | ||
let max_len_topics = quote_spanned!(span=> | ||
<<#event_ident as ::ink::env::Topics>::Env | ||
as ::ink::env::Environment>::MAX_EVENT_TOPICS | ||
); | ||
quote_spanned!(span=> | ||
impl ::ink::codegen::EventLenTopics for #event_ident { | ||
type LenTopics = ::ink::codegen::EventTopics<#len_topics>; | ||
} | ||
|
||
const _: () = ::ink::codegen::utils::consume_type::< | ||
::ink::codegen::EventRespectsTopicLimit< | ||
#event_ident, | ||
{ #max_len_topics }, | ||
> | ||
>(); | ||
) | ||
} | ||
|
||
fn generate_topics_impl(&self) -> TokenStream2 { | ||
let span = self.event_def.span(); | ||
let event_ident = self.event_def.ident(); | ||
|
||
let variant_match_arms = self | ||
.event_def | ||
.variants() | ||
.map(|variant| { | ||
let span = variant.span(); | ||
let variant_ident = variant.ident(); | ||
let (field_bindings, field_topics): (Vec<_>, Vec<_>) = variant.fields() | ||
.filter(|field| field.is_topic) | ||
.map(|field| { | ||
let field_type = field.ty(); | ||
let field_ident = field.ident(); | ||
let push_topic = | ||
quote_spanned!(span => | ||
.push_topic::<::ink::env::topics::PrefixedValue<#field_type>>( | ||
&::ink::env::topics::PrefixedValue { | ||
// todo: figure out whether we even need to include a prefix here? | ||
// Previously the prefix would be the full field path e.g. | ||
// erc20::Event::Transfer::from + value. | ||
// However the value on its own might be sufficient, albeit | ||
// requiring combination with the signature topic and some | ||
// metadata to determine whether a topic value belongs to a | ||
// specific field of a given Event variant. The upside is that | ||
// indexers can use the unhashed value for meaningful topics | ||
// e.g. addresses < 32 bytes. If the prefix is included we | ||
// will always require to hash the value so need any indexer | ||
// would not be able to go from hash > address. | ||
prefix: &[], | ||
value: #field_ident, | ||
} | ||
) | ||
); | ||
let binding = quote_spanned!(span=> ref #field_ident); | ||
(binding, push_topic) | ||
}) | ||
.unzip(); | ||
|
||
let index = variant.index(); | ||
let event_signature_topic = match variant.anonymous() { | ||
true => None, | ||
false => { | ||
Some(quote_spanned!(span=> | ||
.push_topic::<::ink::env::topics::PrefixedValue<()>>( | ||
&::ink::env::topics::PrefixedValue { | ||
prefix: &<#event_ident as ::ink::reflect::EventVariantInfo<#index>>::SIGNATURE_TOPIC, | ||
value: &(), | ||
} | ||
) | ||
)) | ||
} | ||
}; | ||
|
||
let remaining_topics_ty = match variant.len_topics() { | ||
0 => quote_spanned!(span=> ::ink::env::topics::state::NoRemainingTopics), | ||
n => { | ||
quote_spanned!(span=> [::ink::env::topics::state::HasRemainingTopics; #n]) | ||
} | ||
}; | ||
|
||
quote_spanned!(span=> | ||
Self::#variant_ident { #( #field_bindings, )* .. } => { | ||
builder | ||
.build::<#remaining_topics_ty>() | ||
#event_signature_topic | ||
#( | ||
#field_topics | ||
)* | ||
.finish() | ||
} | ||
) | ||
}); | ||
|
||
quote_spanned!(span => | ||
const _: () = { | ||
impl ::ink::env::Topics for #event_ident { | ||
type Env = ::ink::env::DefaultEnvironment; // todo: configure environment? | ||
|
||
fn topics<B>( | ||
&self, | ||
builder: ::ink::env::topics::TopicsBuilder<::ink::env::topics::state::Uninit, Self::Env, B>, | ||
) -> <B as ::ink::env::topics::TopicsBuilderBackend<Self::Env>>::Output | ||
where | ||
B: ::ink::env::topics::TopicsBuilderBackend<Self::Env>, | ||
{ | ||
match self { | ||
#( | ||
#variant_match_arms | ||
)* | ||
} | ||
} | ||
} | ||
}; | ||
) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
With #1533, tests fail because of the same topics.
The
caller
andfrom
accounts are the same. It is clearly a problem with the test because the caller andfrom
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:
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})
orhash({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.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).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.
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.