Add gas report logic

[ddoktorski]

Towards #3660

[ddoktorski]

This change is part of the following stack:

• Store execution summary in trace data #3794
  • Keep resources of failed calls in trace #3795
    • Add gas component to debugging trace #3796
      • Move trace data code to separate file #3798
        • Change default tracked resource to sierra gas #3801
          • Rename fuzzing gas info #3823
            • Display fuzzing output without fractional parts #3840
              • Add gas report logic #3841 ◀
                • Create ContractsDataStore only once #3856
                  • Display gas report table #3842

_{Change managed by git-spice.}

[franciszekjob]

Wdyt about a little refactor here? Seems better to me:

    #[must_use]
    pub fn new_with_report(
        gas_used: GasVector,
        call_trace: &CallTrace,
        contracts_data: &ContractsDataStore,
    ) -> Self {
        let mut info = Self::new(gas_used);
        info.collect_gas_data(call_trace, contracts_data);
        info.finalize();
        info
    }

and we don't call finalize in collect_gas_data

fn collect_gas_data(&mut self, trace: &CallTrace, contracts_data: &ContractsDataStore) {
        let mut stack = trace.nested_calls.clone();

        while let Some(call_trace_node) = stack.pop() {
            if let CallTraceNode::EntryPointCall(call) = call_trace_node {
                let call = call.borrow();
                let class_hash = call.entry_point.class_hash.expect(
                    "class_hash should be set in `fn execute_call_entry_point` in cheatnet",
                );

                let contract_name = get_contract_name(contracts_data, class_hash);
                let selector = get_selector(contracts_data, call.entry_point.entry_point_selector);
                let gas = call
                    .gas_report_data
                    .as_ref()
                    .expect("Gas report data must be updated after test execution")
                    .get_gas();

                self.update_entry(contract_name, selector, gas);
                stack.extend(call.nested_calls.clone());
            }
        }
    }

[cptartur]

Bit weird to have a public constructor that doesn't have a gas report when collect_gas_data is private.

[cptartur]

Is there a point to this wrapper? It's used in a public field, but the inner BTreeMap<ContractName, ContractInfo>); is not public. So I understand it cannot be accessed by external modules anyway.

Why don't we just make the fields of GasSingleTestInfo not pub?

[cptartur]

I'd consider structuring this a bit different. Instead of having this struct technically accessible in non-final state, before finalized is called, I'd make it's internals private and changed the finalize method to return a new, different struct (some fields can probably be reused).

This way it is clear that this one is a "in progress" one and there's a separate struct representing a final result.

[ksew1]

You can even do this on the same struct using type state pattern

pub struct NoReport;
pub struct WithReport;

#[derive(Debug, Clone)]
pub struct GasSingleTestInfo<State> {
    pub gas_used: GasVector,
    pub report_data: ReportData,
    _state: PhantomData<State>,
}


pub fn finalize(self) -> GasSingleTestInfo<WithReport>

[ddoktorski]

I'd consider structuring this a bit different. Instead of having this struct technically accessible in non-final state, before finalized is called

What do you mean by technically accessible in a non-final state? It's possible to modify the fields since they are public, but there's a distinction between new and new_with_report, both return "finalized" versions

[cptartur]

I mean it's bit weird for me to share the same struct for gas data with and without raport, where report_data is typed as non-optional, and finalize method is private, so depending how the struct was constructed, there's no way to move it to a state with gas report non private.

I'd either represent these different states more explicitly, use separate structs or follow the pattern @ksew1 shared.

[ksew1]

I do rest of review tomorrow as I am getting sleepy 😴

[ksew1]

You can even do this on the same struct using type state pattern

pub struct NoReport;
pub struct WithReport;

#[derive(Debug, Clone)]
pub struct GasSingleTestInfo<State> {
    pub gas_used: GasVector,
    pub report_data: ReportData,
    _state: PhantomData<State>,
}


pub fn finalize(self) -> GasSingleTestInfo<WithReport>

[ksew1]

It is not a biggie right now, but we are beginning to leak implementation details, and this crate is becoming entangled with something unrelated—in this case, the gas report. Maybe ContractsData should contain better information so that we can use it here, as it is already used as the base in ContractsDataStore.

[ddoktorski]

Would it make sense to move ContractsDataStore out of the debugging crate?

[ksew1]

It is one of the options, but now we would have three structures: ContractsData, ForkData, and ContractsDataStore. ContractsDataStore is most complete so maybe we could delete other 2?

[cptartur]

Looks good, let's agree on and resolve the open comments