fix: parse user inputs for Option arguments

[AlexD10S]

This PR improves the DevEx for pop call contract when calling contracts with optional parameters (e.g. Option<String>). Previously, users were required to wrap optional parameter values in Some(...) themselves. Now, users can simply enter the raw value, and pop-cli will automatically handle the wrapping, adding Some(...) or None if empty.

This logic (generate_message_args) has been integrated into pop_contracts crate when preparing the preprocessed data for a contract call (set_up_call). With the check there, both CLI users and pop_contracts crate consumers no longer need to manually wrap optional values in Some(...) or specify None for missing values.

Update

Update the logic for the missing feature for pop up contract when calling contracts with optional parameters.
Fixes: #319

The main change is to move the logic for parsing metadata and processing arguments into a filed used by call and up. and includes the parsing metadata and processing arguments for constructors.

Testing

Most of the code changes has been to adjust testing. To adjust that the contract used for testing has been updated. Is basically the flipper contract with a third call to test parsing arguments:

      /// Constructor that initializes the `bool` value to the given `init_value`.
        #[ink(constructor)]
        pub fn new(init_value: bool, number: Option<u32>) -> Self {
            Self {
                value: init_value,
                number: number.unwrap_or(0),
            }
        }

        /// Constructor that initializes the `bool` value to `false`.
        ///
        /// Constructors can delegate to other constructors.
        #[ink(constructor)]
        pub fn default() -> Self {
            Self::new(Default::default(), None)
        }
        /// A message for testing, flips the value of the stored `bool` with `new_value`
        /// and is payable
        #[ink(message)]
        #[ink(payable)]
        pub fn specific_flip(&mut self, new_value: bool, number: Option<u32>) {
            self.value = new_value;
            if let Some(number) = number {
                self.number = number;
            }
        }

[codecov]

Codecov Report

Attention: Patch coverage is 80.56156% with 180 lines in your changes missing coverage. Please review.

