Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Why does ClassInfo::new takes a reference to ContractClass as argument? #1724

Open
tdelabro opened this issue Mar 27, 2024 · 0 comments
Open

Comments

@tdelabro
Copy link
Contributor

tdelabro commented Mar 27, 2024

    pub fn new(
        contract_class: &ContractClass,
        sierra_program_length: usize,
        abi_length: usize,
    ) -> ContractClassResult<Self> {
        let (contract_class_version, condition) = match contract_class {
            ContractClass::V0(_) => (0, sierra_program_length == 0),
            ContractClass::V1(_) => (1, sierra_program_length > 0),
        };

        if condition {
            Ok(Self { contract_class: contract_class.clone(), sierra_program_length, abi_length })
        } else {
            Err(ContractClassError::ContractClassVersionSierraProgramLengthMismatch {
                contract_class_version,
                sierra_program_length,
            })
        }
    }

The method takes contract_class: &ContractClass and then clone it.
I think it should be contract_class: ContractClass because it would spare one clone. The ContractClass should be taking ownership of the ContractClass. The current implem adds an unnecessary clone if the lib a caller has a ContractClass for which he doesn't want to keep ownership after the call to ClassInfo::new.

The only reason I see to have the current implem is to spare a clone (in the case the lib caller needs to clone, which we can't know for sure) and that the conditions are not met, making ClassInfo::new return an error.

Even given this, I don't think we should arbitrage in this direction. It looks like a bad design to take a ref and later clone. If you end up needing the value ownership, take it from the get-go

gswirski pushed a commit to reilabs/blockifier that referenced this issue Jun 26, 2024
…#1744)

fix starkware-libs#1724

updated the test to make sure the gas prices doesn't get modified with the cli value in the [`update_block_env`](https://github.com/dojoengine/dojo/blob/ad58e43df7d162736fced6b071d59732183e77fb/crates/katana/core/src/backend/mod.rs#L182) method.

now actual tx execution would result in a similar fee value that you get when doing fee estimation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant