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

Add hooks to register event types for decoding #227

Merged
merged 19 commits into from
Feb 18, 2021
Merged

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Feb 8, 2021

Fixes #201.

Provides hooks to register missing runtime type sizes either statically via the Runtime trait impl:

impl Runtime for MyCustomRuntime {
    type Signature = MultiSignature;
    type Extra = DefaultExtra<Self>;

    fn register_type_sizes(event_type_registry: &mut EventTypeRegistry<Self>) {
        event_type_registry.with_system();
        event_type_registry.with_balances();
        register_default_type_sizes(event_type_registry);
        // add more custom type registrations here:
        event_type_registry.register_type_size::<u32>("MyCustomType");
    }
}

Or dynamically via the ClientBuilder:

ClientBuilder::<NodeTemplateRuntime>::new()
    .register_type_size::<u32>("MyCustomRuntimeType")
    .build()

In addition, the build() method on the ClientBuilder will now check for missing type sizes by default, and return an Err if there are any missing. This can be disabled by skip_type_sizes_check:

ClientBuilder::<NodeTemplateRuntime>::new()
    .skip_type_sizes_check()
    .build()

Note

I consider this an intermediate solution, since all my efforts are currently focused towards the integrating scale-info with substrate which will provide richer type information for the runtimes. This would eventually remove the requirement to manually provide type information in the client.

todo

  • consider calling check_missing_type_sizes by default in ClientBuilder::build, which could be disabled by e.g. skip_type_size_check
  • Add examples/docs
  • Ensure no duplicate type size registrations

event_type_registry.with_session();
event_type_registry.with_contracts();
event_type_registry.with_sudo();
register_default_type_sizes(event_type_registry);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this implementation must be amended manually for new and removed pallets. It would be possible to rework the macros to do this but since this is a short term solution I think this is good enough.

@ascjones ascjones marked this pull request as ready for review February 10, 2021 17:27
@ascjones ascjones requested a review from dvdplm February 10, 2021 17:35
@ascjones ascjones changed the title Global registration of type segmenters for event decoding Add hooks to register event types for decoding Feb 10, 2021
@niklasad1 niklasad1 self-requested a review February 16, 2021 13:07
# Conflicts:
#	.rustfmt.toml
#	Cargo.toml
src/events.rs Outdated Show resolved Hide resolved
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@@ -72,116 +76,118 @@ impl std::fmt::Debug for RawEvent {
}
}

trait TypeSegmenter: Send {
pub trait TypeSegmenter: DynClone + Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand why the Sync bound is required, can you explain?

Copy link
Member

Choose a reason for hiding this comment

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

data: event_data,
};
/// Register a type.
pub fn register_type_size<U>(&mut self, name: &str)
Copy link
Member

Choose a reason for hiding this comment

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

document that it panics if the type is already registered

@@ -162,6 +179,9 @@ pub trait Runtime: System + Sized + Send + Sync + 'static {
type Signature: Verify + Encode + Send + Sync + 'static;
/// Transaction extras.
type Extra: SignedExtra<Self> + Send + Sync + 'static;

/// Register type sizes for this runtime
fn register_type_sizes(event_type_registry: &mut EventTypeRegistry<Self>);
Copy link
Member

Choose a reason for hiding this comment

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

ok now I see why Sync is required.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones merged commit de859e7 into master Feb 18, 2021
@ascjones ascjones deleted the aj-global-type-sizes branch February 18, 2021 10:28
///
/// *WARNING* can lead to runtime errors if receiving events with unknown types.
pub fn skip_type_sizes_check(mut self) -> Self {
self.skip_type_sizes_check = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this should be self.skip_type_sizes_check = true; ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed looks like it should be...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ascjones ascjones mentioned this pull request Feb 23, 2021
@ascjones ascjones mentioned this pull request Mar 22, 2021
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.

EventSubscription decodes all events in block
3 participants