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.

Sign up for GitHub

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

Jump to bottom

implement variant of subscription that returns finalized storage changes #237

Merged
merged 1 commit into from
Mar 10, 2021
Merged

implement variant of subscription that returns finalized storage changes #237

merged 1 commit into from
Mar 10, 2021

Conversation

gregdhill
Copy link
Contributor

@gregdhill gregdhill commented Mar 4, 2021 •
edited
Loading

Signed-off-by: Gregory Hill gregorydhill@outlook.com

Closes #238

  • Adds RPC method to subscribe to finalized block headers and retrieve storage changes per block.
  • Update extrinsic watcher to wait for finalization.
  • Also fixes the method name for the following unsubscribe calls...

https://github.com/paritytech/substrate-subxt/blob/9959f0d29950219f20b5cc79dfff46f8987239fc/src/rpc.rs#L465
https://github.com/paritytech/substrate-subxt/blob/9959f0d29950219f20b5cc79dfff46f8987239fc/src/rpc.rs#L481

@gregdhill gregdhill mentioned this pull request Mar 4, 2021
Open
@gregdhill gregdhill force-pushed the finalized-event-sub branch from 7ca22c1 to 5ab0ed3 Compare March 5, 2021 09:01
gregdhill
gregdhill commented Mar 5, 2021
View reviewed changes
src/rpc.rs Outdated Show resolved Hide resolved
@gregdhill
Copy link
Contributor Author

Hmm this feature might need to wait until after the jsonrpsee V2 upgrade since for some reason the finalized block subscription isn't dropped...

@ascjones
Copy link
Contributor

ascjones commented Mar 8, 2021

I've merged #214 so you can update this branch now

@gregdhill gregdhill force-pushed the finalized-event-sub branch 2 times, most recently from 34ed3e4 to 7ebf91e Compare March 8, 2021 15:11
@gregdhill
Copy link
Contributor Author

@ascjones please review

ascjones
ascjones reviewed Mar 9, 2021
View reviewed changes
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.

Overall looks good. As mentioned I do think we should still allow subscribing to non finalized events when submitting an extrinsic still if we want to trade immediacy for finality.

Was just looking at the polkadot.js api an they do it slightly differently with a status field: https://polkadot.js.org/docs/api/start/api.tx.subs/, but it's a bit more low level than what we have here.

src/rpc.rs Outdated Show resolved Hide resolved
src/rpc.rs Outdated Show resolved Hide resolved
src/subscription.rs Show resolved Hide resolved
src/rpc.rs Outdated Show resolved Hide resolved
src/subscription.rs Outdated Show resolved Hide resolved
@gregdhill gregdhill force-pushed the finalized-event-sub branch from 7ebf91e to d8ac8ff Compare March 9, 2021 17:00
@gregdhill
Copy link
Contributor Author

@ascjones added a workaround to support configuration of the event subscription on submit_and_watch_extrinsic, let me know what you think!

ascjones
ascjones reviewed Mar 9, 2021
View reviewed changes
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.

Yes that API looks fine, I'd like the comments expanding so it's clear exactly what is going on to avoid footguns.

src/lib.rs Show resolved Hide resolved
src/rpc.rs Show resolved Hide resolved
src/rpc.rs Show resolved Hide resolved
src/rpc.rs
}

/// Subscribe to finalized events.
pub async fn subscribe_finalized_events(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps instead of the two different methods we could just check the weak_inclusion flag like we do in the submit transaction method. Though you would need different client instances if you wanted to do both in that case. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now it's probably cleaner with two distinct methods as we also have subscribe_blocks and subscribe_finalized_blocks.

Copy link
Contributor Author

@gregdhill gregdhill Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Anyway I think we should remove accept_weak_inclusion in favour of some config on the generated traits (like how it is done in polkadot{.js}) pending refactoring of the proc-macro crate.

@gregdhill gregdhill force-pushed the finalized-event-sub branch from d8ac8ff to 3e22f0d Compare March 10, 2021 10:01
ascjones
ascjones reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Branch needs updating with master.
Edit I see already done

src/subscription.rs Outdated Show resolved Hide resolved
@gregdhill
implement variant of subscription that returns finalized storage changes
cc29156 
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@gregdhill gregdhill force-pushed the finalized-event-sub branch from 3e22f0d to cc29156 Compare March 10, 2021 10:05
@ascjones
Copy link
Contributor

BTW the force pushes make it hard to review updates because I can't see just the changes since I last reviewed. Merge commits are fine.

ascjones
ascjones approved these changes Mar 10, 2021
View reviewed changes
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.

Great work!

@ascjones ascjones merged commit 568d6ea into paritytech:master Mar 10, 2021
@gregdhill gregdhill deleted the finalized-event-sub branch March 10, 2021 10:47
@ascjones ascjones mentioned this pull request Mar 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ascjones ascjones ascjones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

event subscription does not ensure finalization
2 participants
@gregdhill @ascjones