There are quite a few places where such code duplication happens as part of match statement which would be nice to avoid. Maybe lets add fn from_receipt(receipt: &Receipt) -> Self as part VersionedActionReceipt which asserts the right receipt type internally. Then we can used binding as part of the match statement: receipt @ ReceiptEnum::PromiseYield(_) | ReceiptEnum::PromiseYieldV2(_) => { action_receipt = VersionedActionReceipt::from_receipt(receipt); .... That this is not ideal because we replace compile-time check with runtime assert on receipt type, but might be worth it to avoid code duplication.
I think a better idea would be to introduce transient VersionedReceiptEnum data structure similar to VersionedActionReceipt. It will have Action and PromiseYield variants containing VersionedActionReceipt and other variants are the same as in ReceiptEnum. Then we can eliminate code duplication while keeping type safety. The downside of that approach is that it is more involved to implement.

Then we can used binding as part of the match statement: receipt @ ReceiptEnum::PromiseYield() | ReceiptEnum::PromiseYieldV2() => { action_receipt = VersionedActionReceipt::from_receipt(receipt); .... That this is not ideal because we replace compile-time check with runtime assert on receipt type, but might be worth it to avoid code duplication.

Personally, I don't hate code duplication (of "simple" code) enough to see this as a benefit. :) But I can see your argument.

I think a better idea would be to introduce transient VersionedReceiptEnum data structure similar to VersionedActionReceipt. It will have Action and PromiseYield variants containing VersionedActionReceipt and other variants are the same as in ReceiptEnum. Then we can eliminate code duplication while keeping type safety. The downside of that approach is that it is more involved to implement.

This sounds good! Implementation shouldn't be too bad. I would be more worried about code readability with all the indirections. But I think it's worth it. Will give it a try.

I've implemented this in the latest commit
fb524f3

It allowed me to remove some annoying code duplication. E.g. how we process receipt in runtime/runtime/src/lib.rs

@@ -1149,7 +1146,7 @@ impl Runtime {
                     }
                 }
             }
-            ReceiptEnum::Action(action_receipt) => {
+            VersionedReceiptEnum::Action(action_receipt) => {
                 // Received a new action receipt. We'll first check how many input data items
                 // were already received before and saved in the state.
                 // And if we have all input data, then we can immediately execute the receipt.
@@ -1164,41 +1161,19 @@ impl Runtime {
                     pipeline_manager,
                     stats,
                     account_id,
-                    action_receipt.into(),
-                )?;
-
-                if executed.is_some() {
-                    return Ok(executed);
-                }
-            }
-            ReceiptEnum::ActionV2(action_receipt) => {
-                // Received a new action receipt. We'll first check how many input data items
-                // were already received before and saved in the state.
-                // And if we have all input data, then we can immediately execute the receipt.
-                // If not, then we will postpone this receipt for later.
-                let executed = self.process_action_receipt(
-                    receipt,
-                    receipt_sink,
-                    validator_proposals,
-                    state_update,
-                    apply_state,
-                    epoch_info_provider,
-                    pipeline_manager,
-                    stats,
-                    account_id,
-                    action_receipt.into(),
+                    action_receipt,
                 )?;

I couldn't make it work for mutable access, which still needs to explicitly match all versions. So code like this still has duplication.

// runtime/runtime/src/lib.rs
                // Modifying a new receipt instead of sending data
                match result
                    .new_receipts
                    .get_mut(receipt_index as usize)
                    .expect("the receipt for the given receipt index should exist")
                    .receipt_mut()
                {
                    ReceiptEnum::Action(new_action_receipt)
                    | ReceiptEnum::PromiseYield(new_action_receipt) => new_action_receipt
                        .output_data_receivers
                        .extend_from_slice(&action_receipt.output_data_receivers()),
                    ReceiptEnum::ActionV2(new_action_receipt)
                    | ReceiptEnum::PromiseYieldV2(new_action_receipt) => new_action_receipt
                        .output_data_receivers
                        .extend_from_slice(&action_receipt.output_data_receivers()),
                    _ => unreachable!("the receipt should be an action receipt"),
                }

Also, the runtime sync calls tests with assert_receipts! still needs to list all versions, too, as we need to extract actions from different versions.

// runtime/runtime/tests/test_async_calls.rs
    let receipts = &*assert_receipts!(group, "near_1" => r1 @ "near_2",
        ReceiptEnum::Action(ActionReceipt{actions, ..}) | ReceiptEnum::ActionV2(ActionReceiptV2{actions, ..}),
        {},
        actions,
        a0, Action::FunctionCall(function_call_action), {
        assert_eq!(function_call_action.gas, GAS_2);
        assert_eq!(function_call_action.deposit, 0);
    });

Overall, I think it's probably worth the extra boiler-plate and indirection, to avoid code duplication in a few key places. It probably also makes future changes easier. (Though it could also make it harder, I'm bad at predicting these things)

@pugachAG Is this what you had in mind?

That is exactly what I meant, thanks!

Regarding mutable access in my understanding the only way around it is introducing dedicated wrappers VersionedReceiptEnumMut and VersionedReceiptEnumMut which probably isn't worth it at this point.

nit: it seems like this comment belongs as part of the extracted method

nit: would be good to used versioned receipt here as well

Maybe I'm missing a trick but the Rust borrow checker doesn't allow borrowing VersionedReceiptEnum::Action(a) => a.actions() beyond the lifetime of a as things are written now. So I'd have to refactor the code to make it work.