feat(starknet_consensus_orchestrator): use InternalConsensusTransacti…

[ShahakShama]

…on in consensus

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-30T05:24:25Z
• Started at 2025-01-30T09:12:55Z
• Started at 2025-01-30T11:47:39Z
• Started at 2025-01-30T12:55:37Z

[matan-starkware]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 156 at r2 (raw file):

    pub fn new(
        config: ContextConfig,
        class_manager_client: SharedClassManagerClient,

Hmm I had thought the plan was to pass in TransactionConverter and make the context generic over that. Instead we are "generic" (Box) over the class manager.

Code quote:

        class_manager_client: SharedClassManagerClient,

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 408 at r2 (raw file):

                bouncer_weights: central_objects.bouncer_weights,
            })
            .await;

Suggestion:

let                 transactions= futures::future::join_all(transactions.into_iter().map(|tx| {
                    self.transaction_converter.convert_internal_consensus_tx_to_executable_tx(tx)
                }))
                .await
                .into_iter().collect::<Result<Vec<_>, _>>()
                // TODO(shahak): Do not panic here.
                .expect("Failed converting transactions for cende");
                
        self.cende_ambassador
            .prepare_blob_for_next_height(BlobParameters {
                // TODO(dvir): use the real `BlockInfo` when consensus will save it.
                block_info: BlockInfo { block_number: BlockNumber(height), ..Default::default() },
                state_diff,
                compressed_state_diff: central_objects.compressed_state_diff,
                transactions,
                execution_infos: central_objects.execution_infos,
                bouncer_weights: central_objects.bouncer_weights,
            })
            .await;

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 811 at r2 (raw file):

            // TODO(shahak): Don't panic here.
            .expect("Failed converting consensus transaction to internal representation");
            debug!("Converted transactions to internal representation.");

This pattern keeps repeating, wanna try creating a helper function?

Code quote:

            let txs = futures::future::join_all(txs.into_iter().map(|tx| {
                transaction_converter.convert_consensus_tx_to_internal_consensus_tx(tx)
            }))
            .await
            .into_iter()
            .collect::<Result<Vec<_>, _>>()
            // TODO(shahak): Don't panic here.
            .expect("Failed converting consensus transaction to internal representation");
            debug!("Converted transactions to internal representation.");

[ShahakShama]

Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @matan-starkware)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 156 at r2 (raw file):

Previously, matan-starkware wrote…

Hmm I had thought the plan was to pass in TransactionConverter and make the context generic over that. Instead we are "generic" (Box) over the class manager.

Yes. There's a TODO in the test to do that. Added also here

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 811 at r2 (raw file):

Previously, matan-starkware wrote…

This pattern keeps repeating, wanna try creating a helper function?

I think the helper function will be either unreadable (in terms of what it does) or the call to the helper function will be longer than these lines

[matan-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 811 at r2 (raw file):

Previously, ShahakShama wrote…

I think the helper function will be either unreadable (in terms of what it does) or the call to the helper function will be longer than these lines

It wasn't so bad:

Callsite:

        let transactions = convert_all(
            |tx| self.transaction_converter.convert_internal_consensus_tx_to_executable_tx(tx),
            transactions,
        )
        .await;

Definition - generic so can handle any converter:

async fn convert_all<FromT, IntoT, ConverterT, FutT>(
    converter: ConverterT,
    txs: Vec<FromT>,
) -> Vec<IntoT>
where
    ConverterT: Fn(FromT) -> FutT,
    FutT: Future<Output = TransactionConverterResult<IntoT>>,
{
    futures::future::join_all(txs.into_iter().map(|tx| { converter(tx) }))
    .await
    .into_iter().collect::<Result<Vec<_>, _>>()
    // TODO(shahak): Do not panic here.
    .expect("Failed converting transactions for cende")
}

Not blocking just consider it.