dethcrypto / TypeChain

🔌 TypeScript bindings for Ethereum smart contracts
MIT License
2.75k stars 361 forks source link

duplicated function name #718

Open EmanuelCampos opened 2 years ago

EmanuelCampos commented 2 years ago

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 commented 2 years ago

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

detroitpro commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/ethers-io/ethers.js/discussions/1456, https://github.com/ethers-io/ethers.js/issues/407#issuecomment-458360013.

In https://github.com/dethcrypto/TypeChain/issues/718#issuecomment-1164435557, 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 commented 2 years ago

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 commented 1 year ago

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