wait_for_finalized behavior if the tx dropped, usurped or invalid

[tadeohepperle]

Fixes #889 the following changes are made:
When a TxProgress gets any of the following events via RpcSubscription, the stream should end:

• Usurped
• Finalized
• FinalityTimeout
• Invalid
• Dropped
  Before this change, Usurped, Invalid and Dropped would cause functions like wait_for_finalized() and wait_for_in_block() to run indefinitely.

The OnlineClient trait bound for TxProgress impl blocks that contain wait_for_finalized() and wait_for_in_block() was not lifted, because too much depends on it. But the trait bound of the Stream impl block was downgraded to Clone.

Tests

I added a few unit tests, that generate a predetermined stream substription, create a TxProgress from it and check if the stream implementation of TxProgress behaves as desired, especially in wait_for_finalized().

[niklasad1]

this type param in doc links is needless and could be simplified?!

Suggested change

	/// Creates a new [`Subscription<Res>`].
	/// Creates a new [`Subscription`].

[niklasad1]

ideally, we should have an integration test against substrate if this behavior gets changed but should be fine for now and the old RPC API will be replaced "soon" :)

[niklasad1]

to elaborate the docs in substrate on the transaction status says one thing and the RPC implementation does something else :P

[niklasad1]

it's possible to create another subscription and watch the extrinsic again if any of the transaction statuses wasn't finalized.

For example

let xt = create_xt()
submit(xt.clone).await?;
let mut watch_ext = rpc.watch_extrinsic(xt).await?;
loop {
   match watch_xt.next().await {
       TxStatus::Finalized(_) => break Ok(()),
       other => {
           log::warning!("Watch extrinsic {xt} failed: {other}, retrying again);
           // create a new subscription
           watch_ext = rpc.watch_extrinsic(xt).await?; 
       }
   }
}

[niklasad1]

for sure complicated but possible

[tadeohepperle]

Ah okay, I did not know this, should I add your code example to the doc comment?

[niklasad1]

That code doesn't really work for the TxProgress as it wraps subscription and extrinsic.
So you can just modify your comment In those cases the stream is closed however, so you currently have no way to find out to something like In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized

I'm just picking hair here but this is tricky to understand for sure

[niklasad1]

I'm not sure whether we should provide an API for this, what do you guys think?

[jsdw]

Ooh hold on; I didn't think that it was possible to "re subscribe" to a transaction again!

This would definitely be a good thing to try out, and I think a nice API to do that (maybe some function on this TxProgress thing) would be very cool.

Lemme write an issue and we can try it out!

[tadeohepperle]

I'm not sure whether we should provide an API for this, what do you guys think?

You mentioned above that "the old RPC API will be replaced soon", so does that mean that also our stream handling will change? If that is the case, will the logic of this API need to change? If not it is certainly a good Idea to have an API for resubscribing right now, something like tx_progress.wait_until_finalized_until_timeout() that does the resubscribing under the hood.

[jsdw]

Personally I think I'd wrap this duplicated stuff into some function with a signature like:

fn mock_tx_progress(statuses: Vec<SubstrateTxStatus>) -> TxProgress

Then it'll be really easy to see the logic that matters in each test because they will just look like:

let tx_progress = mock_tx_progress(vec![SubstrateTxStatus::Ready, SubstrateTxStatus::Dropped]);
let finalized_result = tx_progress.wait_for_finalized().await;
assert!(matches!(
    finalized_result,
    Err(Error::Transaction(crate::error::TransactionError::Dropped))
));

[jsdw]

I think I'd move this to the bottom fo the comment, in part because the rest of this comment is copied from the TxStatus struct in Substrate :)

[jsdw]

Suggested change

	/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
	/// TxProgress repeatedly until the extrinsic is finalized.
	/// that this is true. In those cases the stream is closed however, so you currently have no
	/// way to find out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

[jsdw]

Suggested change

	/// that this is true. In those cases you have to re-subscribe to the extrinsic/create a new
	/// TxProgress repeatedly until the extrinsic is finalized.
	/// that this is true. In those cases the stream is closed however, so you currently have no
	/// way to find out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

[jsdw]

Also needs reverting :)

[jsdw]

Suggested change

	/// In those cases you have to re-subscribe to the extrinsic/create a new TxProgress repeatedly until the extrinsic is finalized.
	/// In those cases the stream is closed however, so you currently have no way to find
	/// out if they finally made it into a block or not.

Just to revert this since turns out you can't re-subscribe :)

[jsdw]

Looks awesome; just need to revert a few doc comments :)

[niklasad1]

nice, LGTM