Enhancing Access to Extrinsic Operations through an API

[tareknaser]

Description

This pull request addresses limitations that hindered integration of the extrinsics into other projects. The existing commands were primarily designed for ink! contracts and lacked the necessary flexibility for customization in different project contexts.
To overcome these challenges, this pull request introduces changes that enhance the crate's usability, and integration capabilities.

Related issue #1195

Changes:

• Implementation of builders guarded by type states to enforce mandatory field requirements before creating objects.
• Implementation of Default for ExtrinsicOpts to simplify their usage throughout the codebase.

[xermicus]

To make this fully usable, we'd later also need to a way to get back the result (instead of just having it printed out). Since many of the run function already seem to return a result, currently the Ok variant is just a unit, this could be done in a follow up, right? @ascjones WDYT?

[ascjones]

As @xermicus alludes to, the issue is that the top level clap commands are tightly coupled to the invocation via a CLI (e.g. with user prompts). So I'm not sure whether it makes sense to make these commands the public API of this crate.

Another way to approach it might be to move the top level commands back to the cargo-contract crate, and make a finer grained API that does the transcoding and node interactions, and any CLI specific stuff (any stdin/stdout) IO be done

[tareknaser]

What I did?

• separated the printing logic into a single run method within the extrinsics structs.
• modified all other methods to exclude printing.

Why?

• By isolating printing within the run method, the extrinsics can be easily integrated into other projects not just CLIs.
• Using builders to initialize an extrinsic object, developers can call only the necessary methods without triggering any terminal printing, making it an adaptable API for interacting with the deployed contracts.
• The run method serves as an excellent template for CLI invocation in other projects.
• The changes are non-invasive to cargo-contract.
  • eg. all the code needed to upload a contract remains in upload.rs.

[xermicus]

That looks better now, but I'm still missing a few things.

Let's take the run method of the UploadCommand for example.

This method creates the code and signer that is then passed to upload_code. Why are code and signer not crafted in the UploadCommandBuilder::done() method? If the idea of the API is that users just use the builders, and the builders know already about the extrinsics opts, why do I need to use them again when using the command after building it. The builder consumes the ExtrinsicOpts, so if I am not using run, I need to create them again or clone them. It creates a weird API where it's confusing as to why I supply ExtrinsicOpts but then need to use them again, potentially confusing signer and code.

Also, I do think the printing logic should be factored out further to go back into cargo contract. If I have my own tool like this:

use std::path::PathBuf;

use contract_extrinsics::{ExtrinsicOpts, UploadCommand};

fn main() {
    let extr = ExtrinsicOpts::new()
        .suri("//Alice".into())
        .file(PathBuf::from("Foo.contract"))
        .done();
    let upld = UploadCommand::new().extrinsic_opts(extr).done();
    upld.run().unwrap();
}

This prints:
To submit the transaction and execute the call on chain, add -x/--execute flag to the command.

But I might not have such a flag. So this is specific to cargo-contract and should not be here.

Maybe we can even make the the UploadCommand generic over dry run. If this is feasable, the DryRun impl could just implement a dry_run and upload_code_rpc methods, and the impl for executing would have run and upload_code? run could return Result<CodeHashResult, ..>, where as dry_run would return Result<UploadDryRunResult, _>. Both can then be printed accordingly in cargo-contract or whoever uses them. It might be tricky because we still want to derive it with clap, maybe returning an Enum in the Ok case is easier.

Small nitpicks on the builder I noticed:

• Can for example set .file() multiple times, I don't know if we really want to allow this.
• .suri() would be nicer if it would take an impl ToString or something so that I don't have to do "//Alice".to_string().
• The same for .file(); if this would take Into<PathBuf> as argument instead, user could just do something like .file("Foo.contract")

[tareknaser]

Having done method of the builders craft the necessary artifacts for the extrinsics structs would introduce two problems:
1- These structs are built to be used from the command line and although we can skip them from the command line parsing or even have them as optional, having complex types in the structs would prevent cargo contract from implementing Debug all over the project.
2- done() is only invoked when all the required fields of the structs are specified and the object is ready to be created. This is the case when the struct is being built manually from the code but done() can't be called on objects created using user's command line arguments since clap handles the creation of the object. done() is implemented only for UploadCommandBuilder<state::ExtrinsicOptions>.

I propose having a method preprocess to handle getting the necessary information for other methods.
This prevents the user from providing wrong data to the methods of the structs while also keeping the code organized and clean.
I implemented this method for upload, call and remove.

• for upload:
  • preprocess returns the code and the signer.
  • I further abstracted upload_code_rpc to return UploadDryRunResult.
  • Now, run method contains only printing
• for call:
  • preprocess returns the transcoder, call data and the signer.
  • I couldn't modify call_dry_run to return CallDryRunResult since the value returned now is printed as it is in cargo contract
• for remove:
  • preprocess returns the transcoder, the signer and the final code hash.

[tareknaser]

For the instantiate command, we can't use the same approach because it's built in a very different way compared to other extrinsics.
The run method created a different object of type InstantiateExec which has methods implemented for it to handle various ways of instantiating the contract on chain.
I think the best way to approach this is to add a preprocess method in InstantiateCommand which returns an object of InstantiateExec that we can later call methods on.

This makes me think we can do the same for other commands for consistency and since this is a cleaner way of handling preprocessing.

Taking upload as an example:

• we can define another struct UploadExec. we can then add whatever information needed to upload a contract without being constrained by implementing Debug or by clap in general. It could look roughly like this

pub struct UploadExec {
    upload_params: UploadCommand,
    code: WasmCode,
    signer: PairSigner,
}

• a method in UploadCommand called preprocess would handle creating an object of UploadExec using the fields of UploadCommand whether they were specified by the user from the command line or created using the builder. We would have the opportunity to add any fields needed to UploadExec so we can do any preprocessing we want and store it in the struct itself so we won't have to call preprocess multiple times as I do in the current PR.
• In the implementation of UploadExec we can have public methods such as upload_code_rpc and upload_code to handle uploading a contract

This is another approach to the api to what I implement now in this PR. Which do you think makes a better API?

[xermicus]

we can define another struct UploadExec. we can then add whatever information needed to upload a contract

This sounds good to me. So the general idea is that the Command can be parsed from clap or use the builders. This can be then turned into a Exec struct that doesn't require Debug impls, can have struct fields that are not supposed to be parsed by clap etc.. This could even happen in a builder fashion (as if your "prepare" method would be a "done"), then users of the API don't even realize this right?

Additionally, regarding the printing. As we just discussed, it's probably better to move the run() methods that does mainly print things to the command line back into cargo contract. It would also be worthwhile to investigate whether these Result structs that have a .print() method could just implement Debug instead.

Also the Instantiate command is written a bit different than all others. We'll see what we can do here, ideally we can make a consistent API over all commands. Introducing these Exec structs should make it easier as well.

[tareknaser]

• The fields within the Exec structs are private. As there are no builders for these structs, they can now be initialized exclusively through the preprocess method of the commands. Access to the fields can be achieved via the getters.
• Certain structs, such as UploadDryRunResult and CallDryRunResult, are used solely for printing purposes. As a result, these structs were modified to have public fields to facilitate their role in printing.
• Printing operations have been centralized within the cargo-contract crate, which integrates the contract-extrinsics crate. This setup serves as an example for leveraging the API.
• Builder methods have been adjusted to accept Into<String> rather than just String. Similar changes were applied to file and other applicable fields in the builders.
• Using the extrinsics now from code is simple. crates/cargo-contract/src/main.rs is a good example but we can even skip some printing. Here is a file representing uploading code from a local contract

use contract_extrinsics::{ExtrinsicOpts, UploadCommand};

#[tokio::main]
async fn main() {
    let opts = ExtrinsicOpts::new()
        .file("flipper.contract")
        .suri("//Alice")
        .done();
    let upload = UploadCommand::new().extrinsic_opts(opts).done().await;

    let result = upload.upload_code_rpc().await.unwrap().unwrap();
    println!(
        "Code uploaded successfully.\nCode hash: {:?}",
        result.code_hash
    );
}

or

use contract_extrinsics::{ExtrinsicOpts, UploadCommand};

#[tokio::main]
async fn main() {
    let opts = ExtrinsicOpts::new()
        .file("flipper.contract")
        .suri("//Alice")
        .done();
    let upload = UploadCommand::new().extrinsic_opts(opts).done().await;

    let upload_result = upload.upload_code().await.unwrap();
    if let Some(code_stored) = upload_result.code_stored {
        println!(
            "Contract uploaded successfully.\nCode hash: {:?}",
            code_stored.code_hash
        );
    } else {
        println!("This contract has already been uploaded");
    }
}

[xermicus]

That looks much better now, thanks! Already makes re-using it much easier.

I gave a first round of review on the updated version. @ascjones please have a look too once you are back and have the time for it.

[tareknaser]

I've written two new tests for the updated API, which mirror existing tests focused on the entire lifecycle of cargo contract extrinsics.
I also encountered an issue while attempting to run integration tests due to the current configurations for conditional compilation in the integration_tests file. I've run the tests locally to ensure they pass successfully.

[ascjones]

There is still some CLI specific code in extrinsics. Is the end purpose of the libraray to be a pure "invoke contract extrinsics" crate? Or "invoke contract extrinsics" plus convenience CLI I/O methods for building your own CLI?

If we went full pure extrinsics invocation there could be even more separation, even moving the clap commands back to cargo-contract, which would involve conversion to extrinsics commands but would make clear the separation of concerns.

[xermicus]

Regarding "invoke contract extrinsics" plus convenience CLI I/O methods: I'd say if these convenience methods are useful for many or all use cases (i.e. gas estimation), they can go in. The clap commands for cargo-contracts as well as anything that does print to the command line do not fall into this category.

[ascjones]

Regarding "invoke contract extrinsics" plus convenience CLI I/O methods: I'd say if these convenience methods are useful for many or all use cases (i.e. gas estimation), they can go in. The clap commands for cargo-contracts as well as anything that does print to the command line do not fall into this category.

The tradeoff there is that now those helpers become part of the API to the crate, so people will start depending on them and it is less easy to do refactoring or change the way they behave without the possibility of breaking somebody's downstream code.

So possibly introducing a higher maintenance cost.

[xermicus]

@tareknaser if you move the CLI structs parsed with clap back into cargo contracts (you might want to store all required fields in the builder instead), and implement what preprocess does on the builder instead, I think not much cargo-contract specific code will be left. Also it should be easy to spot without the CLI structs.

[xermicus]

In the integration tests module, there is #[cfg(any(feature = "integration-tests", feature = "test-ci-only"))], so you need to add these features to the Cargo.toml of the extrinsics library.

[xermicus]

Looks much better now.

@ascjones I'll wait for your review before merging

[ascjones]

Okay, nearly there. I would like all those unwraps gone from the done methods then we can merge this 👍

[xermicus]

This function is duplicate code in call and instantiate

[tareknaser]

Although these functions do have similarities, they use different dry-run methods and object-specific information. Trying to combine them with a macro or common function might make the code more complex.

Do you have any suggestions or ideas for a simpler solution?

[xermicus]

Well, the matching on the result could go into a helper just like that. Everything could go into a helper if Exec was a trait instead. But this might require a bit more careful design. Also it's not the end of the world for me . Let's not bike shed the PR forever. The PR is already a good move into the right direction, we can still improve from here.

[xermicus]

@tareknaser CI failed

[ascjones]

LGTM!

Just missing some file headers on the new files.

[xermicus]

Thank you @tareknaser, good stuff!