dethcrypto / TypeChain

🔌 TypeScript bindings for Ethereum smart contracts
MIT License
2.76k stars 363 forks source link

Broken syntax for generated types #428

Closed RiccardoBiosas closed 2 years ago

RiccardoBiosas commented 3 years ago

I am migrating a truffle project to hardhat/typechain/trufflev5 and apparently typechain is generating types with a broken typescript syntax - the contracts should be compiled correctly as I am able to do that both in a truffle and openzeppelin environment

SyntaxError: '{' expected. (25:37)
  23 |   
  24 |
> 25 |   export interface Committed_address[] {
     |                                     ^
  26 |     name: "Committed"
  27 |     args: {
  28 |     whitelist : (string)[], 
krzkaczor commented 3 years ago

What's the input solidity code (source code/ filename)?

RiccardoBiosas commented 3 years ago

Hi @krzkaczor , I suspect that this is the the line of code responsible for triggering the error: https://github.com/OpiumProtocol/opium-contracts/blob/master/contracts/Lib/WhitelistedWithGovernance.sol#L13

krzkaczor commented 3 years ago

@RiccardoBiosas it would be great if you could prepare a minimal repo with a reproduction example. Even just ABI should be enough. I can try to work on this over next couple of days but without repro it will take much more time.

RiccardoBiosas commented 3 years ago

@krzkaczor Hi there, apologies for the belated reply. Here's the minimal repo which highlights the issue: https://github.com/RiccardoBiosas/typechain-bug-reproduction Apparently the issue affects @typechain/web3-v1 (or maybe @typechain/truffle-v5?), but not @typechain/ethers-v5

derekarends commented 2 years ago

@krzkaczor Using the sample provided by Riccardo it looks like when there are event overloads and those events have array args the code outputs invalid naming. This doesn't happen with ethers-v5 because it looks like for ethers we do not try to write both event interfaces in the overloading contract.

From sample:

contract WhitelistedWithGovernance is Whitelisted {
    event Committed(address[] whitelist);
}
contract WhitelistedWithGovernanceAndChangableTimelock is WhitelistedWithGovernance {
    event Committed(uint256 timelock);
}

These contracts for truffle-v5 and for web3-v1 create WhitelistedWithGovernance.d.ts fine but when it attempts to create WhitelistedWithGovernanceAndChangableTimelock.d.ts file containing overloaded interfaces it errors:

export interface Committed_uint256 { ...

export interface Committed_address[] { ...

I wasn't sure if both interfaces were to be expected or if we should have both interfaces in the types file and it will need to be renamed to something like Committed_address_array.

I am willing to create a PR for this just wasn't sure what is expected.

krzkaczor commented 2 years ago

@derekarends thanks for investigating this!

I think that we should generate names with explicit names like: Committed_address_array, when conflicting events are detected. We already do something like this for function signatures. And there is even a flag to always generate such overrides: --always-generate-overloads. I think you can use the same flag to control this behavior.

derekarends commented 2 years ago

I have a fix up for the issue with signatures when there are contracts that have event overrides.

I can create a new PR if we want to add the --always-generate-overloads. When I looked into it, I wasn't quite sure how it was used in this scenario. Would setting the flag make it so we always created full signatures like Committed_address_array even if it wasn't overloaded or would I want to change the code so that it only generate the overloaded events when this flag was set? Right now it looks to always generate the overload events for web3 and truffle.