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

Query At block #76

Merged
merged 8 commits into from
Aug 19, 2024
Merged

Query At block #76

merged 8 commits into from
Aug 19, 2024

Conversation

S0c5
Copy link
Member

@S0c5 S0c5 commented Aug 17, 2024

No description provided.

Copy link
Member

@olanod olanod left a comment

Choose a reason for hiding this comment

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

now we have query_storage with the added block option but also we already had query_storage_at, sounds like redundancy so we should kill either of the two. I would also suggest accepting a block number instead of a hash since 99% of the time the client will want to provide a number instead of hash and will end up resolving the block to the hash on their own, we should save them that work ;)

@S0c5
Copy link
Member Author

S0c5 commented Aug 18, 2024

remove query_storage_at or query_storage

I understand the Backend trait as a mapping for the underlying rpc methods, while we can only have query_storage_at given it is specific to retrieve multiple keys, the trade off here is an overhead for the implementations that needs to retrieve a single value but have to deal with this:

virto-sdk/sube/src/lib.rs

Lines 395 to 399 in 92398ce

#[derive(Deserialize, Serialize, Debug)]
pub struct StorageChangeSet {
block: String,
changes: Vec<[String; 2]>,
}

So, I'd leve the both methods given they have different goals.

Working on the block number!

sube/src/lib.rs Outdated
@@ -405,11 +411,19 @@ pub struct StorageChangeSet {
/// }
/// ```
pub trait Backend {
async fn query_storage_at(
async fn get_storage_items(
&self,
keys: Vec<String>,
block: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have this only as a number(e.g. u32) and do resolution internally in the RPC implementations . 99% of the time users want to use a block number instead of a hash. If we were to use a hash we should also accept it as a byte array instead of string since the hex encoding of stuff is very JSON RPC specific.

@S0c5 S0c5 merged commit 864afab into main Aug 19, 2024
2 checks passed
@S0c5 S0c5 deleted the feature/query-at-block branch August 19, 2024 12:34
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.

2 participants