refactor: add an argument to enable strict argument checking in abigen macro

[iqdecay]

This PR refactors the function expand_function_arguments to avoid checking for template arguments (gas_, amount_, color_) every time. It propagates the argument up to the abigen macro so user can chose if the expansion should check for types or not.
It closes #75

[iqdecay]

Honestly had trouble coming up with this PR, so really taking any criticisms to improve the design I chose 😸
EDIT: overlooked some tests changes, fixing.

[digorithm]

You see, there seems to be a (very valid) misunderstanding that will lead to worse ergonomics of the abigen! macro -- which is already relatively hard to understand for newcomers.

Adding a bool to the abigen! will make things much more confusing, and here's why this ended up being a valid (mis)direction: this happened because in some of the tests in harness.rs, the JSON ABI being used was handcrafted and didn't follow what the compile generates, that's because it was handcrafted before the compiler was able to generate the JSON ABI.

In practice, this should never happen. In other words, I don't believe there's a reason to complicate the devEx around abigen! because users will use the JSON ABI generated by the compiler, which includes the mandatory 3 params. Meaning it should always follow the "strict_checking" logic introduced in this PR.

The original issue (#75) was created because, in Sway, if you give a different name to one of the mandatory params, it will show up in the JSON ABI generated by the compiler that way. E.g. if your sway fn is do_something(gass: u64, amount: u64, color_: b256, MyInput: u64) (notice the inconsistency), your JSON ABI will be generated the same way and the SDK shouldn't care, it should only care that's u64, u64, b256. This is a common mistake by Fuel users as we've seen in discussions on Discord.

I propose that we remove this extra bool param in the abigen! and use only the (correct) logic introduced in this PR that does the strict checking:

if strict_checking {
    if n_inputs != 4 {
        return Err(Error::MissingData(format!(
            "Expected 4 inputs because `strict_checking` is true, found {}",
            n_inputs
        )));
    }
    let given_types: Vec<String> = fun.inputs[..3]
        .iter()
        .map(|x| x.type_field.clone())
        .collect();
    let expected = ["u64", "u64", "b256"];
    if given_types != expected {
        return Err(Error::InvalidType(format!(
            "Expected the 3 first types to be {:?}, found {:?}",
            expected, given_types
        )));
    };
    // Ignore the first three arguments so we don't have to provide them when calling
    // the contracts' methods
    first_index = 3
}

Except that this will be the only path (no if strict_checking {}).

Then we update all tests to include the 3 mandatory params in the JSON ABI, as it should've been. So that it matches the output of the compiler.

This will effectively solve the friction that users have while incorrectly naming the 3 mandatory params in the Sway code and not complicate the abigen! macro (and not break it for current users!).

In the future, and I already have this documented somewhere, we should have the option of passing, instead of a compiler-generated JSON ABI, a human-readable list of function signatures to abigen! that could be easily handcrafted, just like ethers provide (https://docs.ethers.io/v5/api/utils/abi/formats/#abi-formats--human-readable-abi). This should be the place for users to handcraft the ABI, not in the JSON ABI that's generated by the compiler.

cc @adlerjohn

[iqdecay]

Ok I understand, it's just that John asked in #75 that we have "two" versions of the abigen, I thought about doing one abigen_strict and one abigen but figured factoring it would make more sense. Working on the requested changes: updating the logic and the tests.

[digorithm]

Ok I understand, it's just that John asked in #75 that we have "two" versions of the abigen, I thought about doing one abigen_strict and one abigen but figured factoring it would make more sense. Working on the requested changes: updating the logic and the tests.

Yup, I believe John asked that because you mentioned the tests that didn't include the 3 params. These tests are "invalid" because they should be using the compiler output. This distinction shouldn't be necessary under the (now corrected) assumption that the tests themselves should've included the 3 params.

[adlerjohn]

Ah I guess I wasn't clear. Yeah a abigen! and abigen_strict! would have been better than a Boolean parameter into a single abigen!. Either way, if @digorithm says that abigen should only work on things output from the compiler, then he's the boss.

[digorithm]

Ah I guess I wasn't clear. Yeah a abigen! and abigen_strict! would have been better than a Boolean parameter into a single abigen!. Either way, if @digorithm says that abigen should only work on things output from the compiler, then he's the boss.

IMO, there's a place for handcrafted input and another place for auto-generated inputs. That's one of the things that's nice about ethers-rs (and ethers.js); you have the JSON ABI that's auto-generated and more strict and the handcrafted human-readable version.

Trying to do both, in JSON, and on top of that adding more complexity to the macro... what a pain 😢.

[iqdecay]

Updated according the remarks, which changed a lot of tests along the way. I'm always trying to factorize as much as possible, but I guess for macros it's not necessarily a good idea?

[digorithm]

It's looking great! I played with it a bit and now inconsistent/wrong names for the first 3 params don't break anything 🎉. Great work!

I just left some requests for additional comments.

[iqdecay]

It also closes #13 right?

[adlerjohn]

It also closes #13 right?

Not really, because #13 is about removing workaround altogether. It should probably be re-worded given the changes that have been made since then, but we still need an issue to track removing workarounds for the mandatory arguments altogether.

[digorithm]

Ship it!