RPC end point with more fine grained tx status #9542
Comments
|
Great idea to clean this up! Disclaimer, my knowledge here is not perfect on how these endpoints work. But I'll try to explain how I think it works today. I believe both endpoints look up recursively all execution statuses of the receipts DAG, then they aggregate the status of all receipts into a single The However, putting finality aside, for some reason I used to think So much about what the status quo is. I think the ideal API would be exactly as you describe in points 1,2,3. But for the aggregated status, I would probably keep it as is but at least make it much clearer in the documentation what it means. |
|
I like the idea of reusing the same wait enum for both broadcast and status endpoints. I have no clue how do clients actually use the endpoint so it's hard to say for me what is the best design. Logically speaking there are three separate functionalities that we're considering: broadcast, status and wait. Today 'wait' seems to be integrated into the other two in one way or another. Depending on the client usage it may make sense to add the WaitCondition as either an argument, return value or both for both endpoints. I think what you're suggesting is one viable option. At the end of the day what matters is that the API is usable and well documented so any improvement is welcome. |
…atus`, `broadcast_tx_commit`, `broadcast_tx_async` (#9556) This PR is a part of RPC tx methods redesign https://docs.google.com/document/d/1jGuXzBlMAGQENsUpy5x5Xd7huCLOd8k6tyMOEE41Rro/edit?usp=sharing Addressing point 3 in #9542 and start working on #6837 Changes: - New field `status` appeared in the response of RPC methods `tx`, `EXPERIMENTAL_tx_status`, `broadcast_tx_commit`, `broadcast_tx_async` - Added some comments with the explanations In #6837, Illia suggested to have the structure ```rust enum BroadcastWait { /// Returns right away. None, /// Waits only for inclusion into the block. Inclusion, /// Waits until block with inclusion is finalized. InclusionFinal, /// Waits until all non-refund receipts are executed. Executed, /// Waits until all non-refund receipts are executed and the last of the blocks is final. ExecutedFinal, /// Waits until everything has executed and is final. Final, } ``` I've renamed it to `TxExecutionStatus` and simplified it a little by dropping all refund options: 1. We may add them later without breaking backwards compatibility 2. To support them, we need to have the access to `receipt` (it's the method `get_tx_execution_status`). We have there only `execution_outcome` by default. We created special logic to be able not to retrieve the receipts (it's the only difference between `tx` and `EXPERIMENTAL_tx_status`). I have a feeling (please correct me here if I'm wrong) that retrieving corresponding receipt may slow down the execution that we tried to optimise. BTW, maybe the data from `execution_outcome` is enough (we have there `gas_burnt` and `tokens_burnt`). @jakmeier suggested the condition `outcome.tokens_burnt == 0 && outcome.status == Success`. Unfortunately, we need to work with any status. But maybe the first part (`outcome.tokens_burnt == 0`) is enough to say that it could be only refund receipt. I need more input here.
…atus`, `broadcast_tx_commit`, `broadcast_tx_async` (#9556) This PR is a part of RPC tx methods redesign https://docs.google.com/document/d/1jGuXzBlMAGQENsUpy5x5Xd7huCLOd8k6tyMOEE41Rro/edit?usp=sharing Addressing point 3 in #9542 and start working on #6837 Changes: - New field `status` appeared in the response of RPC methods `tx`, `EXPERIMENTAL_tx_status`, `broadcast_tx_commit`, `broadcast_tx_async` - Added some comments with the explanations In #6837, Illia suggested to have the structure ```rust enum BroadcastWait { /// Returns right away. None, /// Waits only for inclusion into the block. Inclusion, /// Waits until block with inclusion is finalized. InclusionFinal, /// Waits until all non-refund receipts are executed. Executed, /// Waits until all non-refund receipts are executed and the last of the blocks is final. ExecutedFinal, /// Waits until everything has executed and is final. Final, } ``` I've renamed it to `TxExecutionStatus` and simplified it a little by dropping all refund options: 1. We may add them later without breaking backwards compatibility 2. To support them, we need to have the access to `receipt` (it's the method `get_tx_execution_status`). We have there only `execution_outcome` by default. We created special logic to be able not to retrieve the receipts (it's the only difference between `tx` and `EXPERIMENTAL_tx_status`). I have a feeling (please correct me here if I'm wrong) that retrieving corresponding receipt may slow down the execution that we tried to optimise. BTW, maybe the data from `execution_outcome` is enough (we have there `gas_burnt` and `tokens_burnt`). @jakmeier suggested the condition `outcome.tokens_burnt == 0 && outcome.status == Success`. Unfortunately, we need to work with any status. But maybe the first part (`outcome.tokens_burnt == 0`) is enough to say that it could be only refund receipt. I need more input here.
|
Done 😎 |
I want to rework
txandEXPERIMENTAL_tx_statusmethods:include_receiptsflag totxmethod (defaultfalse, it saves backward compatibility).truewill return the result fromEXPERIMENTAL_tx_statusEXPERIMENTAL_tx_statusand hopefully we may drop it in one day. It's not experimental from it's very first dayI suggest to add to response the new enum value, as Illia suggested in RPC end point to configure broadcast transaction await #6837, it will answer what is the status of the execution.
The text was updated successfully, but these errors were encountered: