Refactoring the bundle producer to get rid of the test-only variable

[NingLin-P]

In the bundle producer, there are a few test-only variables used to control the bundle producer logic in the test, which is quite invasive to the production code:

subspace/domains/client/domain-operator/src/domain_bundle_producer.rs

Lines 65 to 71 in eb24a91

	// TODO: both `skip_empty_bundle_production` and `skip_out_of_order_slot` are only used in the
	// tests, we should introduce a trait for `DomainBundleProducer` and use a wrapper of `DomainBundleProducer`
	// in the test, both `skip_empty_bundle_production` and `skip_out_of_order_slot` should move into the wrapper
	// to keep the production code clean.
	skip_empty_bundle_production: bool,
	skip_out_of_order_slot: bool,
	last_processed_slot: Option<Slot>,

To get rid of them, we can abstract the bundle producer to introduce a trait:

trait BundleProducer {
    fn produce_bundle(
        &mut self,
        operator_id: OperatorId,
        slot_info: OperatorSlotInfo,
    ) -> sp_blockchain::Result<Option<DomainProposal<Block, CBlock>>>;
}

For the production code, we can use the current implementation with the test-only logic removed:

subspace/domains/client/domain-operator/src/domain_bundle_producer.rs

Line 173 in eb24a91

pub async fn produce_bundle(

For the test, we can introduce a TestBundleProducer that is a wrapper of the production bundle producer and having the test-only logic in the wrapper:

struct TestBundleProducer {
    inner: BundleProducer,
    skip_empty_bundle_production: bool, 
    skip_out_of_order_slot: bool, 
    last_processed_slot: Option<Slot>,
}

impl BundleProducer for struct TestBundleProducer {
    fn produce_bundle(..) -> sp_blockchain::Result<Option<DomainProposal<Block, CBlock>>> {
         // Test logic 

         inner.produce_bundle(..)?;

         // Test logic 
    }
}

cc @teor2345

[teor2345]

Wouldn't it be simpler to eliminate those variables using #[cfg(test)]?
(Or whatever features or profile is used for tests vs production code.)

[NingLin-P]

The goal of this issue is to clean up the production code, using #[cfg(test)] doesn't help much, also you remind me of this issue #3162

[teor2345]

impl BundleProducer for struct TestBundleProducer {
    fn produce_bundle(..) -> sp_blockchain::Result<Option<DomainProposal<Block, CBlock>>> {
         // Test logic 

         inner.produce_bundle(..)?;

         // Test logic 
    }
}

It's not quite this simple, because the skip_empty_bundle_production check depends on the extrinsics (and consensus_chain_best_hash) which are only available (and consistent) inside DomainBundleProducer::produce_bundle().

So we'll either need a DomainBundleProducer::produce_bundle_inner() method which takes a closure to determine whether to skip empty bundle production, or a BundleProducerTesting trait with a skip_empty_bundle_production() method. Edit: or splitting the DomainBundleProducer::produce_bundle() method.

Splitting the method is likely to be more performant, and the code is slightly less complex that way, so I'll go with that alternative.

Since there's a big refactor involved, it seems easier to do this in two PRs.