Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Definitive solution for Contract with functions overloading (ethers.js issue) #172

Open
marcelomorgado opened this issue Jan 28, 2019 · 3 comments
Labels
important and urgent tech debt Technical debt not involving bugs or features
Milestone

Comments

@marcelomorgado
Copy link
Contributor

Seems that ethers.js is loading the first function that appears on ABI and ignoring the others.
Ref: ethers-io/ethers.js#407

Related to: #160

@marcelomorgado marcelomorgado added the tech debt Technical debt not involving bugs or features label Jan 28, 2019
@marcelomorgado marcelomorgado added this to the 0.1.0 release milestone Jan 28, 2019
@marcelomorgado marcelomorgado removed this from the 0.1.0 release milestone Jan 28, 2019
@marcelomorgado marcelomorgado changed the title Contract with functions overloading - ethers.js issue Definitive solution for Contract with functions overloading (ethers.js issue) Jan 29, 2019
@marcelomorgado
Copy link
Contributor Author

As ethers.js is attaching to contract only the first ABI function by name if a contract has two functions with the same name (function overloading) only one (which appears on ABI first) will work.
For short term, as around solution, we are cutting-off from ABI functions that we aren't using.
As definitive solutions we can do:

  1. Wait for ethers.js fix.
    Contract with function overloading ethers-io/ethers.js#407

  2. Implement a smarter function attaching in our TasitAction.Contract.
    Since ethers allow do call that way: contract["overloading(string)"]("a"); we can improve our addFunctionsToContract function to read ABI and then dynamic create functions properlly.

I think that issue has medium severity since some standard contracts are using overloading (e.g.: ERC712.safeTransferFrom, ZOS.initialize) and we can't control our users/developers implementations.

@marcelomorgado
Copy link
Contributor Author

After ricmoo response on the issue, it's clear now that isn't an ethers.js issue. This kind of "smart dynamic functions" can't be done 100% for all cases. Ethers.js approach is to restrict overloading calls

...I error on the side of non-ambiguity. In v5, overloaded operations will not be exposed.
refs

We have to think about how the approach is more appropriated to our users. More friendly as web3.js or more strict as ethers.js.

@pcowgill
Copy link
Member

That makes sense. Hmm yes, this is interesting. I’ll have to think about this a bit more. Thanks for looking into it.

I think let’s put this in milestone 0.2.0, but handle it as one of the first tasks in that milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important and urgent tech debt Technical debt not involving bugs or features
Projects
None yet
Development

No branches or pull requests

2 participants