starknet-rs
starknet-rs copied to clipboard
Unify `pub enum TransactionStatus` variants
At the moment, we have multiple TransactionStatus enum variants around the codebase:
https://github.com/xJonathanLEI/starknet-rs/blob/4dbc1692d4f295de803484181e07803201528922/starknet-core/src/types/mod.rs#L178-L183
https://github.com/xJonathanLEI/starknet-rs/blob/4dbc1692d4f295de803484181e07803201528922/starknet-core/src/types/codegen.rs#L1536-L1545
https://github.com/xJonathanLEI/starknet-rs/blob/4dbc1692d4f295de803484181e07803201528922/starknet-providers/src/sequencer/models/transaction_receipt.rs#L35-L51
I think this should be unified, probably in core to have a single variant for this purpose.
In an ideal world there should probably just be one. However:
- The sequencer-specific type exists because the sequencer apparently indeed returns more variants than what's seen on JSON-RPC. If we unify everything into a single type we will have to include those sequencer-specific variants, which means someone trying doing a
matchexpression would have to deal with these extra variants when using JSON-RPC (as they're supposed to). - The
TransactionStatustype is the idiomatic wrapper for thefinality_statusandexecution_statusfields. It combines these 2 fields so that users whomatchcan directly access the execution status without an unnecessary unwrap. This is different fromSequencerTransactionStatus, which is pretty much just a C-style enum. - On the other hand,
SequencerTransactionStatusis also necessary as it's needed to supportNoTraceAvailableErrorData, which only gives you the finality status, so it cannot be replaced byTransactionStatus.
I don't like it either, but I don't really see a way out of this mess.
As explained there doesn't seem to be an apparent clean solution. Closing this for now.