Project coverage is 71.78%. Comparing base (582db18) to head (3670eb5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/pop-cli/src/commands/call/contract.rs	83.06%	31 Missing and 65 partials ⚠️
crates/pop-cli/src/cli.rs	56.58%	50 Missing and 6 partials ⚠️
crates/pop-contracts/src/call/metadata.rs	91.24%	1 Missing and 11 partials ⚠️
crates/pop-contracts/src/init_tests.rs	70.00%	0 Missing and 6 partials ⚠️
crates/pop-contracts/src/up.rs	83.33%	0 Missing and 6 partials ⚠️
crates/pop-contracts/src/call/mod.rs	89.18%	0 Missing and 4 partials ⚠️

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   70.32%   71.78%   +1.46%     
==========================================
  Files          53       55       +2     
  Lines        9098     9910     +812     
  Branches     9098     9910     +812     
==========================================
+ Hits         6398     7114     +716     
- Misses       1718     1735      +17     
- Partials      982     1061      +79     

Files with missing lines	Coverage Δ
crates/pop-contracts/src/errors.rs	100.00% <ø> (+100.00%)	⬆️
crates/pop-contracts/src/call/mod.rs	80.45% <89.18%> (ø)
crates/pop-contracts/src/init_tests.rs	70.00% <70.00%> (ø)
crates/pop-contracts/src/up.rs	80.41% <83.33%> (+2.76%)	⬆️
crates/pop-contracts/src/call/metadata.rs	91.24% <91.24%> (ø)
crates/pop-cli/src/cli.rs	67.36% <56.58%> (-4.89%)	⬇️
crates/pop-cli/src/commands/call/contract.rs	81.44% <83.06%> (+81.44%)	⬆️

... and 2 files with indirect coverage changes

[evilrobot-01]

Nicely done! Mostly minor naming and parameter type improvements.

Also - the linked issue is about constructor args, but there is nothing specific about that in this PR, only generalised message calling. Has it been tested with constructor args? If not, then this PR doesnt address that issue and it should be unlinked as appropriate.

[al3mart]

I believe the constructor case is still not covered. Tested locally modifying a contract to accept Option<bool> as a constructor argument.

But when executing pop up contract --constructor new --args false the following error is returned

 An error occurred instantiating the contract: Invalid enum variant value 'Bool(false)'

Which seems to point to the argument not being wrapped in the Option type as it is done for the contract calls.

[AlexD10S]

You're right; the logic for constructors was indeed missing.
Updated the logic to include the logic in pop up contract when calling contracts with optional parameters.
Fixes: #319

The main change is to move the logic for parsing metadata and processing arguments into a filed used by call and up. and includes the parsing metadata and processing arguments for constructors.

[al3mart]

Option types are covered for constructors now, awesome!

Doing some local testing I detected the following:

The contract constructor I have been using for this test:

#[ink(constructor)]
pub fn new(init_value: Option<bool>, other_value: Option<bool>) -> Self {
    Self { value: init_value.unwrap_or(false) }
}

If I execute:

pop up contract --args false,false

The behavior is the expected, the args are wrapped an it works nicely.

Although, it seems we are counting space chars as arguments.

pop up contract --constructor new --args false, false 
// outputs
An error occurred instantiating the contract: Incorrect number of arguments provided. Expecting 2, 3 provided

I have created an issue for this. https://github.com/r0gue-io/pop-cli/issues/336
Removed. This is better addressed as part of this PR as pointed out.

[AlexD10S]

Option types are covered for constructors now, awesome!

Doing some local testing I detected the following:

The contract constructor I have been using for this test:

#[ink(constructor)]
        pub fn new(init_value: Option<bool>, other_value: Option<bool>) -> Self {
            Self { value: init_value.unwrap_or(false) }
        }

If I execute:

pop up contract --args false,false

The behavior is the expected, the args are wrapped an it works nicely.

Although, it seems we are counting space chars as arguments.

pop up contract --constructor new --args false, false 
// outputs
An error occurred instantiating the contract: Incorrect number of arguments provided. Expecting 2, 3 provided

I have created an issue for this. #336

Good catch! Seems clap were discussing this: clap-rs/clap#5587
Let me look into the best approach here, it's easy to overlook that single space.

[evilrobot-01]

I have created an issue for this. #336

Why create an issue, this should be addressed in this PR.

[AlexD10S]

The best solution I have found for this issue is to remove the "," as delimiter value_delimiter = ',' (as it was before), which makes space the delimiter. Commas in arguments are removed when processing arguments, allowing users to input using,.
Example correct args:
--args false, false
--args false, ""
--args false false
--args false ""
Wrong args (without spaces):
--args false,false
--args false""

Open to better suggestions!

[evilrobot-01]

As a (lazy) user, I would want to write as little as possible, so would opt for --args false false. What would happen when there are two default args I want to omit: a: u8, b: Option<u8>, c: Option<u8>, d: u8. I dont expect --args 0 0 would work? Fine if not, provided --args 0, , , 0 does work.

[AlexD10S]

As a (lazy) user, I would want to write as little as possible, so would opt for --args false false. What would happen when there are two default args I want to omit: a: u8, b: Option<u8>, c: Option<u8>, d: u8. I dont expect --args 0 0 would work? Fine if not, provided --args 0, , , 0 does work.

In your example with parameters a: u8, b: Option<u8>, c: Option<u8>, d: u8, I liked the idea of allowing --args 0 0 to work. However, this approach could lead to confusion. For instance, with --args 0 1 0, it’s unclear which argument will be 1.

The format --args 0, , , 0 works well with the current implementation and avoids ambiguity. Similarly, --args 0, "", "", 0 is also effective. The key here is maintaining the whitespace between args, otherwise will fail.

[al3mart]

I agree that separating the different args with a space is nicer. Less typing.

Check this out - https://docs.rs/clap/latest/clap/struct.Arg.html#method.num_args
It gives some hints when a delimiter is not specified, and space separated values are just fine.

[evilrobot-01]

Whilst manually testing with Options I found that the UI would show the type as Option only, without the underlying type which makes valid entry impossible. #339 uses the type registry to format the type with its full representation, which should ease entry.

An example is a complex type like Weight, where previously I had to try various combinations until I discovered the correct syntax. The linked PR should show the expected type/value/shape for data entered.