Add ability to set custom metadata etc on OnlineClient #794
Conversation
subxt/src/client/online_client.rs
Outdated
| let (genesis_hash, runtime_version, metadata) = future::join3( | ||
| rpc.genesis_hash(), | ||
| rpc.runtime_version(None), | ||
| rpc.metadata(None), | ||
| // This _could_ always be the latest runtime version, but offhand it makes sense to keep |
There was a problem hiding this comment.
not sure what you mean here? Are you saying that it could be the latest because it doesn't impact anything downstream? like event decoding?
There was a problem hiding this comment.
I personally interpret the comment as the block hash (if provided) could already contain the latest metadata
There was a problem hiding this comment.
Hmm I would almost argue the comment makes things more confusing than no comment in that case
There was a problem hiding this comment.
yeah, I think you can remove this comment.
The docs and the function name itself makes this code obvious IMO
There was a problem hiding this comment.
Yeah I can remove it; what I meant was just that even if the metadata was at some specific block and the runtime version was always the latest, I don't think it would break anything related to working with older blocks (and would increase the chance that submitting transactions would work). But in the end I prefer to keep the two aligned always.
subxt/src/client/online_client.rs
Outdated
| /// to instantiate a client at such a block. | ||
| pub async fn from_rpc_client_at<R: RpcClientT>( | ||
| rpc_client: Arc<R>, | ||
| block_hash: Option<T::Hash>, |
There was a problem hiding this comment.
it's almost like we should remove this Option stuff with a enum or something which it's much more obvious than to have None which means the latest state/block.
I guess we have adopted it around how substrate RPC's works but we could do something more readable.
There was a problem hiding this comment.
I agree generally with this, but imo probably better to be consistent with substrate itself, which uses None to be latest block. Though yeah, I think substrate should probs have an enum instead
There was a problem hiding this comment.
or perhaps document that None implies the latest state
There was a problem hiding this comment.
I don't care too much about Substrate "compatibility" here, so maybe an enum is the way to go, especially with all of the other at() methods we have around (my thinking there was to provide .at() and .at_current() in those places, but maybe it's easier to just do .at(Block::Current)/.at(Block::Hash) (cc @lexnv).
Anyway, I think it's all good for now but def food for thought :)
|
As I said in the associated issue, I added |
| /// | ||
| /// Setting a custom runtime version may leave Subxt unable to | ||
| /// submit valid transactions. | ||
| pub fn set_runtime_version(&self, runtime_version: RuntimeVersion) { |
There was a problem hiding this comment.
Personally I would have the runtime_version and metadata setting in one method, rather than one each. Makes it less likely they are put out of sync, and also means we only need to acquire the lock once, when we are updating both runtime the metadata (which we should be doing). But this would work for my use case
There was a problem hiding this comment.
I did contemplate this, as I'm def aware of the extra locking and synchronisation things, but from an API perspective it just felt nicer to keep them separate and ignore the fact that it happens to lock under the hood (and the locking should be very trivial anyway in terms of time cost), and for your sake you probably only really ever need to update the Metadata and can ignore the rest :)
jsdw
changed the title
jsdw
changed the title
This allows one to instantiate clients capable of decoding older (as long as still V14) blocks and such, with suitable warnings that such clients may not be able to interact with the head of the chain and such.
Hopefully this closes #793