Update should be exposed externally

[kylezs]

In general, the OnlineClient is much too opinionated about how runtime updates should be handled, a small thing that would help this is allowing us to pass in an Update to apply_update. This method is currently public, but it can't be passed in Update to call it from outside, effectively making it private.

[niklasad1]

I don't really follow what you refer that when you say that it's OnlineClient is much too opinionated about how runtime updates should be handled, can you be more specific please?

Sure, of course Update should be exported and be public that's a nit.
Are you willing to open a PR for that?

[kylezs]

I don't really follow what you refer that when you say that it's OnlineClient is much too opinionated about how runtime updates should be handled, can you be more specific please?

The design seems to want me to use the subscription and update model in a separate thread, but this isn't necessarily always what one would want to do, for a couple of reasons, like race conditions (or more precisely being able to reason about the correctness of the program, ensuring it's free of race conditions), or when combined with wanting to query updates from the past, which requires you to ensure the metadata on the client is set to that past runtime's metadata. I feel like the current API is kind of working against me.

Happy to open a PR to expose Update (will do tomorrow)

[niklasad1]

The design seems to want me to use the subscription and update model in a separate thread, but this isn't necessarily always what one would want to do, for a couple of reasons, like race conditions (or more precisely being able to reason about the correctness of the program, ensuring it's free of race conditions), or when combined with wanting to query updates from the past, which requires you to ensure the metadata on the client is set to that past runtime's metadata. I feel like the current API is kind of working against me.

Ok, I see.
You don't really have to spawn a separate task/thread for updating, you could have that logic in your main loop to update when you have no other pending calls or whatever but it would require you to "block at some point" and the read update stream then apply the updates if any new updates has been sent but it requires some async runtime.

As this API is a wrapper around the runtime subscription RPC it's no way around the async stuff rather then to poll state_metadata directly which is super expensive.

//cc @jsdw @lexnv any other ideas

[kylezs]

rather then to poll state_metadata directly which is super expensive.

Might help if I explain the approach I was going to take. Basically the way I was going to work around this, is rather than to poll for metadata each time, just poll for runtime-version. If runtime version is different to the one in the client, only then would querying for metadata occur, and then it would be set to this in the client (using apply_update, hence this issue) In this way, can ensure that Event deserialization won't fail ever (due to metadata or runtime-version at least), not due to a race condition between the even reading thread and the runtime version updater thread, nor due to some caller deciding to query at a block height below the previous runtime upgrade, because we could then do a metadata query at that block height (which is a change that exists in main but not in release, currently - in 0.25.0 - rpc().metadata() doesn't take an at param)

[kylezs]

Put up #791

[jsdw]

Just to check; the current API allows you to do something like this:

let updater = client.subscribe_to_updates();
let runtime_update_stream = updater.runtime_updates(); 

// Just to show that we are subscribed to runtime updates; you may want to call `.next()` in your own time.
while let Ok(update) = runtime_update_stream.next().await {
    // Once you have an update you can apply it when you choose:
    updater.apply_update(update)?;
}

I can see that Update isn't exposed and so you can't name that type, and we should fix that for sure (Edit: ah you've opened said PR; thank you!).

The fields in Update are not pub at the moment. Perhaps they could be. The reason not to do this so far is to help ensure that the metadata and runtime version line up together, although I can certainly see some scope to make this whole API more flexible.

[kylezs]

Yeah, so that example works fine for when you only need to worry about runtimes going forward in time, but as soon as you need to query for events before the latest runtime, it's not clear the best way to do that with the current API. You almost need to split the logic, which is what I'm trying to avoid. By querying runtime-version each time and maybe metadata if the runtime version is different, this works no matter which block you query at, whether it's a block behind in runtime version or ahead.

Good point re: the fields not being pub, for my use case, I'd also need that to be the case, since I'd need to construct it somehow other than than via the subscription.