New Event Subscription API #442

jsdw · 2022-02-09T10:59:21Z

This PR introduces a new Event Subscription API, which aims to be simpler and more flexible.

Some notes:

Subscribing to events is now done using api.events().subscribe() or api.events.subscribe_finalized().
The EventSubscription you get back implements Stream, so you can apply StreamExt combinators to it.
The transaction stuff now leans on the same code to obtain events.
EventSubscription hands back Events objects, which contain yet-to-be-decoded event data. We can decode statically using the events from the generated runtime code, or dynamically to search for specific events (or just return the raw bytes for all).
We can always see the block_hash() that Events come from, and event indexes, as well as Phase.
Modify subscription examples to clarify separation from subscription and actually doing things, and show how the Stream api can be used to filter events.

closes #413

dvdplm

(partial review)

examples/examples/custom_type_derives.rs

examples/examples/subscribe_all_events.rs

dvdplm · 2022-02-10T19:17:30Z

examples/examples/subscribe_one_event.rs

+        .filter_map(|events| future::ready(events.ok()))
+        // Map events to just those we care about:
+        .flat_map(|events| {
+            let transfer_events = events
+                .find::<polkadot::balances::events::Transfer>()
+                .collect::<Vec<_>>();
+            stream::iter(transfer_events)
+        });


I wonder if these two are common enough usage patterns that it would make sense to have them packaged up into a utility method of some sort. It would be nice to be able to do let transfer_events = api.events().subscribe().to<events::Transfer>().await?; or something along those lines.

I had that exact thought; it felt like it would be a nice API to add and I did ponder it, but I wasn’t sure enough on how people actually would use events to know whether to go ahead and add it or wait for a feature request to come in :)

(I think one thing I started pondering was; how likely is it that people would care about plucking out more than one event type, and then, should I try to support that (harder to do) here to avoid people creating multiple subscriptions? In fact, a general optimisation would be to have only one shared subscription in the background for some of these things to make that more palettable...)

In most cases I would use such API since I only care about one event. However here is a case where I need two events: https://github.com/paritytech/cargo-contract/blob/0e9ffe78947f458c89c5ee0ef25234a146b797d9/src/cmd/extrinsics/instantiate.rs#L246.

Maybe tuples of event types would be one idea?

Tuples would be the ideal way to provide the type params to the function, but the return type would need to be more sum-type in nature as we'll get back just one of the possible types each iteration. I have a couple of thoughts, but I think it'd be good to merge this first and then I can add this functionality in a separate PR so that it can be given a little more thought!

Absolutely! Defo not a job for this PR was just brainstorming here since it was brought up.

Hehe, I suppose I was going to just add it if it was simple enough (ie the one-event variation)! Thanks for your thoughts; very useful :))

subxt/src/client.rs

ascjones

WIP review

subxt/tests/integration/events.rs

codegen/src/api/calls.rs

examples/examples/subscribe_all_events.rs

subxt/src/events.rs

ascjones

Just brilliant, have wanted Stream for events for a long time 🙏

subxt/src/events.rs

…ff bytes

ascjones

LGTM

dvdplm

LGTM, this is a big chunk of work, nice job.

One question I have is if it would make sense to separate the a static API from the dynamic one more clearly. It might not be obvious to a user of find() that they are using dynamic decoding. OTOH forcing users to use, dunno, dynamic_find() would be pretty annoying.

dvdplm · 2022-02-12T21:34:18Z

subxt/src/events.rs

+                Some(Ok(block_header)) => {
+                    use sp_runtime::traits::Header;
+                    // Note [jsdw]: We may be able to get rid of the per-item allocation
+                    // with https://github.com/oblique/reusable-box-future.


That looks pretty neat.

dvdplm · 2022-02-12T21:48:02Z

subxt/src/events.rs

+                let mut decode_one_event = || -> Result<_, BasicError> {
+                    let phase = Phase::decode(cursor)?;
+                    let ev = Evs::decode(cursor)?;
+                    let _topics = Vec::<T::Hash>::decode(cursor)?;


Are we just tossing these away? Can we do better (at later date)?

Answering myself here: no we can't do better and topics are not used for anything in modern substrate chains

dvdplm · 2022-02-12T21:52:09Z

subxt/src/events.rs

+            if start_len == 0 || self.num_events == index {
+                None
+            } else {
+                match decode_raw_event_details::<T>(self.metadata, index, cursor) {


How much slower is it to do this dynamically vs statically?

I've no idea; it did cross my mind to benchmark this at some point but I haven't gotten around to it! As a completely random plucked-out-of-air guess, I'd guess maybe 5-10x slower!

I think we should do some measurements of this so we can provide users with some guidelines and annotate the docs with "NOTE: if performance is a concern, consider using … … instead"?

dvdplm · 2022-02-12T21:58:57Z

subxt/src/events.rs

+
+    /// Iterate through the events using metadata to dynamically decode and skip
+    /// them, and return only those which should decode to the provided `Ev` type.
+    pub fn find<Ev: Event>(&self) -> impl Iterator<Item = Result<Ev, BasicError>> + '_ {


Worth documenting how this behaves on error (returns None after first error?)?

dvdplm · 2022-02-12T22:00:47Z

subxt/src/events.rs

+/// A decoded event and associated details.
+#[derive(Debug, Clone, PartialEq)]
+pub struct EventDetails<Evs> {
+    /// When was the event produced?


Suggested change

/// When was the event produced?

/// During which `[Phase]` was the event produced?

dvdplm · 2022-02-12T22:03:21Z

subxt/src/events.rs

+    pub event: Evs,
+}
+
+/// The raw bytes for an event with associated details.


Suggested change

/// The raw bytes for an event with associated details.

/// The raw bytes for an event with associated details about where and when it was emitted.

dvdplm · 2022-02-12T22:06:01Z

subxt/src/events.rs

+    // topics come after the event data in EventRecord. They aren't used for
+    // anything at the moment, so just decode and throw them away.


Reading paritytech/substrate#2491 I wonder if topics will become a topic again (sorry) now that smoldot is a thing?

At least we properly decode them, so things won't break if they do start being used! (though I expect other stuff may have changed that would be breaking by then...)

dvdplm · 2022-02-12T22:11:22Z

subxt/src/events.rs

+        let metadata = metadata::<Event>();
+
+        // Encode our events in the format we expect back from a node, and
+        // construst an Events object to iterate them:


Suggested change

// construst an Events object to iterate them:

// construct an Events object to iterate them:

subxt/src/events.rs

subxt/src/transaction.rs

jsdw · 2022-02-14T11:12:37Z

LGTM, this is a big chunk of work, nice job.

One question I have is if it would make sense to separate the a static API from the dynamic one more clearly. It might not be obvious to a user of find() that they are using dynamic decoding. OTOH forcing users to use, dunno, dynamic_find() would be pretty annoying.

I've added notes to all of the methods to try and make it clear. I guess I'm not too fussed about making the dynamic bit clear in those method names because there is only one option to choose from (dynamic), and dynamic is more lenient anyway, so there is no significant disadvantage to doing so (I'll have to benchmark the performance; think I'll add an issue for benchmarks).

* Add reworked event types * first pass implementing event subscriptions * make clear that some methods are private * comment tidy * use Events in transaction stuff * align transaction and event APIs * remove __private_ prefixes; they are ugly * fix examples and remove old events and subscription code * better comments on hidden event functions * re-add find_first_event; it's used a bunch in tests and examples * cargo check --all-targets now passes * Fix up existing event tests * cargo fmt * change todo to note * clippy and doc niggles * revert to find_first_event * Add specific subscription related tests * cargo fmt * Update tests and add/fix examples * cargo fmt * add a little to subscribe_all_events example * cargo fmt * move an example comment * easy access to root mod for more clarity * add a couple of tests to ensure that events properly decoded until naff bytes * Simplify EventSubscription Stream impl a little * Address some PR feedback

jsdw added 20 commits February 7, 2022 16:57

Add reworked event types

18b95a4

first pass implementing event subscriptions

18482d8

make clear that some methods are private

644b638

comment tidy

3443e6c

use Events in transaction stuff

589d1a6

align transaction and event APIs

8627117

remove __private_ prefixes; they are ugly

9c409a6

fix examples and remove old events and subscription code

72e66a9

better comments on hidden event functions

1273c7e

re-add find_first_event; it's used a bunch in tests and examples

63a4e1a

cargo check --all-targets now passes

9ac234f

Fix up existing event tests

9a40790

cargo fmt

d9bc620

change todo to note

457cd6e

clippy and doc niggles

b85acf1

revert to find_first_event

9d99ddb

Add specific subscription related tests

ea71f74

cargo fmt

5b1ed7b

Update tests and add/fix examples

221135c

cargo fmt

4d0085d

jsdw marked this pull request as ready for review February 10, 2022 15:13

jsdw requested review from ascjones and a team February 10, 2022 15:13

jsdw mentioned this pull request Feb 10, 2022

Simplify and Streamify EventSubscription #413

Closed

jsdw added 2 commits February 10, 2022 16:20

add a little to subscribe_all_events example

1d202b3

cargo fmt

a74c8ed

lexnv approved these changes Feb 10, 2022

View reviewed changes

dvdplm reviewed Feb 10, 2022

View reviewed changes

move an example comment

e20d171

ascjones reviewed Feb 11, 2022

View reviewed changes

subxt/tests/integration/events.rs Show resolved Hide resolved

codegen/src/api/calls.rs Outdated Show resolved Hide resolved

examples/examples/subscribe_all_events.rs Show resolved Hide resolved

subxt/src/events.rs Show resolved Hide resolved

ascjones approved these changes Feb 11, 2022

View reviewed changes

subxt/src/events.rs Outdated Show resolved Hide resolved

subxt/src/events.rs Show resolved Hide resolved

subxt/src/events.rs Outdated Show resolved Hide resolved

jsdw added 2 commits February 11, 2022 16:26

easy access to root mod for more clarity

aa96d8d

add a couple of tests to ensure that events properly decoded until na…

18ca3be

…ff bytes

ascjones approved these changes Feb 11, 2022

View reviewed changes

Simplify EventSubscription Stream impl a little

4056f29

dvdplm approved these changes Feb 12, 2022

View reviewed changes

Address some PR feedback

ef96407

jsdw merged commit b1b7173 into master Feb 14, 2022

jsdw deleted the jsdw-event-subscription branch February 14, 2022 11:18

lexnv mentioned this pull request Feb 14, 2022

Improve documentation #449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Event Subscription API #442

New Event Subscription API #442

jsdw commented Feb 9, 2022 •

edited

Loading

dvdplm left a comment

dvdplm Feb 10, 2022

jsdw Feb 10, 2022

jsdw Feb 11, 2022 •

edited

Loading

ascjones Feb 11, 2022

jsdw Feb 11, 2022 •

edited

Loading

ascjones Feb 11, 2022

jsdw Feb 11, 2022

ascjones left a comment

ascjones left a comment

ascjones left a comment

dvdplm left a comment

dvdplm Feb 12, 2022

dvdplm Feb 12, 2022

dvdplm Feb 13, 2022

dvdplm Feb 12, 2022

jsdw Feb 14, 2022

dvdplm Feb 14, 2022

dvdplm Feb 12, 2022

dvdplm Feb 12, 2022

dvdplm Feb 12, 2022

dvdplm Feb 12, 2022

jsdw Feb 14, 2022

dvdplm Feb 12, 2022

jsdw commented Feb 14, 2022

	/// When was the event produced?
	/// During which `[Phase]` was the event produced?

	/// The raw bytes for an event with associated details.
	/// The raw bytes for an event with associated details about where and when it was emitted.

		// topics come after the event data in EventRecord. They aren't used for
		// anything at the moment, so just decode and throw them away.

	// construst an Events object to iterate them:
	// construct an Events object to iterate them:

New Event Subscription API #442

New Event Subscription API #442

Conversation

jsdw commented Feb 9, 2022 • edited Loading

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Feb 14, 2022

jsdw commented Feb 9, 2022 •

edited

Loading

jsdw Feb 11, 2022 •

edited

Loading

jsdw Feb 11, 2022 •

edited

Loading