dethcrypto / TypeChain

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

typechain ethers-v5 inclusion of hardhat-ethers #399

Open wighawag opened 3 years ago

wighawag commented 3 years ago

In the newest version of typechain, typechain ethers-5 target include types for the plugin hardhat-ethers

This pose problem for plugin that are alternative to hardhat-ethers like hardhat-deploy-ethers which have different function signature, but use the same hre.ethers object

An option to at least disable hardhat-ethers type generation would be good. AFter all the ethers v5 target should focus on ethers itself.

An alternative would be to create a specific hardhat ethers target

krzkaczor commented 3 years ago

In the newest version of typechain, typechain ethers-5 target include types for the plugin hardhat-ethers

What do you mean exactly? What kind of types for the plugin hardhat-ethers do you have in mind? HH specific stuff should be generated only if hardhat is detected.

wighawag commented 3 years ago

I have my own hardhat plugin that is an alternative to hardhat-ethers but have different types : https://github.com/wighawag/hardhat-deploy-ethers/

krzkaczor commented 3 years ago

@wighawag oh, I get it now. So, I thought in the past about extracting hardhat stuff from ethers target but I wanted to optimize for the most common usecase and making it as easy as possible for standard users.

An option to at least disable hardhat-ethers type generation would be good. AFter all the ethers v5 target should focus on ethers itself.

I would suggest not using hardhat integration then but rather use CLI directly. We detect harhdat by checking environment like:

this.cfg.flags.environment === 'hardhat'

If you don't set it you will get standard ethers types. Also creating your own plugin makes total sense here (which can use standard ethers plugin for most type generation I think).

wighawag commented 3 years ago

The issue is that I need to tell existing user that already use typechain hardhat plugin to remove it and replace it with something else. I could create my own typechain plugin but would be eaiser if I could extend the ethers-v5 target rather than forking it with a different hardhat handling..

Also the hardhat plugin system is designed around adding new types, so if hardhat-typechain keep forcing one way, other such plugin will have issues. This does not seem to be fitting with hardhat extensibility.

Would an option to disable hardhat not be a simple change ?

I currently do it to remain compatible : https://github.com/wighawag/hardhat-deploy-ethers/blob/a56433d7c1bdd687deea20bc55c8874b26ac147a/src/internal/index.ts#L16-L24

but that's a workarund

krzkaczor commented 3 years ago

What if we set environment to hardhat only if we detect that hardhat-ethers is installed? If I get it correctly your users shouldn't have this dep installed, right? Also exposing the option to force specific environment (non hardhat to avoid generation) in hardhat config should be useful.

wighawag commented 3 years ago

What if we set environment to hardhat only if we detect that hardhat-ethers is installed? If I get it correctly your users shouldn't have this dep installed, right?

the current version of hardhat-deploy-ethers is a fork, but I aim to make it an extension to hardhat-ethers and so hardhat-deplot-ethers users will have both installed, but the type will be still be affected by the hardhat-deploy-ethers plugin. hardhat allows this kind of type extension.

Also the current version of hardhat-deploy-ethers is often installed as an alias to @nomiclabs/hardhat-ethers to be compatible with other plugin that hardcode a dependency to @nomiclabs/hardhat-ethers

haydenyoung commented 2 years ago

As an outsider, I understand the issue results from the hardhat getContract* types being hardcoded; the hardcoded types are actually in typechain/ethers-v5, dist/congen/hardhat to be precise.

Because hardhat-deploy-ethers exposes new methods (in particular getContract()) and because typechain/ethers-v5 is hardcoding the ethers types, the getContract types are not available and, hence, typechain has no idea what getContract is returning (because the type definition never gets created).

Currently, the hardhat-deploy-ethers plugin deletes hardhat.d.ts which results in all of the getContract* functions having no type definitions.

Is there a way to create the types "on-the-fly" somehow, in order to take advantage of the extended getContract() function? Happy to help if I know I'm interpreting the problem correctly and can be given some guidance in how to potentially solve this issue.

luisnaranjo733 commented 2 years ago

As an outsider, I understand the issue results from the hardhat getContract* types being hardcoded; the hardcoded types are actually in typechain/ethers-v5, dist/congen/hardhat to be precise.

Because hardhat-deploy-ethers exposes new methods (in particular getContract()) and because typechain/ethers-v5 is hardcoding the ethers types, the getContract types are not available and, hence, typechain has no idea what getContract is returning (because the type definition never gets created).

Currently, the hardhat-deploy-ethers plugin deletes hardhat.d.ts which results in all of the getContract* functions having no type definitions.

Is there a way to create the types "on-the-fly" somehow, in order to take advantage of the extended getContract() function? Happy to help if I know I'm interpreting the problem correctly and can be given some guidance in how to potentially solve this issue.

So in theory I think it's possible for hardhat-deploy to ship with Typescript module definition likes (*.d.ts) that could "brute force" overwrite the types but that's pretty messy

I think it would be better if typechain could offer a way to de-couple itself from the hardhat ethers plugin to support forked plugins (mainly hardhat-deploy). The need for this is growing as other template projects (such as scaffold-eth) now depend internally on hardhat-deploy (which is honestly just a better version that the built in HH plugin).

Unrelated question - are there plans for hardhat-deploy to get unforked with the official HH plugin at some point? Would make the need for fixing this in typechain a lot more clear and present.

One final random note - I'm working around this typechain issue in my hardhat-deploy project by explicitly setting the generic type per the suggestion here. It's a minor nuisance, but it's a bummer because Typechain is so cool and it's so close to just working for hardhat-deploy projects too

https://github.com/wighawag/hardhat-deploy-ethers/issues/23#issuecomment-1101040259

krzkaczor commented 2 years ago

I am more than happy to accept a PR that would decouple ethersjs target from hardhat stuff, and enable creation of hardhat-deploy-ethers target.