Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature to add a Contracts object to typechain #370

Closed
sshmaxime opened this issue Apr 8, 2021 · 20 comments
Closed

Feature to add a Contracts object to typechain #370

sshmaxime opened this issue Apr 8, 2021 · 20 comments

Comments

@sshmaxime
Copy link

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:

systemF = (await ethers.getContractFactory('System')) as System__factory;
system = await systemF.deploy();

You can directly do:

import Contracts from "./ContractsHelper"

system = await Contracts.System.deploy();

With full types.

Here is how I did it:

let contractStore: { [key: string]: ContractFactory } = {};

const deployContract = async <ParamsTypes extends typeof ContractFactory.prototype.deploy, T extends Promise<Contract>>(
    contractName: string,
    argsNumber: number,
    args: Parameters<ParamsTypes>
): Promise<T> => {
    let signer: SignerWithAddress = (await ethers.getSigners())[0];

    // If full arguments then last arg is override
    if (argsNumber == args.length) {
        let overrides = args.pop();
        if (overrides != {} && overrides.from) {
            signer = await ethers.getSigner(overrides.from);
        }
    }

    let signerAddress = await signer.getAddress();
    if (contractStore[contractName + signerAddress] === undefined) {
        contractStore[contractName + signerAddress] = await ethers.getContractFactory(contractName, signer);
    }

    return args === undefined || args.length === 0
        ? await contractStore[contractName + signerAddress].deploy()
        : await contractStore[contractName + signerAddress].deploy(...args);
};

const attachContract = async <T extends Promise<Contract>>(
    contractName: string,
    address: string,
    signer?: SignerWithAddress
): Promise<T> => {
    return await ethers.getContractAt(contractName, address, signer);
};

const deployOrAttach = <T extends Contract, ParamsTypes extends typeof ContractFactory.prototype.deploy>(
    contractName: string,
    argsNumber: number
) => {
    return {
        deploy: async (...parameters: Parameters<ParamsTypes>): Promise<T> => {
            return await deployContract<ParamsTypes, Promise<T>>(contractName, argsNumber, parameters);
        },
        attach: async (address: string, signer?: SignerWithAddress): Promise<T> => {
            return await attachContract<Promise<T>>(contractName, address, signer);
        }
    };
};

export default {
    myContract: deployOrAttach<Contract, typeof ContractFactory.prototype.deploy>(
        'myContract',
        ContractFactory.prototype.deploy.length
    ),
}

Allowing anyone to do:

import Contracts from "./ContractsHelper";

// Normal deploy with default signer
contract = Contracts.myContract.deploy();

// Normal deploy with custom signer
contract = Contracts.myContract.deploy({ from: accounts[1].address });

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

@krzkaczor
Copy link
Member

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 #363

@krzkaczor krzkaczor mentioned this issue Apr 8, 2021
9 tasks
@sshmaxime
Copy link
Author

sshmaxime commented Apr 8, 2021

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 :)

@krzkaczor
Copy link
Member

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 😆

@sshmaxime
Copy link
Author

Makes total sense, maybe I should reach out to hardhat about this then ;)

@krzkaczor
Copy link
Member

Yes, most def!

I will keep this issue open to track the improvement that I've mentioned.

@alcuadrado
Copy link
Contributor

it should be possible to make await ethers.getContractFactory('System') typed ie. return System__factory not any.

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?

@krzkaczor
Copy link
Member

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 :/

https://github.com/ethereum-ts/TypeChain/blob/fc9182b33beceb0037607a00d8508c0d1419215c/packages/target-truffle-v4-test/types/truffle-contracts/index.d.ts#L17

@alcuadrado
Copy link
Contributor

alcuadrado commented Apr 13, 2021

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.

@alcuadrado
Copy link
Contributor

BTW, if someone wants to implement this, I can help translate this from a mock to the real thing

@sshmaxime
Copy link
Author

@alcuadrado I'd be happy to help on this :) Will contact you on Discord if that's fine :)

@krzkaczor
Copy link
Member

@sshmaxime i feel like this shuld be coordinated in this repo. If you want to pick it up please use #363 as base branch.

@sshmaxime
Copy link
Author

sshmaxime commented Apr 21, 2021

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 ?

@alcuadrado
Copy link
Contributor

@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?

@sshmaxime
Copy link
Author

sshmaxime commented Apr 23, 2021

@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.

@sshmaxime
Copy link
Author

@krzkaczor Seems like you already fixed it 🙏 . Any ETA on the release on Typechain v5 ?

@krzkaczor
Copy link
Member

krzkaczor commented Apr 29, 2021

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: #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.

@sshmaxime
Copy link
Author

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 ?

@krzkaczor
Copy link
Member

@sshmaxime can you shoot me a message (@krzkaczor) via telegram?

@zemse
Copy link
Contributor

zemse commented Apr 29, 2021

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?

@krzkaczor
Copy link
Member

krzkaczor commented Apr 29, 2021

@zemse please make sure that your tsconfig includes hardhat.d.ts file: "include": ["*.ts", "**/*.ts"],. (our examples will need some tweaking).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants