Failure for `instantiate` and `call` on `String` argument

[cmichi]

Minimal reproducer contract

Execute cargo contract new bug and then replace lib.rs with:

#![cfg_attr(not(feature = "std"), no_std)]

#[ink::contract]
mod bug {
    use ink::prelude::string::String;

    #[ink(storage)]
    pub struct Bug {}

    impl Bug {
        #[ink(constructor)]
        pub fn new(val: String) -> Self {
            Self {}
        }

        #[ink(message)]
        pub fn get(&self) -> () {
            ()
        }
    }
}

Provoke bug

Run substrate-contracts-node and execute:

$ cargo contract instantiate --manifest-path=./Cargo.toml --suri //Alice --args Foo
ERROR: Expected a String value
$ RUST_LOG=trace cargo contract instantiate --manifest-path=./Cargo.toml --suri //Alice --args Foo
2022-10-13T08:55:39.060960Z DEBUG cargo_contract::crate_metadata: Fetching cargo metadata for ./Cargo.toml
2022-10-13T08:55:39.104122Z DEBUG contract_transcode::transcoder: No matching type in registry for path PathKey(["ink_primitives", "types", "AccountId"]).
2022-10-13T08:55:39.104143Z DEBUG contract_transcode::transcoder: No matching type in registry for path PathKey(["ink_primitives", "types", "AccountId"]).
2022-10-13T08:55:39.104148Z DEBUG contract_transcode::transcoder: No matching type in registry for path PathKey(["ink_primitives", "types", "Hash"]).
2022-10-13T08:55:39.104160Z DEBUG contract_transcode::encode: Encoding value `Tuple(Tuple { ident: Some("Foo"), values: [] })` with type id `0` and definition `Primitive(Str)`
ERROR: Expected a String value

Same thing happens for cargo contract call.

[SkymanOne]

The reason behind the bug is strange behaviour of scon_string() which delimites string by " char there???

The tests are also somehow "misleading" they assume that strings are provided in r#""Foo""# format?

[SkymanOne]

@cmichi you can actually parse a correct value without modifying any code:
RUST_LOG=trace cargo contract instantiate --suri //Alice --args \""Foo"\"

[SkymanOne]

So there are 3 possible solutions to this problem:

1. get rid off .delimited_by(tag("\"")) and accept a string as it is (without parenthesises)
2. somehow detect that the value is a string and add extra parenthesises to it
3. leave as it is and document that string must be provided with parenthesises explicitly

[cmichi]

@SkymanOne s/parnthesises/quote marks/g in your comment above.

Sidenote: Shell substitution removes the quotes " before passing the arguments to the executed process, quotes without escaping will never be visible to the executed command. With \"Foo\" they are retained.

Ideally we would auto-detect that the argument for the contract at this position is a String and just interpret the cli argument as a String then. Is this possible currently?

[SkymanOne]

@SkymanOne s/parnthesises/quote marks/g in your comment above.

Sidenote: Shell substitution removes the quotes " before passing the arguments to the executed process, quotes without escaping will never be visible to the executed command. With \"Foo\" they are retained.

Ideally we would auto-detect that the argument for the contract at this position is a String and just interpret the cli argument as a String then. Is this possible currently?

It was late night, apologies for confusion 😁

Auto-detecting the contract args is actually partially related to #750 and is more involved that it seems.

My speculated guess would that we need to parse the metadata during the parsing stage and accept/reject args based on the specified grammar (e.g. string arg is indeed in String format, the tuple is tuple, etc). This actually sounds more or less what I meant in the second proposed solution in the comment earlier.

I have to play around and see how to approach this the best.

[ascjones]

The original idea was to allow input in Rust like object initializer syntax e.g. MyStruct { a: 1, b: "hello" }. So strings should be delimited for this to work, so MyStruct { a: 1, b: hello } would be invalid, the parser would assume that hello is a type identifier now.

For passing strings from the command line without escaped quotation marks...I don't know what the solution will be. I do think though that some delimiters are required since strings can also contain spaces. I think in order to stick with the Rust like syntax, we should continue to interpret e.g cargo contract instantiate --manifest-path=./Cargo.toml --suri //Alice --args Foo as a type identifier.

[SkymanOne]

The original idea was to allow input in Rust like object initializer syntax e.g. MyStruct { a: 1, b: "hello" }. So strings should be delimited for this to work, so MyStruct { a: 1, b: hello } would be invalid, the parser would assume that hello is a type identifier now.

For passing strings from the command line without escaped quotation marks...I don't know what the solution will be. I do think though that some delimiters are required since strings can also contain spaces. I think in order to stick with the Rust like syntax, we should continue to interpret e.g cargo contract instantiate --manifest-path=./Cargo.toml --suri //Alice --args Foo as a type identifier.

While it makes sense. We might introduce an additional flag --raw-args to explicitly treat contracts' args as string values and try to convert them to corresponding types

[ascjones]

to explicitly treat contracts' args as string values and try to convert them to corresponding types

I'm not sure about this: what benefits would this give over the existing solution?

To solve for the problem of avoiding having to escape double quotation marks, we could consider allowing e.g. single quotation marks e.g. 'Foo' or some alternative delimiter (possibly configurable).

Then you can always enhance the error message ERROR: Expected a String value with a hint about how to specify strings.

Dealing with quotation marks on a CLI is tricky and also needs testing across OSs.

[ascjones]

Ideally we would auto-detect that the argument for the contract at this position is a String and just interpret the cli argument as a String then. Is this possible currently?

It would be possible, at the moment my guess is that Foo is interpreted as a "unit tuple" https://github.com/paritytech/cargo-contract/blob/f7d6b0673df7957087365202ee5a7ac1196bc4aa/crates/transcode/src/scon/parse.rs#L77

So you could allow it that a "unit tuple" (and any other scon literal) can be cooerced into a string where a String is expected. Although the parser is only valid if it is a valid SCON literal (falling back to a valid Rust identifier) so would not parse for other strings. Also it does slightly reduce the type checking, imagine if you have fn message(a: bool, b: String) it would then be possible to supply --args false true.

So we are looking at a tradeoff between ease of use "I don't want to have to specify that I am supplying a String, escaping, argghh!", and strictness "If it expects a string as an argument, you must supply a delimited string, sorry".

If javascript has taught us anything, it is better to be strict?

[SkymanOne]

@ascjones I agree with your philosophy of explicitness. Improving error message by giving a little more context would be a nice option as well as allowing single quotation marks. I would like to hear what @cmichi has to say

[pgherveou]

Just experienced this. As a user, it would have been nice to be told to explicitly wrap my string between escaped quotes