GraphQL API changes to support pre-confirmations #2752
Conversation
…ionStatusStorage`
crates/client/assets/schema.sdl
Outdated
| type FailureDuringBlockProductionStatus { | ||
| blockHeight: U32! | ||
| } | ||
|
|
There was a problem hiding this comment.
What can lead to a transaction failing with a FailureDuringBlockProductionStatus?
I’ve noticed this status doesn’t include a reason property like FailureStatus does.
There was a problem hiding this comment.
What can lead to a transaction failing with a FailureDuringBlockProductionStatus?
There could be a change in state (for example, different gas price) between these two points in time:
- executor grabs the transaction from the TxPool (where it sits, validated and ready to be executed)
- executor actually tries to execute the transaction
I’ve noticed this status doesn’t include a reason property like FailureStatus does.
The exact shape of the API is not finalized yet. We decided to add the reason to FailureDuringBlockProductionStatus as well. There will be more commits added to this PR.
There was a problem hiding this comment.
reason is now added to the FailureDuringBlockProductionStatus variant
rafal-ch
changed the title
rafal-ch
requested review from
xgreenx,
Dentosal,
MitchTurner,
netrome,
acerone85,
rymnc and
AurelienFT
as code owners
| @@ -187,8 +190,11 @@ impl TryFrom<ProgramState> for fuel_vm::ProgramState { | |||
| pub enum TransactionStatus { | |||
| SubmittedStatus(SubmittedStatus), | |||
| SuccessStatus(SuccessStatus), | |||
| SuccessDuringBlockProductionStatus(SuccessDuringBlockProductionStatus), | |||
There was a problem hiding this comment.
Should we use a name which relates more closely to the pre-confirmations? For example SuccessPreConfirmed, FailurePreConfirmed, etc.
cc: @xgreenx
There was a problem hiding this comment.
Yes, maybe this name is better, buy you can ask in Slack channel what SDKs prefer more=D
There was a problem hiding this comment.
Renamed in a473dbb, as agreed with SDK.
The only minor drawback I see is that in case in the future other components than BlockProducer are able to create preconfirmations we'll not carry the information about the preconfirmation source. Most likely a YAGNI case, that's why I didn't add any kind of "source" field to preconfirmations.
| @@ -187,8 +190,11 @@ impl TryFrom<ProgramState> for fuel_vm::ProgramState { | |||
| pub enum TransactionStatus { | |||
| SubmittedStatus(SubmittedStatus), | |||
| SuccessStatus(SuccessStatus), | |||
| SuccessDuringBlockProductionStatus(SuccessDuringBlockProductionStatus), | |||
There was a problem hiding this comment.
Yes, maybe this name is better, buy you can ask in Slack channel what SDKs prefer more=D
| @@ -67,7 +67,7 @@ pub struct TransactionStatuses; | |||
| impl Mappable for TransactionStatuses { | |||
| type Key = Bytes32; | |||
| type OwnedKey = Self::Key; | |||
| type Value = TransactionStatus; | |||
| type Value = TransactionStatusStorage; // TODO[RC]: TransactionStatusStorage should ultimately only contain `Success` and `Failed` variants. We don't need to persist any other states. | |||
There was a problem hiding this comment.
Do you want to solve it in this PR, or in follow-up?
| /// The transaction failed to execute and was included in a block. | ||
| Failed, | ||
| /// Transaction was eligible for execution and inclusion in the block but | ||
| /// it failed during the execution. | ||
| FailureDuringBlockProduction, |
There was a problem hiding this comment.
IT would be nice to have parity on Failed and Failure
crates/types/src/services/txpool.rs
Outdated
| @@ -333,7 +334,7 @@ impl From<&PoolTransaction> for CheckedTransaction { | |||
| /// The status of the transaction during its life from the tx pool until the block. | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
| #[derive(Clone, Debug, PartialEq, Eq)] | |||
| pub enum TransactionStatus { | |||
| pub enum TransactionStatusStorage { | |||
There was a problem hiding this comment.
It would be nice to use some other name, since we also use this type as a result of execution, maybe TransactionExecutionStatus , idk
There was a problem hiding this comment.
Updated in 4667fc3
TransactionExecutionStatus already exists in executor: https://github.com/FuelLabs/fuel-core/blob/master/crates/types/src/services/executor.rs#L185-L190
We also seem to already have what we need (albeit there are differences in fields) in the form of TransactionExecutionResult: https://github.com/FuelLabs/fuel-core/blob/master/crates/types/src/services/executor.rs#L195-L218
For now I used the explicit qualification (txpool::TransactionExecutionStatus) to resolve conflicts to avoid introducing yet another name for transaction status.
Follow-up issue for grooming: #2795
| async fn transaction( | ||
| &self, | ||
| ctx: &Context<'_>, | ||
| ) -> Option<async_graphql::Result<Transaction>> { |
There was a problem hiding this comment.
| ) -> Option<async_graphql::Result<Transaction>> { | |
| ) -> async_graphql::Result<Option<Transaction>> { |
| ) -> Option<async_graphql::Result<Transaction>> { | ||
| self.tx_id.map(|tx_id| { | ||
| let query = ctx.read_view()?; | ||
| let transaction = query.transaction(&tx_id)?; |
There was a problem hiding this comment.
Usually pre-confirmed transaction will live in TxPool, not in the database, so I would suggest to go there=D
| }) | ||
| } | ||
|
|
||
| async fn receipts(&self) -> Option<async_graphql::Result<Vec<Receipt>>> { |
There was a problem hiding this comment.
Why do we need to return an error? Looks like we can't have error here
There was a problem hiding this comment.
I did it like that for consistency with already existing types, like SuccessStatus here.
| } | ||
|
|
||
| #[Object] | ||
| impl SuccessDuringBlockProductionStatus { |
There was a problem hiding this comment.
Same comments as for failure
| #[derive(Debug)] | ||
| pub struct SuccessDuringBlockProductionStatus { | ||
| pub tx_pointer: TxPointer, | ||
| pub tx_id: Option<TxId>, |
There was a problem hiding this comment.
tx_id always will exist in pre-confirmation
Closes #2771
Description
This PR contains the changes to the GraphQL API supporting the upcoming pre-confirmations feature.
Summary:
TransactionStatushas grown three new variants:SuccessDuringBlockProductionStatus,SqueezedOutDuringBlockProductionStatusandFailureDuringBlockProductionStatusTransactionStatusStoragetype, which is just an oldTransactionStatusunder the new name. Ultimately, we need to convert it to a new type which only containsSuccessandFailurevariants, since only these two states are only ever persisted in the DBBefore requesting review
After merging, notify other teams