-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add block-centric Storage API #774
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
let block_hash = match block_hash { | ||
Some(hash) => hash, | ||
None => { | ||
client | ||
.rpc() | ||
.block_hash(None) | ||
.await? | ||
.expect("didn't pass a block number; qed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
What I mean is; all of the "old" storage methods took an Option<T::Hash>
as an arg and didn't do this; they just passed it into the relevant RPC method as-is. And if we didn't need to do this, we'd both save an API call and be able to remove the .await?
from all of the api.storage().at()
calls :)
There might need to be something like this for the new API, but we could have a little helper function which resolves the Option<T::Hash>
as part of each storage call, so that the .await?
there can handle it all :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that all said, I can see that it's nice how aligned this is with how Events
work, and it is nice to "resolve" the block hash immediately and have no ambiguity. So maybe it's good as-is! And anyway, perhaps it'll become more common to get the latest block via blocks.subscribe()
or blocks.at()
anyway.
(random thought; both this and Events could expose .at_current()
and .at(hash)
rather than an .at(Option<Hash>)
perhaps, and then .at(hash)
would not need to be async
and only .at_current()
would, which might be a nice little clarity/QoL improvement :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
We spoke about the .at().await
stuff, and for now I'm happy to merge this and let's tweak relevant APIs to be .at(hash)
/.at_current().await
for now to make this a touch nicer :)
Edit: From another PR, the suggestion came of having a different enum, so we might consider At::Current
/At::Hash(hash)
/(eventually) At::Height(number)
as an alternate idea. But this isn't pressing so let's think about it separately.
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
.fetch_keys(&key_addr.to_root_bytes(), 10, None, None) | ||
.at(None) | ||
.await? | ||
.fetch_keys(&key_addr.to_root_bytes(), 10, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, I like this new API better it's easier to read with fn at
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
pub async fn call( | ||
&self, | ||
function: String, | ||
call_parameters: Option<&[u8]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this assumes that the call_parameters
is scale encoded right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should provide the parameters scale-encoded concatenated and then apply to_hex
internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice
This looks like a nice quality-of-life-improvement but am I correct when I say that where we previously were making 1 call to |
we could change/add another function such as: |
Or change what |
The storage API is modified from a method-centric to a block-centric approach.
This change requires users to optionally specify the block hash for which to make storage queries.
This PR enables subxt in the future to make a seamless transition to the
chainHead
#766 methods.For better ergonomics, the storage client can be constructed from the main API client, as well as from the blocks API.
In this way, transitioning to the RPC SpecV2 will be seamless: the method-centric approach constructed with
.at()
method will make RPC calls to thearchive
methods, while the block-centric approach will call directly into thechainHead
class of methods.While at it, this PR adds the runtime API call RPC method that will be extended to mirror this storage API.
Part of #732.