Skip to content

Conversation

@lexnv
Copy link
Contributor

@lexnv lexnv commented Mar 13, 2024

This PR stabilizes the following methods:

  • transactionWatch_unstable_submitAndWatch -> transactionWatch_v1_submitAndWatch
  • transactionWatch_unstable_unwatch -> transactionWatch_v1_unwatch

Opened this PR to discuss the best path forward for transactionWatch.
We had different views about this method class on #139, and would love to get your feedback on this 🙏

cc @paritytech/subxt-team @tomaka @josepot

lexnv added 2 commits March 13, 2024 19:49
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Collaborator

jsdw commented Mar 14, 2024

My views from the other places lean in favour of stabilising this.

I think that it's good to expose such a method for when people just want to submit a TX in relative isolation. When we want to coordinate with chainHead_follow we have transaction_v1_broadcast, which plays more nicely with chainHead_follow.

While having a higher level API like Subxt/PAPI on top is recommended, I'd prefer that one could also talk directly with these JSON-RPC methods to accomplish things. Without transactionWatch, submitting a transaction and waiting for its inclusion/error feels a bit out of reach without such a higher level API on top (you need to use/understand chainHead_follow, make runtime calls to validate the TX, ask for and download potentially megabytes of block bodies and then understand how to decode those bodies, search for the tx and hash it appropriately to compare, and know when to stop searching for it in case of error (ie revalidate it periodically)).

My final thought is that this function is in a separate group from others and new light clients etc could simply opt not to implement it if it is too much effort for them.

My only reason against this method existing is that we do already have another means of submitting transactions, and so there is a part of me that does not like the fact that there are now two ways of doing something.

@tomaka
Copy link
Contributor

tomaka commented Mar 14, 2024

For what it's worth, since smoldot is backwards-compatible with the author_submitAndWatch function of the legacy API, implementing transactionWatch is very straight-forward and doesn't add much code.
Of course, in the future, smoldot could drop support for both of them at the same time.

jsdw
jsdw previously approved these changes Mar 14, 2024
@lexnv
Copy link
Contributor Author

lexnv commented Mar 21, 2024

We'll plan to merge this PR in the next few days (prob Monday) if there are no further objections.

Ideall #146 should get merged first

@jsdw jsdw merged commit 602068f into main Apr 2, 2024
@jsdw jsdw deleted the lexnv/stabilize-tx-watch branch April 2, 2024 16:48
@tomaka
Copy link
Contributor

tomaka commented Apr 3, 2024

It would have been appreciated to not merge #146 (which modifies the API) and this PR together. The point of stabilizing is to finalize a specific API. Modifying the API right before stabilizing it isn't great.

tomaka added a commit to tomaka/json-rpc-interface-spec that referenced this pull request Apr 3, 2024
@jsdw
Copy link
Collaborator

jsdw commented Apr 3, 2024

Sorry about that @tomaka! i'd forgotten that you were on holiday and had taken the lack of repsonse to mean there were no objections :)

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Apr 9, 2024
This PR ensures that the transaction API generates an `Invalid` events
for transaction bytes that fail to decode.

The spec mentioned the `Invalid` event at the jsonrpc error section,
however this spec PR makes things clearer:
- paritytech/json-rpc-interface-spec#146

While at it have discovered an inconsistency with the generated events.
The drop event from the transaction pool was incorrectly mapped to the
`invalid` event.

Added tests for the API stabilize the API soon:
- paritytech/json-rpc-interface-spec#144


Closes: #3083


cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants