refactor!: separate Contract loading and deploying

[hal3e]

closes: #892

Updated PR description

After taking in consideration all comments made in this PR, I have decided to do a bit more refactoring on Contract. I have tried to move functions not related directly to Contract to places where they make more sense. I have:

• Moved contract_deployment_transaction to CreateTransactionBuilder::prepare_contract_deployment
• Moved get_storage_slots and its logic to StorageConfiguration - No need for Contract to handle this
• Moved validate_path_and_extension outside as it is now also used by StorageConfiguration
• Moved method_hash and should_compute_custom_input_offset - No need for Contract to handle this
• Deleted deploy and renamed deploy_loaded to deploy - big breaking change will elaborate below
  Other refactors:
• Made DeployConfiguration into LoadConfiguration and took out TxParametres
  • The idea is to separate loading and deploying of the contracts. Now the user will add the TxParameters when he actually deploy the contract

The biggest change for the user is the way to deploy contracts. Previously the user would use deploy directly:

     let contract_id = Contract::deploy(
        "tests/contracts/configurables/out/debug/configurables.bin",
        &wallet,
        DeployConfiguration::default(),
    )
    .await?;

This function basically did two things: Load and then deploy the contract. I propose we use this instead:

    let contract_id = Contract::load_from(
        "tests/contracts/configurables/out/debug/configurables.bin",
        LoadConfiguration::default(),
    )?
    .deploy(&wallet, TxParameters::default())
    .await?;

This makes it clear what is being done. Makes the LoadConfiguration simpler and also now the user can get the contract_id or state_root easily.

    let contract_id = Contract::load_from(
        "tests/contracts/configurables/out/debug/configurables.bin",
        LoadConfiguration::default(),
    )?
    .contract_id();

For reference this is what the user had to do previously to get the contract_id

    let compiled_contract = Contract::load_contract(
        contract_binary_path,
        DeployConfiguration::default(),
    )?;
    let (test_contract_id, _) = Contract::compute_contract_id_and_state_root(&compiled_contract);

thanks @segfault-magnet for your suggestions and helping me move code around 😄

Checklist

• I have linked to any relevant issues.
• I have updated the documentation.
• I have added tests that prove my fix is effective or that my feature works.
• I have added necessary labels.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[segfault-magnet]

Left a suggestion.

Love it.

[iqdecay]

I think the approach of making CompiledContract a good type would improve UX vastly as you did, without loosing semantic richness

[hal3e]

I think the approach of making CompiledContract a good type would improve UX vastly as you did, without loosing semantic richness

I did consider that approach. However, Contract would not have to hold CompiledContract as it is never used. We would just pass CompiledContract around. (This is currently the case on master). My reasoning was: Why have CompliedContractas a separate struct when our Contract can hold the info and we can turn some of the associated functions to methods.

[iqdecay]

Why have CompliedContractas a separate struct when our Contract can hold the info and we can turn some of the associated functions to methods.

Because there can be multiple Contracts associated to a single CompiledContract. A compiled contract is essentialy always the same, whereas there can be multiple different Contract instances that share the same CompiledContract base. I think this composition is important to understand the difference between what gets deployed (ie at an instant t) and what's the contract itself (ie bytecode and storage slots). Also, even if it wasn't the case, I still think a private struct to convey this meaning of "a contract is built on top of the compiled contract abstraction" is interesting.

After doing some testing, I realize you are right, it does feel like we're just passing things around. I withdraw my remark, but I feel like my confusion arises from the fact that we are not distinguishing between a Contract deployed at block h and the same Contract deployed at block h+10. They are different objects but have the same base (bytecode + storage slot and salt). I feel like with our current abstraction we don't differentiate between those two objects.

[iqdecay]

🙏 🙏 please keep this PR down to a single change. A PR should only fix one thing or introduce one feature, not do two things at the same time.

[MujkicA]

After doing some testing, I realize you are right, it does feel like we're just passing things around. I withdraw my remark, but I feel like my confusion arises from the fact that we are not distinguishing between a Contract deployed at block h and the same Contract deployed at block h+10. They are different objects but have the same base (bytecode + storage slot and salt). I feel like with our current abstraction we don't differentiate between those two objects.

I'm not sure I follow here. From my perspective, they are the same object (albeit not instance), a deployed contract. The difference is only the salt which leads to a different contract_id being computed.

we agreed to change how the contract is loaded and deployed

[digorithm]

This is an amazing UX improvement; the previous way of loading + deploying was getting way too convoluted. Super clean implementation, too; great stuff, @hal3e.

Approving a minor grammar-related change request, feel free to merge after!

[digorithm]

awesome work!