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

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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions