Implement new API for `sign_and_submit_then_watch` #354

jsdw · 2021-12-03T17:28:45Z

This PR is an attempt to implement the API for subscribing to transaction state and obtaining events as described in #312.

Closes #312

jsdw · 2021-12-07T17:14:18Z

So, I'm fairly happy with this API now overall.

Some things that come out of this PR that could be addressed separately:

More docs, tests and examples of the new API. (especially examples, to show how you can dig into each stage of a transaction in more detail if needed).
Making TransactionProgress impl Stream (probably an upstream change in jsonrpsee is easiest).
Tidy up the event subscription stuff. Now that the new API doesn't need to subscribe to events, we can probably simplify and have a second look at the use cases for the separate event subscription API. (I'd like to think that we could strip it right down, and rely on a Stream impl to provide combinators for filtering and working with events).

insipx · 2021-12-07T19:28:55Z

src/client.rs

+            })
+            // If we successfully obtain the block hash we think contains our
+            // extrinsic, the extrinsic should be in there somewhere..
+            .ok_or(Error::Transaction(TransactionError::BlockHashNotFound))?;


I think this error message could be a little confusing:
Block hash containing non-finalized transaction can no longer be found

maybe 'extrinsic not found' or 'extrinsic position not found' instead?

I ended up going for "The block containing the transaction can no longer be found (perhaps it was on a non-finalized fork?)"; what do you reckon?

insipx

Looks pretty clean to me, nj

examples/submit_and_watch.rs

src/client.rs

src/error.rs

…errors

jsdw · 2021-12-08T13:54:47Z

@gregdhill @sander2 you guys may both be interested in this PR, and it would def break some stuff :)

See the examples/submit_and_watch.rs example for the high level usage. I need to expand this to show the lower level usage as well.

dvdplm

LGTM! A few nitpicks.

src/client.rs

dvdplm · 2021-12-08T16:03:18Z

src/client.rs

-pub enum TransactionProgressStatus<'client, T: Config> {
-    /// Transaction is part of the future queue.
+pub enum TransactionStatus<'client, T: Config> {
+    /// The transaction is part of the "future" queue.


Suggested change

/// The transaction is part of the "future" queue.

/// The transaction is part of the "future" queue in the transaction pool.

maybe?

I'm not sure enough on the terminology to really know! are "future" and "ready" transactions both part of the pool?

The substrate glossary for transaction pool: "A collection of transactions that are not yet included in blocks but have been determined to be valid."

Maybe Future transactions haven't been validated yet? But in the big comment, they do mention "future and ready pool"...

Anyway, shrug, I'd lean towards playing it safe and leaving it as is for now :D

Maybe @ascjones knows? Either way, fine by me to leave it as is.

This distinction is new to me TBH

src/client.rs

ascjones

LGTM

ascjones · 2021-12-09T11:03:06Z

src/client.rs

-pub enum TransactionProgressStatus<'client, T: Config> {
-    /// Transaction is part of the future queue.
+pub enum TransactionStatus<'client, T: Config> {
+    /// The transaction is part of the "future" queue.


This distinction is new to me TBH

ascjones · 2021-12-09T11:09:08Z

src/client.rs

+///    - `Retracted`
+/// 5. Block finalized:
+///    - `Finalized`
+///    - `FinalityTimeout`


Nice categorisation. Would it bring any value to bring this grouping to the type level, instead of the simple 1 => 1 mapping with TransactionStatus? i.e. split the groups into their own enums combined as variants in the top level enum.

I reckon there are few enough "groups", and things can move between them easily enough, that the flat enum is probably easier to work with (since one mode of use involves matching on variants), and has the nice benefit of aligning very closely with thr substrate version, and so easy to keep in sync going forwards :)

ascjones · 2021-12-09T11:15:45Z

src/client.rs

@@ -280,3 +284,416 @@ where
        Ok(signed)
    }
 }
+
+/// This struct represents a subscription to the progress of some transaction, and is


I reckon all this stuff deserves it's own file, shouldn't be in client.rs

I actually did do this initially. I like the separation; I just wasn't keen on the circular dependency it would introduce (client makes a transactionprogress, transactionprogress et al have a reference to client) :)

(I think I'll give it a try anyway though, because it would be nice breaking that file down!)

@dvdplm

* WIP Implementing new event subscription API * back to lifetimes, fix example * no more need for accept_weak_inclusion * thread lifetime through to prevent 'temporary dropped' issue * make working with events a little nicer * Get tests compiling * fmt and clippy * _name back to name * dont take ownership, just have stronger note * Attempt to fix test * remove commented-out code * Add a couple more helper methods and a test * Remove custom ExtrinsicFailed handling; treat them like other events * Handle runtime errors in TransactionProgress related bits * cargo fmt + clippy * Fix some of the failing tests * remove unused import * fix transfer_error test * Fix compile errors against new substrate latest * Comment tweaks, and force test-runtime rebuild * Drop the TransactionProgress subscription when we hit 'end' statuses * cargo fmt * find_event to find_first_event and helper to return all matching events * TransactionProgressStatus to TransactionStatus * Copy and improve docs on TransactionStatus from substrate * debug impl for Client to avoid manual debug impls elsewhere * Add and tweak comments, specifically a note about block inclusion on errors * clippy + fmt * Fix docs * Ignore 'error' statuses and adhere to the substrate docs * tweak and improve some comments per @dvdplm's suggestions * Break transaction* structs into separate file * fmt and fix doc link

jsdw added 20 commits December 3, 2021 16:28

WIP Implementing new event subscription API

388a335

back to lifetimes, fix example

aa27ce4

no more need for accept_weak_inclusion

fe40bab

thread lifetime through to prevent 'temporary dropped' issue

c802499

make working with events a little nicer

4187e65

Get tests compiling

50f45b1

fmt and clippy

4255c3f

_name back to name

66420fe

dont take ownership, just have stronger note

7913613

Attempt to fix test

a52b296

remove commented-out code

970d7ed

Add a couple more helper methods and a test

36ad6a2

Remove custom ExtrinsicFailed handling; treat them like other events

c44cc60

Handle runtime errors in TransactionProgress related bits

b51e43a

cargo fmt + clippy

3102267

Fix some of the failing tests

0176d07

remove unused import

1032e93

fix transfer_error test

a42dfc3

Fix compile errors against new substrate latest

50a309a

Comment tweaks, and force test-runtime rebuild

9407ef9

jsdw marked this pull request as ready for review December 7, 2021 17:14

jsdw requested a review from ascjones December 7, 2021 17:14

jsdw added 2 commits December 7, 2021 17:35

Drop the TransactionProgress subscription when we hit 'end' statuses

ec3050c

cargo fmt

b2e61d2

insipx reviewed Dec 7, 2021

View reviewed changes

insipx approved these changes Dec 7, 2021

View reviewed changes

dvdplm reviewed Dec 7, 2021

View reviewed changes

jsdw mentioned this pull request Dec 8, 2021

Subxt integration tests failed against latest Substrate build. #357

Closed

find_event to find_first_event and helper to return all matching events

4c66bab

jsdw added 6 commits December 8, 2021 11:04

TransactionProgressStatus to TransactionStatus

7f92f46

Copy and improve docs on TransactionStatus from substrate

5f9ce69

debug impl for Client to avoid manual debug impls elsewhere

f05e68a

Add and tweak comments, specifically a note about block inclusion on …

8ba531a

…errors

clippy + fmt

c161d87

Fix docs

19a2116

Ignore 'error' statuses and adhere to the substrate docs

2afd9ac

dvdplm approved these changes Dec 8, 2021

View reviewed changes

tweak and improve some comments per @dvdplm's suggestions

41ef757

ascjones approved these changes Dec 9, 2021

View reviewed changes

jsdw added 2 commits December 9, 2021 12:00

Break transaction* structs into separate file

2a4c4b4

fmt and fix doc link

f4f113e

jsdw merged commit 5db9b73 into master Dec 9, 2021

jsdw deleted the jsdw-events branch December 9, 2021 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new API for `sign_and_submit_then_watch` #354

Implement new API for `sign_and_submit_then_watch` #354

jsdw commented Dec 3, 2021 •

edited

Loading

jsdw commented Dec 7, 2021

insipx Dec 7, 2021

jsdw Dec 8, 2021

insipx left a comment

jsdw commented Dec 8, 2021

dvdplm left a comment

dvdplm Dec 8, 2021

jsdw Dec 8, 2021

dvdplm Dec 8, 2021

ascjones Dec 9, 2021

ascjones left a comment

ascjones Dec 9, 2021

ascjones Dec 9, 2021

jsdw Dec 9, 2021

ascjones Dec 9, 2021

jsdw Dec 9, 2021

jsdw Dec 9, 2021

	/// The transaction is part of the "future" queue.
	/// The transaction is part of the "future" queue in the transaction pool.

Implement new API for sign_and_submit_then_watch #354

Implement new API for sign_and_submit_then_watch #354

Conversation

jsdw commented Dec 3, 2021 • edited Loading

jsdw commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insipx left a comment

Choose a reason for hiding this comment

jsdw commented Dec 8, 2021

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement new API for `sign_and_submit_then_watch` #354

Implement new API for `sign_and_submit_then_watch` #354

jsdw commented Dec 3, 2021 •

edited

Loading