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

Get event context on EventSubscription #423

Merged
merged 9 commits into from
Feb 3, 2022

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Feb 1, 2022

Currently, the next method on the EventSubscription type only returns the event itself, not clarifying where it occurred. This PR introduces a new method, next_context, that additionally returns the corresponding block hash and the index of the event.

Btw, I think it would be great to have a block number, too, but I think that'd require upstream substrate changes (or additional queries on subxt's side).

Feedback/criticism appreciated.

EDIT: Will wait for your feedback before I'll resolve the conflicts.

@jsdw
Copy link
Collaborator

jsdw commented Feb 2, 2022

I'm happy with the general idea (though I think next_context should probably return a struct with named fields at this point), although please note that I'd like to work on #413 in the coming weeks, which will probably lead to a bit of a revamp of all of this code. I'm definitely in favour of exposing all of the available information we have about events and will make sure that that is the case!

If you're keen on exposing these details sooner then I'd be happy for this to progress and get merged in the meantime.

Comment on lines 46 to 47
pub struct EventContext<T> {
pub block: T,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

Suggested change
pub struct EventContext<T> {
pub block: T,
pub struct EventContext<Hash> {
pub block_hash: Hash,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done! Also rebased to master.

@lamafab
Copy link
Contributor Author

lamafab commented Feb 2, 2022

Had to rebase twice, missed your second push to master :)

@jsdw
Copy link
Collaborator

jsdw commented Feb 2, 2022

Awesome; CI runs approved so we can see how it goes.

Given this PR and comments on #413 I'll start work on simplfying this EventSubscription API quite soon now, and I'll take into account this use case. So I'm happy to merge this when it's ready, but it may not be around for too long before the whole thing is overhauled :)

@lamafab
Copy link
Contributor Author

lamafab commented Feb 3, 2022

@jsdw I would appreciate it if you could get this merged. I'm okay if you'll eventually replace it :)

@jsdw
Copy link
Collaborator

jsdw commented Feb 3, 2022

@lamafab if you could resolve the small cargo fmt errors (run cargo +nightly fmt) and clippy then we should be good to go on this :)

@lamafab
Copy link
Contributor Author

lamafab commented Feb 3, 2022

@jsdw done, please run the workflows.

Copy link
Contributor

@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

@jsdw jsdw merged commit 9f88761 into paritytech:master Feb 3, 2022
0623forbidden pushed a commit to DEIPworld/substrate-subxt that referenced this pull request Feb 15, 2022
* implement next_context

* write test_context for method next_context

* change how events are uniquely identified

* undo local changes for test-runtime

* introduce EventContext struct

* adjust test_context to EventContext struct

* fix return type for next_context

* add suggestions by jsdw

* ran cargo fmt and clippy
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.

3 participants