Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

chainHead_storage: Backport queries for value types #14551

Merged
merged 18 commits into from
Jul 21, 2023
Merged

chainHead_storage: Backport queries for value types #14551

merged 18 commits into from
Jul 21, 2023

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jul 11, 2023 •
edited
Loading

This PR backports the following RPC V2 spec changes for chainHead_storage:

  • Add a new Storage event since the ChainHeadEvent::Done has diverged
  • Test serialization/deserialization for both the added events and input parameters to the method
  • Allow users to fetch initially the value similarly as before
  • Adjust testing to reflect the new API

Closes: #14547

// @paritytech/subxt-team

@lexnv lexnv added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T2-API This PR/Issue is related to APIs. labels Jul 11, 2023
@lexnv lexnv self-assigned this Jul 11, 2023
@bkchr bkchr requested a review from skunert July 13, 2023 13:19
skunert
skunert reviewed Jul 18, 2023
View reviewed changes
client/rpc-spec-v2/src/chain_head/api.rs Show resolved Hide resolved
client/rpc-spec-v2/src/chain_head/event.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/chain_head/chain_head_storage.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/chain_head/error.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/chain_head/chain_head_storage.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/chain_head/chain_head_storage.rs
match result {
QueryResult::Buffered(storage_result) => storage_results.push(storage_result),
QueryResult::Immediate(event) => {
let _ = sink.send(&event);
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec says this about errors: "This can only be the first event generated by this subscription. No other event will be generated with this subscription.". As far as I see we continue to send events here even after some error was sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! I opened paritytech/json-rpc-interface-spec#63 in the spec to ask for some clarifications here, since we could have multiple items in the query we might still want to return the Error after processing and sending some results back to the user

Copy link
Contributor Author

@lexnv lexnv Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oki doki, have made the error event here the first event and only event submitted.
No more events would be produced now after we encounter an error here

I've opened paritytech/json-rpc-interface-spec#63 for further discussion about intermediate error events.

In the meanwhile, to make some progress here we'll stick with today's version of the spec
Thanks!

@lexnv lexnv mentioned this pull request Jul 19, 2023
Closed
skunert
skunert approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks good!

jsdw
jsdw approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@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.

Looks good to me from what I understand!

@lexnv lexnv merged commit 750fa6b into master Jul 21, 2023
@lexnv lexnv deleted the lexnv/rpc_query_value_hash branch July 21, 2023 16:56
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
@lexnv @skunert @Ank4n
chainHead_storage: Backport queries for value types (#14551)
35203d8 
* chainHead/events: Add storage params and events

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/tests: Check storage events serialization / deserialization

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/error: Add error for invalid WaitForContinue storage call

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/storage: Use new items params

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/tests: Adjust storage tests to the new API

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/events: Generalize StorageQuery by provided key

Signed-off-by: Alexandru Vasile <[email protected]>

* chain_head: Add dedicated ChainHeadStorage client for queries

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/storage: Implement queries for hashes of values

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/tests: Check storage queries for hashes of values

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead: Improve API documentation wrt multiple entries

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead/event: Rename StorageQueue ty to queue_ty

Signed-off-by: Alexandru Vasile <[email protected]>

* chianHead: Add helper to encode chainHead results as hex str

Signed-off-by: Alexandru Vasile <[email protected]>

* Update client/rpc-spec-v2/src/chain_head/error.rs

Co-authored-by: Sebastian Kunert <[email protected]>

* chainHead: Change the `queryResult` to a plain `Result`

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead: Stop producing events after the first error

Signed-off-by: Alexandru Vasile <[email protected]>

* chainHead: Change child_key to child_trie API param

Signed-off-by: Alexandru Vasile <[email protected]>

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Co-authored-by: Sebastian Kunert <[email protected]>
@lexnv lexnv mentioned this pull request Jul 24, 2023
Closed
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skunert skunert skunert approved these changes

@jsdw jsdw jsdw approved these changes

Assignees

@lexnv lexnv

Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T2-API This PR/Issue is related to APIs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RPC-Spec-V2] Storage: Backport queries for value and hash of value
3 participants
@lexnv @jsdw @skunert