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

archive: Use subscriptions for storageDiff #161

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Nov 4, 2024

This PR modifies the archive_unstable_storageDiff as a subscription instead of a method.

The change mainly leverages subscriptions' backpressure without implementing a low-level API by the RPC crates.

This is a followup from:

cc @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
pkhry
pkhry previously approved these changes Nov 5, 2024
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Where `subscription` is the value returned by this function, and `result` can be one of:

### storageDiff
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps just remove this line, since this object is no longer returned by this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the The JSON object returned by this function has the following format. Dq: is that the line mentioned? 🤔

jsdw
jsdw previously approved these changes Nov 5, 2024
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM, just a small wording tweak suggestion.

I would still like to see whether @tomaka has any further thoughts, but my general feeling is that this change is worth doing.

I also wonder how consistent this should be with archive_storage. That call contains a paginationStartKey param which I don't fully understand (given it appears to return everything at once). Should it also be a subscription so that it's able to stream the possibly large set of values found from a users request (again, rather than building up such a set in memory and then sending the whole thing)?

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv dismissed stale reviews from jsdw and pkhry via c2112e3 November 5, 2024 16:13
lexnv and others added 3 commits November 5, 2024 18:14
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
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.

4 participants