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

Refactor event type decoding and declaration #221

Merged
merged 9 commits into from Jan 20, 2021

Conversation

h4x3rotab
Copy link
Contributor

@h4x3rotab h4x3rotab commented Jan 14, 2021

Fixes #196, fixes #181, fixes #28

Dynamic sized types

Before this change, the event decoder assume all the event types have fixed sizes. Some counterexamples are: Hashes, AuthorityList.

In this change, instead of decoding by skipping the fixed-length bytes, we introduce type_segmenter registry which decodes the raw event bytes with the actual scale codec. So variable length types can be handled correctly.

New attribute for pallet type definition

In the past, trait associated type is the only way to add types to the EventsDecoder implementation of a pallet. But in reality it's common that the events in a pallet references some types not defined in the trait associated types. Some examples are: IdentificationTuple and SessionIndex in Session pallet.

In this change, we introduce more attributes to add the types:

#[module]
trait Pallet: System {
    #![event_type(SomeType)]
    #![event_alias(TypeNameAlias = SomeType)]
    #![event_alias(SomeOtherAlias = TypeWithAssociatedTypes<T>)]
}

Tested

Compile with nightly-2020-10-01; smoke test to sync a full
Phala bockchain.

@h4x3rotab h4x3rotab marked this pull request as draft January 14, 2021 09:57
@h4x3rotab h4x3rotab changed the title Dynamic sized event Refactor event type decoding hand declaration Jan 14, 2021
Fixes paritytech#196, paritytech#181, #28

## Dyanmic sized types

Before this change, the event decoder assume all the event types
have fixed sizes. Some counterexamples are: Hashes, AuthorityList.

In this change, instead of decoding by skipping the fixed-length bytes,
we introduce `type_segmenter` registry which decodes the raw event
bytes with the actual scale codec. So variable length types can be
handled correctly.

## New attribute for pallet type definition

In the past, trait associated type is the only way to add types to
the EventsDecoder implementation of a pallet. But in reality it's
common that the events in a pallet references some types not defined
in the trait associated types. Some examples are: `IdentificationTuple`
and `SessionIndex` in Session pallet.

In this change, we introduce more attributes to add the types:

```rust
#[module]
trait Pallet: System {
    #![event_type(SomeType)]
    #![event_alias(TypeNameAlias = SomeType)]
    #![event_alias(SomeOtherAlias = TypeWithAssociatedTypes<T>)]
}
```

## Tested

Compile with `nightly-2020-10-01`; smoke test to sync a full
Phala bockchain.
@h4x3rotab h4x3rotab marked this pull request as ready for review January 14, 2021 17:54
@h4x3rotab h4x3rotab changed the title Refactor event type decoding hand declaration Refactor event type decoding and declaration Jan 14, 2021
@AurevoirXavier
Copy link

Any updates?

Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

In general this looks good, just a few nits.

src/events.rs Outdated Show resolved Hide resolved
src/events.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/frame/staking.rs Outdated Show resolved Hide resolved
src/events.rs Show resolved Hide resolved
@h4x3rotab
Copy link
Contributor Author

The CI failed at "pure virtual method called". I believe it's a flaky test. It looks like due to the test full node not being properly killed: openethereum/parity-ethereum#6213

@ascjones
Copy link
Member

The CI failed at "pure virtual method called". I believe it's a flaky test. It looks like due to the test full node not being properly killed: openethereum/parity-ethereum#6213

Yeah that happens fairly frequently unfortunately, see #178.

src/events.rs Outdated Show resolved Hide resolved
Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ascjones ascjones merged commit 3c46002 into paritytech:master Jan 20, 2021
@ascjones ascjones mentioned this pull request Feb 5, 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
3 participants