Skip to content

Conversation

@lexnv
Copy link
Contributor

@lexnv lexnv commented Feb 26, 2024

This PR stabilizes the following methods:

  • transaction_unstable_broadcast -> transaction_v1_broadcast
  • transaction_unstable_stop -> transaction_v1_stop

cc @paritytech/subxt-team @tomaka @josepot

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Feb 26, 2024
jsdw
jsdw previously approved these changes Mar 4, 2024
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm game to stabilise these; @josepot what do you reckon?

Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry to be the one disagreeing a bit on this, but my honest opinion is that:

I'm in favour of stabilizing transaction.

However, with regards of transactionWatch: I'm in favour of completely removing it from the spec. The reason being that as @tomaka has said many times:

one of the guiding principles of this new JSON-RPC API is to not do anything magic in the background

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 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 previously emitted event is no longer relevant? What would be the rigth way to communicate that?

IMO it's the responsibility of library authors to come up with their own systems/abstractions to communicate these things to the final consumer. The thing is that this watch does too much magic. The only way of using it effectively is IMO to communicate to the consumer exactly which blocks the server has scrutinized so far... but then, I would argue that this is pointless because the consumer may as well do that on their own... right? Therefore: wouldn't it be best to just remove it?

@jsdw
Copy link
Collaborator

jsdw commented Mar 6, 2024

@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).

@lexnv lexnv changed the title Stabilize transaction and transactionWatch to version 1 Stabilize transaction to version 1 Mar 6, 2024
@tomaka
Copy link
Contributor

tomaka commented Mar 6, 2024

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
Copy link
Contributor Author

lexnv commented Mar 6, 2024

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
Copy link
Contributor

tomaka commented Mar 6, 2024

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
Copy link
Collaborator

jsdw commented Mar 7, 2024

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)?

@lexnv lexnv merged commit edf5e54 into main Mar 13, 2024
@lexnv lexnv deleted the lexnv/stabilize-tx branch March 13, 2024 11:11
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Mar 19, 2024
This PR enables the `transaction_unstable_broadcast ` and
`transaction_unstable_stop` RPC API.

Since the API is unstable, we don't need to expose this in the release
notes.

After merging this, we could validate the API in subxt and stabilize it.

Spec PR that stabilizes the API:
paritytech/json-rpc-interface-spec#139

cc @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
)

This PR enables the `transaction_unstable_broadcast ` and
`transaction_unstable_stop` RPC API.

Since the API is unstable, we don't need to expose this in the release
notes.

After merging this, we could validate the API in subxt and stabilize it.

Spec PR that stabilizes the API:
paritytech/json-rpc-interface-spec#139

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.

5 participants