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

Remove wait_for_event and simplify event access in subscribe_events #437

Merged
merged 19 commits into from
Jan 23, 2023

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Jan 12, 2023

Removes the obsolete wait_for_event functions. I don't see the use case of these functions any more.

In addition, the event subscription has been updated:

  • Renames subscribe_frame_events to subscribe_events and moved to more fitting event.rs instead of the frame_system file.
  • Instead of returning the StorageChangeSet, which required quite some type insight to retrieve the events from it, a struct EventSubscription is returned. Upon next_event it directly tries to retrieve the events.

closes #413

@haerdib haerdib added F7-enhancement Enhances an already existing functionality E1-breaksnothing labels Jan 12, 2023
@haerdib haerdib self-assigned this Jan 12, 2023
@haerdib haerdib changed the title Exchange wait_for_event with subscribe_for_event_type Remove wait_for_event and simplify events access in subscribe_events. Jan 13, 2023
@haerdib haerdib changed the title Remove wait_for_event and simplify events access in subscribe_events. Remove wait_for_event and simplify events access in subscribe_events Jan 13, 2023
@haerdib haerdib changed the title Remove wait_for_event and simplify events access in subscribe_events Remove wait_for_event and simplify event access in subscribe_events Jan 13, 2023
@haerdib haerdib marked this pull request as ready for review January 13, 2023 10:03
src/api/rpc_api/frame_system.rs Show resolved Hide resolved
testing/examples/frame_system_tests.rs Show resolved Hide resolved
testing/examples/frame_system_tests.rs Show resolved Hide resolved
src/api/rpc_api/events.rs Show resolved Hide resolved
Copy link
Contributor

@Niederb Niederb left a comment

Choose a reason for hiding this comment

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

I like the new EventSubscription. I think it really makes the event extraction easier.
One thing that is not quite clear to me is how the EventDetails and EventRecord relate as they seem quite similar. Is it a possibility to get rid of EventDetails?

testing/examples/frame_system_tests.rs Show resolved Hide resolved
examples/examples/event_callback.rs Show resolved Hide resolved
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me! But we should clarify the drop stuff.

examples/examples/event_callback.rs Show resolved Hide resolved
testing/examples/frame_system_tests.rs Show resolved Hide resolved
@haerdib
Copy link
Contributor Author

haerdib commented Jan 18, 2023

One thing that is not quite clear to me is how the EventDetails and EventRecord relate as they seem quite similar. Is it a possibility to get rid of EventDetails?

True enough. That might be something one could look into. EventDetails has been copy pasted from subxt. It's basically a helper / wrapper struct. EventRecord is the Substrate type. But that should be investigated in a different issue.

EventDetails does contain quite some convenience features. But maybe it's an overkill. Will create an issue for that.

See: #440

Copy link
Contributor

@Niederb Niederb 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 for the clarification in the comment and the opening of the issue 👍

Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks a lot, lgtm now!

@haerdib haerdib merged commit 7dd79e4 into master Jan 23, 2023
@haerdib haerdib deleted the bh/413-redo-wait-for-events branch January 23, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2-breaksapi F7-enhancement Enhances an already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate usage of wait_for_event and wait_for_event_details
3 participants