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

Fix target-ethers-v5 to allow transaction and contract call overrides #254

Merged

Conversation

bradennapier
Copy link
Contributor

@bradennapier bradennapier commented Jul 9, 2020

Note: A lot of these changes appear to be due to the fact that I called yarn test:fix which the documentation for contributing specifies to do before pushing.

I only changed two files in this: codegen/index.ts and codegen/functions.ts

According to https://docs.ethers.io/v5/api/contract/contract/#Contract--check the signature of the estimateGas , populateTransaction and others should allow for overrides. These are not currently being built making it impossible to call them in many cases.

This simply reproduces the logic done for the functions which are produced correctly by adding the overrides (and documentation) for those calls.

It may be ideal to modify the documentation to mention it is an estimateGas etc but I didn't want to make that change myself.

I noticed shortly after this that the ethers-v4 is also affected by this flaw. We used the ethers-v4 typechain previously to produce for ethers-v5 and just made some modifications to make it work. When we did this it was defaulting estimateGas to the ethers definition which allows any arguments.

When using overloads there is a slight bug that should be addressed (probably in another PR). When you use overloads, the first function defined should be defined as the name of the fn, then all overloads should have the function names.

Given the ERC-20 example which is better types by ethers: https://docs.ethers.io/v5/api/contract/example/#example-erc-20-contract--meta-class-methods

Before

estimateGas: {
    name(): Promise<BigNumber>;
    symbol(): Promise<BigNumber>;
    decimals(): Promise<BigNumber>;
    totalSupply(): Promise<BigNumber>;
    balanceOf(account: string): Promise<BigNumber>;
    transfer(recipient: string, amount: BigNumberish): Promise<BigNumber>;
    allowance(owner: string, spender: string): Promise<BigNumber>;
    approve(spender: string, amount: BigNumberish): Promise<BigNumber>;
    transferFrom(
      sender: string,
      recipient: string,
      amount: BigNumberish
    ): Promise<BigNumber>;
    increaseAllowance(
      spender: string,
      addedValue: BigNumberish
    ): Promise<BigNumber>;
    decreaseAllowance(
      spender: string,
      subtractedValue: BigNumberish
    ): Promise<BigNumber>;
  };

After

estimateGas: {
    /**
     * Returns the name of the token.
     */
    name(overrides?: CallOverrides): Promise<BigNumber>;

    /**
     * Returns the symbol of the token, usually a shorter version of the name.
     */
    symbol(overrides?: CallOverrides): Promise<BigNumber>;

    /**
     * Returns the number of decimals used to get its user representation. For example, if `decimals` equals `2`, a balance of `505` tokens should be displayed to a user as `5,05` (`505 / 10 ** 2`).     * Tokens usually opt for a value of 18, imitating the relationship between Ether and Wei. This is the value {ERC20} uses, unless {_setupDecimals} is called.     * NOTE: This information is only used for _display_ purposes: it in no way affects any of the arithmetic of the contract, including {IERC20-balanceOf} and {IERC20-transfer}.
     */
    decimals(overrides?: CallOverrides): Promise<BigNumber>;

    /**
     * See {IERC20-totalSupply}.
     */
    totalSupply(overrides?: CallOverrides): Promise<BigNumber>;

    /**
     * See {IERC20-balanceOf}.
     */
    balanceOf(account: string, overrides?: CallOverrides): Promise<BigNumber>;

    /**
     * See {IERC20-transfer}.     * Requirements:     * - `recipient` cannot be the zero address. - the caller must have a balance of at least `amount`.
     */
    transfer(
      recipient: string,
      amount: BigNumberish,
      overrides?: Overrides,
    ): Promise<BigNumber>;

    /**
     * See {IERC20-allowance}.
     */
    allowance(
      owner: string,
      spender: string,
      overrides?: CallOverrides,
    ): Promise<BigNumber>;

    /**
     * See {IERC20-approve}.     * Requirements:     * - `spender` cannot be the zero address.
     */
    approve(
      spender: string,
      amount: BigNumberish,
      overrides?: Overrides,
    ): Promise<BigNumber>;

    /**
     * See {IERC20-transferFrom}.     * Emits an {Approval} event indicating the updated allowance. This is not required by the EIP. See the note at the beginning of {ERC20};     * Requirements: - `sender` and `recipient` cannot be the zero address. - `sender` must have a balance of at least `amount`. - the caller must have allowance for ``sender``'s tokens of at least `amount`.
     */
    transferFrom(
      sender: string,
      recipient: string,
      amount: BigNumberish,
      overrides?: Overrides,
    ): Promise<BigNumber>;

    /**
     * Atomically increases the allowance granted to `spender` by the caller.     * This is an alternative to {approve} that can be used as a mitigation for problems described in {IERC20-approve}.     * Emits an {Approval} event indicating the updated allowance.     * Requirements:     * - `spender` cannot be the zero address.
     */
    increaseAllowance(
      spender: string,
      addedValue: BigNumberish,
      overrides?: Overrides,
    ): Promise<BigNumber>;

    /**
     * Atomically decreases the allowance granted to `spender` by the caller.     * This is an alternative to {approve} that can be used as a mitigation for problems described in {IERC20-approve}.     * Emits an {Approval} event indicating the updated allowance.     * Requirements:     * - `spender` cannot be the zero address. - `spender` must have allowance for the caller of at least `subtractedValue`.
     */
    decreaseAllowance(
      spender: string,
      subtractedValue: BigNumberish,
      overrides?: Overrides,
    ): Promise<BigNumber>;
  };

This also handles the payable example properly since it is just reusing your previous logic:

/**
* @notice Deposit ETH
*/
function depositEther() external payable {
  deposit(msg.sender, address(0x0), msg.value);
}

from:

depositEther(): Promise<BigNumber>;

to:

/**
* Deposit ETH
*/
depositEther(overrides?: PayableOverrides): Promise<BigNumber>;

The 3 PR's combined, which we are using to generate working types for our contracts is https://github.com/bradennapier/TypeChain/tree/combined-prs

@bradennapier bradennapier changed the title fix: fn types to match ethers api Fix target-ethers-v5 to allow transaction and contract call overrides Jul 9, 2020
@zemse
Copy link
Contributor

zemse commented Jul 9, 2020

@bradennapier Thanks for finding the target's shortcomings and creating the PR, I'll try your PR this evening in my project and confirm.

@krzkaczor
Copy link
Member

this looks good @bradennapier! Thanks for the review @zemse! Did you have a chance to try it out?

@bradennapier The formatting changes that you mentioned are fine - prettier reformatted generated types because of a new argument.

@zemse
Copy link
Contributor

zemse commented Jul 9, 2020

I just tried this PR in my project and didn't get any issues. LGTM!

@bradennapier
Copy link
Contributor Author

bradennapier commented Jul 10, 2020

Yeah we ran through on our new contracts at IDEX, which are quite extensive, and they all work well! The changes on v4 are essential as well since it is plagued with the same issue - we tested on both since we use both versions atm.

@bradennapier
Copy link
Contributor Author

@krzkaczor any update on this? would love to see it deployed so we can stop using our symlinked versions :-)

@krzkaczor
Copy link
Member

Sorry @bradennapier i will get this PRs merged today.

@krzkaczor krzkaczor merged commit 65399af into dethcrypto:fork/ethers-v5-target Jul 16, 2020
@krzkaczor
Copy link
Member

Thanks! Published as @typechain/[email protected]

@bradennapier bradennapier deleted the fix/ethersv5-fn-types branch August 2, 2020 17:54
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

Successfully merging this pull request may close these issues.

3 participants