Skip to content

Fix ethers-v5 code-generation for no-argument events - #331

Merged
krzkaczor merged 2 commits into
dethcrypto:masterfrom
dwardu:master
Feb 27, 2021
Merged

Fix ethers-v5 code-generation for no-argument events#331
krzkaczor merged 2 commits into
dethcrypto:masterfrom
dwardu:master

Conversation

@dwardu

@dwardu dwardu commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@typechain/ethers-v5-test
 | types/Events.d.ts(242,20): error TS2314: Generic type 'TypedEventFilter<EventArgsArray, EventArgsObject>' requires 2 type argument(s).
@changeset-bot

changeset-bot Bot commented Feb 26, 2021

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 680cef3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@quezak quezak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
Does this fix #321 (comment) ?

@krzkaczor

Copy link
Copy Markdown
Member

CC: @zemse

@dwardu

dwardu commented Feb 26, 2021

Copy link
Copy Markdown
Contributor Author

@quezak The code introduced in #321 generates bad code for events that have 0 arguments. I think the comment you pointed to is different, because the Transfer event being discussed there has 3 arguments.

The way the “output type” is being generated is by calling generateOutputTypes (which calls generateOutputComplexType) , which returns “complex output types” like '[boolean, BigNumber] & {value1: boolean,value2: BigNumber }'… and then taking this string and replacing & with , to produce the type-parametrization necessary for TypedEventFilter<EventArgsArray, EventArgsObject>. It’s a bit hacky, and the hack doesn't work for 0-arg events, because internally generateOutputComplexType generates the 2nd output-type only for 1+ outputs.

My fix is to, for the 0-arg event case, avoid the “hack” and use '[], {}' as the “output type” directly.

@quezak

quezak commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

@quezak The code introduced in #321 generates bad code for events that have 0 arguments. I think the comment you pointed to is different, because the Transfer event being discussed there has 3 arguments.

The way the “output type” is being generated is by calling generateOutputTypes (which calls generateOutputComplexType) , which returns “complex output types” like '[boolean, BigNumber] & {value1: boolean,value2: BigNumber }'… and then taking this string and replacing & with , to produce the type-parametrization necessary for TypedEventFilter<EventArgsArray, EventArgsObject>. It’s a bit hacky, and the hack doesn't work for 0-arg events, because internally generateOutputComplexType generates the 2nd output-type only for 1+ outputs.

My fix is to, for the 0-arg event case, avoid the “hack” and use '[], {}' as the “output type” directly.

OK, thanks for the explanation -- I wanted to confirm if this solves that linked case too.
Thanks for the PR!

@dwardu

dwardu commented Feb 26, 2021

Copy link
Copy Markdown
Contributor Author

You’re welcome @quezak. Thank you. TypeChain is a great tool, and I am happy to contribute this tiny fix.

@krzkaczor

Copy link
Copy Markdown
Member

Thanks @dwardu! This looks good. I just want to hear @zemse's opinion on this before merging.

I will make sure to release this ASAP as it seems like an urgent fix.

@zemse

zemse commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

Thanks @dwardu for the fix! I regret that this error was introduced due to the previous PR and sorry for the inconvenience in your live projects. Since the generateOutputTypes generates the 2nd output-type only for 1+ outputs, the case for 0 outputs was needed to be handled, which this PR takes care of. 👍

@dwardu

dwardu commented Feb 26, 2021

Copy link
Copy Markdown
Contributor Author

Hi @zemse, no problem at all!, and thanks for programming the events support in the first place.

@krzkaczor
krzkaczor merged commit 46ede07 into dethcrypto:master Feb 27, 2021
@krzkaczor

Copy link
Copy Markdown
Member

Thanks for the fix. Released as: https://github.com/ethereum-ts/TypeChain/releases/tag/%40typechain/ethers-v5%406.0.1

@dwardu

dwardu commented Feb 27, 2021

Copy link
Copy Markdown
Contributor Author

That’s great. Thanks 🎉

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.

4 participants