search

xJonathanLEI / starknet-rs

Complete Starknet library in Rust™
https://starknet.rs
Apache License 2.0
286 stars 101 forks source link

Unify `pub enum TransactionStatus` variants #607

Closed tcoratger closed 3 weeks ago

tcoratger commented 4 months ago

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.

xJonathanLEI commented 4 months ago

In an ideal world there should probably just be one. However:

  1. 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 match expression would have to deal with these extra variants when using JSON-RPC (as they're supposed to).
  2. The TransactionStatus type is the idiomatic wrapper for the finality_status and execution_status fields. It combines these 2 fields so that users who match can directly access the execution status without an unnecessary unwrap. This is different from SequencerTransactionStatus, which is pretty much just a C-style enum.
  3. On the other hand, SequencerTransactionStatus is also necessary as it's needed to support NoTraceAvailableErrorData, which only gives you the finality status, so it cannot be replaced by TransactionStatus.

I don't like it either, but I don't really see a way out of this mess.

xJonathanLEI commented 3 weeks ago

As explained there doesn't seem to be an apparent clean solution. Closing this for now.