ethers should not warn on multiple functions if their signatures are different

[cellog]

Currently, ethers warns if overloaded functions in the contract abi exist, but it should not warn unless the signatures are actually different. Fixing this requires disabling all warning levels (#407 (comment)) manually, which is a bit of a hack. I would expect ethers to be silent by default, and have to explicitly opt into noisy console calls

[ricmoo]

In general, ethers is noisy about things that can cause failures. In v5 (coming soon), overloaded functions will not be available by their bare name, you will need to specify the full signature.

Are you including multiple overloaded functions where the signatures are the same?

The other solution is to only provide the fragment which you care about. For example:

// Full ABI
let abi = [
    "function foo(address)",
    "function foo(uint256)",
];

// This issues warnings about namespace collisions
let contract = new Contract(addr, abi, provider);

// If you only ever need the address variant, you can just exclude the uint256 from the fragment
let abiFragment = [
    "function foo(address)",
];

// This is nice an clean, and when you call contract.foo(someValue) it is obvious what is being called
let contract = new Contract(addr, abiFragment, provider);

Does that make sense?

[cellog]

It does. In our case, we are pulling in the auto-generated JSON from truffle into an ethers.Interface in order to do things. We don't want to monkey with this, as it could result in real consequences if we were to screw up.

I would much prefer that the warning happen when calling the overloaded function, rather then when parsing the json. In other words, I would rather that the parsing stage emit a notice for overloaded functions, and an error if the method descriptions are identical. On calling, a warning seems appropriate, with a note that the behavior is deprecated.

What do you think?

[ricmoo]

The ABI from Truffle should not contain duplicate entries for signatures that are the same though.

But I agree, it would be better if it complained at call time, which is how v3 did it, however, then it needs additional tracking, since we would ideally only complain the first time an ambiguous (and therefore assumed) call is made. Otherwise, every time you call it, you get a warning, which (oddly) is a serious performance impact on React Native environments.

I could put this back to be the default, but with v5 coming out so soon, I don't know if it is worth it. Are the console.log calls otherwise being an issue? I'm guessing you have a complex contract with many collisions?

[cellog]

Oh, the abi has different signatures, but the code that checks for collisions is comparing the name, not the signature. When is v5 coming out (ish)?

I can work around by disabling warnings for now

[cellog]

Oh and the main issue is our regression tests have 50+ lines of warnings :)

[ricmoo]

You can technically use v5 today (npm install ethers@next), but I would consider it quite unstable and there are no docs for it, and the source isn't available on GitHub yet. I'm frantically trying to get the source up on GitHub as we speak, it's just taking longer than expected, since there are a few other things that need to be split out and co-ordination with another team. ;)

You can also probably disable the warnings before constructing the Contract, and then re-enabling them after. If that is slightly less terrible. ;)

[wighawag]

I agree with @cellog that we should not get warning when parsing abi that contains function with different signature but same name.

These are actually valid ABI and while I would avoid naming function with same name in my own contract, some standard and other contracts have it this way.

I should not need to disable log for that.

[ricmoo]

This is why is it just a warning. Maybe it would make sense to just issue a single warning? But it is difficult, because there is no way at call-time to indicate that was the problem.

It does this in the first place because a lot of people were confused why they got “XXX is not a function” and caused a lot of support issues and e-mail. :p

Once we switch to Proxy, this all goes away because ambiguous operations can be detected at call-time, but that won’t be until v6.

I’m open to removing the warning, but wary...

[ricmoo]

This should be good now, if you still have problems, please feel free to re-open.

Thanks! :)