-
Notifications
You must be signed in to change notification settings - Fork 262
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 archive RPCs to subxt-rpcs #1940
Conversation
// we can't have a boolean tag to denote variants. | ||
#[derive(Deserialize)] | ||
struct Response { | ||
success: bool, |
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 success flag is annoying, why do we need it at all?
result/value or error should be sufficient?
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.
Yeah true; I just added it as an extra check to catch any weird responses eg success:false but a value!
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.
isn't success part of the spec? whats the issue here
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.
the flag is useful for pattern matching because you can have a union that looks like this
S.Union(
S.Struct({
success: S.Literal(true),
result: HexString,
}),
S.Struct({
success: S.Literal(false),
error: S.String,
}),
S.Null
)
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.
Yep this is quite annoying in rust, IIRC we only left the success flag because it might be easier for js/ts realm interpret the data easier
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.
aight, I see
rpcs/src/methods/chain_head.rs
Outdated
.subscribe( | ||
"archive_unstable_storage", | ||
rpc_params![block_hash, items, child_key.map(to_hex)], | ||
"archive_unstable_storage_unsub", // TODO unsub method undefined in spec: look it up/add to spec |
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.
how can archive method have subscription? isn't that a chainhead only thing? B/c ur looking in the past right, so how can it change? whereas chainhead is constantly moving forward so it makes sense to have a subscription?
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.
We have discussed quite a bit about this in the spec, I ll try to summarize as best as I can remember: Our general approach is that an external party can craft a request that would take the server a considerable time to produce the response. To avoid such cases (DoS), we decided that subscriptions are more appropriate because they can enforce backpressure in an easy way that does not break higher-level tools (like subxt or jsonrpsee). It might be possible to add backpressure by sending chunks of the method response, but then you'd need to implement custom serializer / deserializer on top of the existing tools, and the amount of change that would require leaned towards us using subscriptions.
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.
So its basically a subscription that will only ever emit 1 value right? Interesting!
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, technically, the subscription could emit only one value, but that’s not the intended behavior. The implementation ensures backpressure by sending one "storage-lookup/item" at a time (the substrate impls does this)
Let’s take a step back: this API allows querying multiple storage items in a single request, which could generate a massive response and become a DoS risk. To mitigate this, the system enforces backpressure—ensuring that a client cannot send a few calls that are computationally expensive for the server all at once.
Without a subscription, achieving proper backpressure would require a custom implementation that process the items as long as the client can keep up in a "streaming way" producing a single JSON-RPC method response. However, general JSON-RPC libraries like jsonrpsee don’t natively support such behavior and tricky to implement especially for websocket frames.
So, while it's possible for a subscription to emit just one value, the intended behavior is to send items one by one via subscription, maintaining backpressure and preventing overwhelming responses.
subxt-rpcs
crate.TODO