search

paritytech / json-rpc-interface-spec

30 stars 3 forks source link

Stabilize `transaction` to version 1 #139

Closed lexnv closed 8 months ago

lexnv commented 9 months ago

This PR stabilizes the following methods:

cc @paritytech/subxt-team @tomaka @josepot

jsdw commented 8 months ago

@lexnv we could tweak the PR to just stabilise transaction if we are all OK with that one, and then transactionWatch can be separately discussed and such?

My view on transactionWatch is that it's nice to have because if I'm eg writing a CLI tool or whaetevr to submit a TX and monitor its progress, it's way easier than using transaction, downloading block bodies, decoding things etc in order to provide said progress updates. In other words, transaction is supposed to be used with chainHead_follow really, so that you can download block bodies and do said checks, but transactionWatch is perhaps more aimed at being used as a standalone call (although I expect @tomaka can comment much better on the intended usage of it).

tomaka commented 8 months ago

I'm conflicted about transactionsWatch because it's indeed very convenient for small tools. Replicating the behaviour of transactionsWatch on top of chainHead and transactions is possible but pretty complicated.

lexnv commented 8 months ago

I've changed this PR to stabilize only the transaction API for now (this includes the transaction_broadcast and transaction_stop). Thanks for the reviews everyone 🙏

From my perspective, I do like the idea of having a transactionWatch because it offers an easy way for developers to get the status of the transaction. I see the transaction API as a low-level alternative that requires a bit more work. Developers opting for the transaction API would correlate if the transaction is finalized by decoding block bodies. We currently rely on the chainHead methods to subscribe to blocks and fetch block's body. There's no guarantee at the moment that if a node exposes the transaction API, it will also expose the chainHead API. The transaction API could be implemented in a few hundred lines (from subp2p-explorer).

Would love to get your thoughts on this 🙏

tomaka commented 8 months ago

I completely agree with that guiding principle, and to me transactionWatch does too much magic in the background for my liking.

Also, there are cases that are not properly covered. For instance: what happens when if the consumer receives a bestChainBlockIncluded event and then, right after emitting that event, the "server" realizes that block is no longer considered to be part of the "best blocks", and the server is still figuring out where in the new best branch of blocks the tx is located...? Shouldn't the API in this case let the consumer know that the transaction is no-longer considered to be inside a best-block? What would be the rigth way to do that?

Given that different servers might be at different best blocks, and that the client has no way to know which best block the server has, it doesn't matter at all what the server does in that specific situation.

The client expects the server to follow the chain and report the latest status of the transaction. The server is allowed to do less than this, and report the status less quickly/frequently than expected. Whether this is too magic depends on the small details of what exactly is considered magic. There are several underlying reasons why magic should in general be avoided (more complexity, surprising behaviors, etc.), and I don't really think any of these reasons apply here.

jsdw commented 8 months ago

This PR is only about stabilising transaciton now, so I'll merge this is no objections from anybody (and we'll open a separate PR for transactionWatch)?