Improve typings for contracts

[krzkaczor]

Hey Richard!

I am the maintainer of TypeChain and I would like to forward some ideas that would greatly improve our generated types. I am more than happy to make PRs to fix this - I just want to discuss these ideas first here.

1. TypeChain generated contracts extend packages/contracts/src.ts/index.ts/Contract class. The problem is that this class has index signatures for functions which we can't override in any way.

Example:
readonly functions: { [ name: string ]: ContractFunction };

Despite the fact that we generate exact typings for all contract methods it's possible to mistype a method name and use a generic ContractFunction. This should result in a type error. I would propose extracting BaseContract interface without any index signatures so both ethers's Contract and TypeChain generated types can extend that.

Related to: dethcrypto/TypeChain#276 and dethcrypto/TypeChain#351

2. We realized that packages/contracts/src.ts/index.ts/Overrides is missing { from?: string | Promise<string> }. I would like to add it.

Related to: dethcrypto/TypeChain#345

WDYT? As I mentioned already I can prepare PR if you approve these changes.

[ricmoo]

Heya! Yes, I remember you. :)

I have that exact thing in v6 right now, even the same name, BaseContract. :)

I’m also hoping the next version of TS includes the PR for lifting the requirement of string indexes, and allows subsets of strings (e.g. keyof T, where T is passed into the Contract, which means things like TypeChain and the ethers-ts tool can simply define an interface to template against ;)).

I don’t think it is safe to make that change in v5 though... but I will consider porting the change back after I think about it a bit... it might be.

[krzkaczor]

I can't find a reason why this wouldn't be safe to change in the current codebase 🤔

Also what about 2)?

[ricmoo]

It may be. I just need to think more about it. Simple changes have broke things before, especially when they change .d.ts files.

2. I think you prolly want CallOverrides?

[krzkaczor]

@ricmoo when sending (not calling) a tx which one should I use? I think it should be Overrides. 🤔

[ricmoo]

Correct. When sending a transaction, it should use either Overrides or PayableOverrides. For either of those from is invalid. To override from the Contract object must be assigned a Signer. You cannot override the transaction from with Overrides, since that is very much a JSON-RPC-specific concept.

In ethers, providers don't have a notion of an account. It just happens that JsonRpcProvider has a method to fetch accounts, because that type of Provider has them. But it helps keep the distinction between a Provider and Signer.

To accomplish changing the from, you would need to use contract.connect(fromSigner).method(...).

Make sense?

[krzkaczor]

It does, but somehow passing from not only works but in some cases, it seems to be necessary. I saw some tests that impersonate some other contracts and they don't work when using .connect but they do when using from...

https://github.com/ethereum-optimism/contracts/blob/master/test/contracts/OVM/bridge/assets/OVM_L1ERC20Gateway.spec.ts#L95

Perhaps it only works in hardhat like environments (custom providers or something?) 🤔

[krzkaczor]

Anyway, regarding 1) did you make up your mind? 😆 maybe we could release a beta version with BaseContract but I really don't see any way that this could break something. "Contents" of Contract type won't change at the end of the day.

[ricmoo]

It’s a Hackathon weekend (NFTHack), so I won’t get a chance to dig in until after the weekend and after sleep. ;)

[krzkaczor]

@ricmoo have you got a chance to think about this? I am still up for making a PR to fix 1)

[ricmoo]

I’ve made the change locally and am playing with it.

I’ve also decided a lot is changing in this version, so I’ve bumped the minor version, which has required a lot of changes in the build process, extra scripts, etc., so I’m taking things extra careful.

And I’m using this as an opportunity to update a few other things that should only be made during a minor version bump.

I may get it out tonight or tomorrow, or I may need to spend a few more days looking at the changes more broadly. But it’s coming along. :)

[krzkaczor]

Okay, great to hear. Thanks!

I am working on the next major version of TypeChain and I wanted to ship it with this fix but it still needs more work. Probably I will release it sometime next weekend so it seems like it should be doable to get this included.

[ricmoo]

This is now in 5.1.0. Try it out and let me know how it goes. :)

[krzkaczor]

It seems to work great. Thanks!

I'll close it for now and I will re-open it in case of any problems.

[ricmoo]

Awesome! :)

[krzkaczor]

@ricmoo BaseContract migration is still not done on TypeChain side. I did some progress here: dethcrypto/TypeChain#364 BUT it's not that simple as BaseContract uses Contract here and there.

I wanted to come up with some reasonable fix and get back to you but maybe you have some good ideas how to fix this. Maybe using this instead of Contract as return type will work.

I will reopen that issue for now, I should have time to work on this very soon.

[ricmoo]

Does this still need to be open? I think it is difficult to make safe backwards compatible changes needed to directly facilitate this. In v6, I’m still hoping for a wider index signature to make it into TypeScript which will make things more elegant. :)

[krzkaczor]

@ricmoo closing this now. The problem that I described above was caused by ethers version mismatch. Thanks! Ethers & TypeChain work great together now!