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

duplicated function name #718

Open
EmanuelCampos opened this issue Jun 10, 2022 · 6 comments
Open

duplicated function name #718

EmanuelCampos opened this issue Jun 10, 2022 · 6 comments

Comments

@EmanuelCampos
Copy link

I'm using https://docs.openzeppelin.com/contracts/4.x/api/finance#VestingWallet that have some functions like release() and release(address token) and the types are coming like:

image

How would be a good solution for this?

@krzkaczor
Copy link
Member

hmm what's wrong with the current solution? can you show the typings for release() method?

@detroitpro
Copy link

I'm running into the same problem, here are screenshots of the d.ts file and the imports for the .sol file.

image
image
image
image

@detroitpro
Copy link

I think I found and understand what is going on here:

https://ethereum.stackexchange.com/a/127836

ethers handles overloaded functions differently - https://docs.ethers.io/v5/single-page/#/v5/migration/web3/-%23-migration-from-web3-js--contracts--overloaded-functions

changing

const a = await mynft.release(user1.address) 

to

const a = await mynft['release(address)'].(user1.address) 


@zemse
Copy link
Contributor

zemse commented Jun 26, 2022

@EmanuelCampos exactly as specified in the above message, if ethers find a name collision, in order to be explicit it does not populate the short method for avoiding ambiguity. See ethers-io/ethers.js#1456, ethers-io/ethers.js#407 (comment).

In #718 (comment), it seems that you are looking at different buckets in ethers.Contract object, one is contract.functions[methodName] and other is contract[methodName]. Only difference in them is the way static methods return, e.g. solidity methods can return multiple values, if they return just one then the contract[methodName] would simply return the value (that's why released(address) returns Promise<BigNumber>). While functions bucket directly returns the array of return arguments hence it returns Promise<[BigNumber]> (notice the square brackets).

I don't think this is an issue, but if you think it is please revert with more information.

@bfondevila
Copy link

I don't think this is a problem; but we could definitely improve the developer experience.

Rather than using the form const a = await mynft['release(address)'].(user1.address), I would say it's best to have a single function with optional parameters. For example, adding an overload of a method in solidity currently will force you to go and update all the previous references to that method in typescript to use the "string call" version of it, rather than directly the function.

I'll give you an example. This:

    "safeTransferFrom(address,address,uint256)"(
      from: PromiseOrValue<string>,
      to: PromiseOrValue<string>,
      tokenId: PromiseOrValue<BigNumberish>,
      overrides?: Overrides & { from?: PromiseOrValue<string> }
    ): Promise<ContractTransaction>;

    "safeTransferFrom(address,address,uint256,bytes)"(
      from: PromiseOrValue<string>,
      to: PromiseOrValue<string>,
      tokenId: PromiseOrValue<BigNumberish>,
      data: PromiseOrValue<BytesLike>,
      overrides?: Overrides & { from?: PromiseOrValue<string> }
    ): Promise<ContractTransaction>;

Could be simplified to this:

    safeTransferFrom(
      from: PromiseOrValue<string>,
      to: PromiseOrValue<string>,
      tokenId: PromiseOrValue<BigNumberish>,
      data?: PromiseOrValue<BytesLike>,
      overrides?: Overrides & { from?: PromiseOrValue<string> }
    ): Promise<ContractTransaction>;

If data is passed in, internally it would resolve to calling the contract using the more specific "safeTransferFrom(address,address,uint256,bytes)" signature, and if data wasn't present it would call "safeTransferFrom(address,address,uint256)".

This would obviously only be possible if the solidity code being written respected the standard of keeping the "optional" parameters at the end of the method signature, since Solidity doesn't understand the concept of optional.

@noahgenesis
Copy link

Just to add to the bad developer experience:

await factory['deploy(address,(address,bool),(uint256,uint256,uint256,uint256,uint256,uint256,uint256),(string,string,string),(address,address,address))'](
    deployer,
    params.bondParams,
    params.bondParamsInitializeBond,
    params.nftParams,
    params.treasuryParams,
)

I unfortunately have this code in production right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants