Skip to content
This repository was archived by the owner on Aug 21, 2024. It is now read-only.
This repository was 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

@tdelabro

Description

@tdelabro
    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions