Closed sshmaxime closed 3 years ago
Thanks for writing this. Definitely hardhat integration can be improved.
I thought about a way simpler implementation - it should be possible to make await ethers.getContractFactory('System')
typed ie. return System__factory
not any
.
We already do something similar for truffle target and it should be possible to do it here.
LMK what do you think about this idea. It should be done as part of https://github.com/ethereum-ts/TypeChain/pull/363
Not bad, that would definitely be a step to a better integration.
In addition, note that, as I mentioned in the beginning, imo, it's a bit annoying to set the factory and then deploy from them. Even with your new system, if I have 5 contracts that I need to deploy here and there then I need to have 5 contractFactory variables along with 5 Contract variables and 5 contract factory init and 5 contract init, ie:
let network: Contract__Factory;
let network1: Contract1__Factory;
let network2: Contract2__Factory;
let network3: Contract3__Factory;
let network4: Contract4__Factory;
let networkC: Contract;
let network1C: Contract1;
let network2C: Contract2;
let network3C: Contract3;
let network4C: Contract4;
// then
describe('Test', async () => {
before(async() => {
network = await ethers.getContractFactory('network');
network1 = await ethers.getContractFactory('network1');
network2 = await ethers.getContractFactory('network2');
network3 = await ethers.getContractFactory('network3');
network4 = await ethers.getContractFactory('network4');
networkC = await network.deploy();
networkC1 = await network1.deploy();
networkC2 = await network2.deploy();
networkC3 = await network3.deploy();
networkC4 = await network4.deploy();
})
}
It's easily tiring ... (the fact that describe is not async doesn't help but that's not the point).
In essence, your system can and should definitely be done. But I'd add to it the extra feature that I presented above, would make it so much easier/cleaner, ie:
import Contracts from "../typechain";
let networkC: Contract;
let network1C: Contract1;
let network2C: Contract2;
let network3C: Contract3;
let network4C: Contract4;
// then
describe('Test', async () => {
before(async() => {
networkC = await Contracts.deploy();
networkC1 = await Contracts.deploy();
networkC2 = await Contracts.deploy();
networkC3 = await Contracts.deploy();
networkC4 = await Contracts.deploy();
})
}
I guess you get the idea.
Can also work on it btw if necessary :)
I get you BUT the rule that we try to follow in TypeChain is that we try not to come up with our own APIs but rather just provide types to existing not typed stuff. That's why typing properly getContractFactory
is in the scope of TypeChain (or rather ethers-v5/hardhat target) but arbitrary helpers on top are not. Some ppl might prefer one thing others something else.
I think what you did should be distributed as a Hardhat plugin (or simply lib?). Personally, I got used to squeeze deploy
in one line like: (await ethers.getContractFactory('network')).deploy()
etc but obviously it's not the most readable code 😆
Makes total sense, maybe I should reach out to hardhat about this then ;)
Yes, most def!
I will keep this issue open to track the improvement that I've mentioned.
it should be possible to make
await ethers.getContractFactory('System')
typed ie. returnSystem__factory
notany
.
How can i help with this? How do you type that?
@sshmaxime how do you think a new api should look like if it were added to hardhat-ethers?
How can i help with this? How do you type that?
We do it like this for truffle. I assume that something similar should be possible for a hardhat. I still haven't got time to work on this :/
Ok, I found a way of doing it! This is gonna be so cool!
Here's an example:
// file: mod.ts -- A mock up of our ethers helpers module
export abstract class Base {}
export class Child extends Base {
public f() {
return 1;
}
}
export interface E {
getContract(a: string): Base;
}
export function createE(): E {
return {
getContract(c: string) {
if (c === "Child") {
return new Child();
}
throw new Error("Not found " + c);
},
};
}
// file: overloads.ts -- this would be a typechain generated module
import { Child } from "./mod";
declare module "./mod" {
interface E {
getContract(a: "Child"): Child;
}
}
// file: a.ts -- This is a file that uses it.
import { createE } from "./mod";
const e = createE();
const c = e.getContract("Child");
console.log(c.f()); // typed as Child
const d = e.getContract("A"); // typed as Base, but in reality it throws.
BTW, if someone wants to implement this, I can help translate this from a mock to the real thing
@alcuadrado I'd be happy to help on this :) Will contact you on Discord if that's fine :)
@sshmaxime i feel like this shuld be coordinated in this repo. If you want to pick it up please use https://github.com/ethereum-ts/TypeChain/pull/363 as base branch.
Honestly, after thinking about it for a bit, i'd go a step further and extends the hre with a contracts object with every contracts built by the typechain.
Exemple:
import { contracts } from "hardhat";
describe(() => {
const alpha = contracts.myContract.deploy(1, 2, { from: signer1 });
});
describe(() => {
const alpha = contracts.myContract.attach({ from: signer1 });
});
Fully typed obviously. I think this would make the typechain more valuable. Thoughts ?
@sshmaxime, I think that's not always possible, as contract factories need to be initialized with a signer, and getting one is an async operation (especially if you are not using the in-process hardhat network). Also, some projects have 100s of contracts, and that may degrade the performance.
I'm really interested in making the hardhat-ethers plugin easier to work with. Do you have any idea on how we can improve it given those constrains?
@alcuadrado I totally get it, after mentioning those constrains it indeed makes it harder but as long as factories are there it won't be easier imo (for instance, in my project, in test or else it's annoying to load the factories then deploy, it's an extra line everytime I wanna use a contract). The plugin is already great like that but it forces me to do some extra work to avoid having to take care of the factory. IMO factory should be invisible for the end user.
@krzkaczor Seems like you already fixed it 🙏 . Any ETA on the release on Typechain v5 ?
Yes, this should be out today. ethers.getContractFactory(name)
returns concrete factory type 🥳 If you want to test it (which I would really appreciate):
Please checkout: https://github.com/ethereum-ts/TypeChain/pull/363
Build it:
yarn
yarn build
and then link packages locally
cd packages/typechain
yarn link
do the same for hardhat and ethers-v5 packages.
Finally, in the project that you want to test: yarn link typechain
(and do the same for hardhat and ethers-v5) and you will use your local packages.
Either it doesn't work or packages are unable to link correctly but I followed what you said step by step and getContractFactory(name)
is not typed. Maybe we can switch over to discord or telegram ?
@sshmaxime can you shoot me a message (@krzkaczor) via telegram?
I get the hardhat.d.ts
generated in my project's typechain
now. But the overloads are not visible when I hover the getContractFactory
method in the test cases, I've tried restarting vscode. Any ideas what could be wrong?
@zemse please make sure that your tsconfig
includes hardhat.d.ts
file: "include": ["*.ts", "**/*.ts"],
. (our examples will need some tweaking).
So it's a bit of a pain to use the lib as is with hardhat.
First I need to get the SystemFactory, then cast it, then deploy the contract from it. In order to avoid that I developed something that makes it easier.
And instead of doing that:
You can directly do:
With full types.
Here is how I did it:
Allowing anyone to do:
Indeed, everything is typed.
I think that could be inserted as a part of the typechain build. What's your thoughts ? It would make things easier imo