refactor(papyrus_protobuf): add converters for new transaction messages

[AlonLStarkWare]

No description provided.

[reviewable-StarkWare]

This change is 

[ShahakShama]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @AlonLStarkWare, @eitanm-starkware, and @noamsp-starkware)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 137 at r1 (raw file):

}

// TODO(alonl): remove once Transaction is replaced with TransacitonInBlock

I prefer you do this TODO in this PR, as your PR is partial without it
IIUC it should only affect this file, right?

---

crates/papyrus_protobuf/src/converters/transaction.rs line 195 at r1 (raw file):

}

impl TryFrom<protobuf::TransactionInBlock> for (Transaction, TransactionHash) {

Comment when is this used and when is the conversion without the hash

---

crates/papyrus_protobuf/src/converters/transaction.rs line 209 at r1 (raw file):

            .map(TransactionHash)?;

        let txn = match txn {

Remove code duplication with the hashless conversion. Same in other places

---

crates/papyrus_protobuf/src/converters/transaction.rs line 252 at r1 (raw file):

}

// TODO(alonl): remove once Transaction is replaced with TransacitonInBlock

Same here

---

crates/papyrus_protobuf/src/converters/transaction.rs line 350 at r1 (raw file):

}

// TODO(alonl): remove once Transaction is replaced with TransacitonInBlock

Same

[AlonLStarkWare]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @eitanm-starkware, @noamsp-starkware, and @ShahakShama)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 137 at r1 (raw file):

Previously, ShahakShama wrote…

I prefer you do this TODO in this PR, as your PR is partial without it
IIUC it should only affect this file, right?

The protobuf::Transaction <-> Transaction (without hash) converters are used in consensus converters in this PR.
In a future PR when TransactionBatch will use ConsensusTransaction instead of Transaction we can remove these, I'll fix the TODO.

The protobuf::Transaction <-> (Transaction, TransactionHash) converters are used when deserializing FullTransaction <-> protobuf::TransactionWithReceipt which is already using TransactionInBlock so I removed them.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 195 at r1 (raw file):

Previously, ShahakShama wrote…

Comment when is this used and when is the conversion without the hash

Done.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 209 at r1 (raw file):

Previously, ShahakShama wrote…

Remove code duplication with the hashless conversion. Same in other places

Done.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 252 at r1 (raw file):

Previously, ShahakShama wrote…

Same here

Done.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 350 at r1 (raw file):

Previously, ShahakShama wrote…

Same

Done.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 548 at r1 (raw file):

                protobuf::TransactionInBlock {
                    txn: Some(protobuf::transaction_in_block::Txn::DeclareV0(declare_v0.into())),
                    transaction_hash: None,

Does this make sense? Shouldn't we calculate the hash?

[ShahakShama]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @AlonLStarkWare, @eitanm-starkware, and @noamsp-starkware)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 195 at r1 (raw file):

Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…

Done.

What I meant was to comment where is this used. The user can tell what is being converted.
Something like: // Used in sync
And in the other place write: // Used in consensus or Used in mempool, depending where you use it

---

crates/papyrus_protobuf/src/converters/transaction.rs line 252 at r1 (raw file):

Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…

Done.

Same

---

crates/papyrus_protobuf/src/converters/transaction.rs line 350 at r1 (raw file):

Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…

Done.

Same

---

crates/papyrus_protobuf/src/converters/transaction.rs line 548 at r1 (raw file):

Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…

Does this make sense? Shouldn't we calculate the hash?

I think you can remove this conversion and change it to From<(Transaction, TransactionHash)> instead (unless that conversion already exists)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 141 at r2 (raw file):

    type Error = ProtobufConversionError;
    fn try_from(value: protobuf::TransactionInBlock) -> Result<Self, Self::Error> {
        let transaction: Transaction = value.clone().try_into()?;

No need to clone. Just put this line below the tx_hash line instead and clone value.transaction_hash if you need (which is much smaller)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 389 at r2 (raw file):

}

// TODO(alonl): remove once Transaction is replaced with TransacitonInBlock

Same here. Do this TODO now

---

crates/papyrus_protobuf/src/converters/transaction.rs line 530 at r2 (raw file):

}

// TODO(alonl): remove once Transaction is replaced with TransacitonInBlock

Same here. Do this TODO now

[AlonLStarkWare]

Reviewable status: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @eitanm-starkware, @noamsp-starkware, and @ShahakShama)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 195 at r1 (raw file):

Previously, ShahakShama wrote…

What I meant was to comment where is this used. The user can tell what is being converted.
Something like: // Used in sync
And in the other place write: // Used in consensus or Used in mempool, depending where you use it

Removed the conversions without hash, so I assume this isn't relevant anymore

---

crates/papyrus_protobuf/src/converters/transaction.rs line 252 at r1 (raw file):

Previously, ShahakShama wrote…

Same

Same

---

crates/papyrus_protobuf/src/converters/transaction.rs line 350 at r1 (raw file):

Previously, ShahakShama wrote…

Same

Which same?

---

crates/papyrus_protobuf/src/converters/transaction.rs line 548 at r1 (raw file):

Previously, ShahakShama wrote…

I think you can remove this conversion and change it to From<(Transaction, TransactionHash)> instead (unless that conversion already exists)

Removed and restored the implementation for From<(Transaction, TransactionHash)>

---

crates/papyrus_protobuf/src/converters/transaction.rs line 141 at r2 (raw file):

Previously, ShahakShama wrote…

No need to clone. Just put this line below the tx_hash line instead and clone value.transaction_hash if you need (which is much smaller)

Done.

---

crates/papyrus_protobuf/src/converters/transaction.rs line 389 at r2 (raw file):

Previously, ShahakShama wrote…

Same here. Do this TODO now

Cannot be done yet because of the conversion in consensus (these conversions are used in Transaction <-> protobuf::Transaction)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 530 at r2 (raw file):

Previously, ShahakShama wrote…

Same here. Do this TODO now

Same here

[ShahakShama]

Reviewed 1 of 4 files at r4, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlonLStarkWare, @eitanm-starkware, and @noamsp-starkware)

---

crates/papyrus_protobuf/src/converters/transaction.rs line 996 at r7 (raw file):

}

/// Defined to match the protobuf schema.

Move this to the head of the file