Skip to content

[ethers-v5] Generate and export typed events - #457

Merged
krzkaczor merged 11 commits into
dethcrypto:masterfrom
zemse:440-export-events
Sep 2, 2021
Merged

[ethers-v5] Generate and export typed events#457
krzkaczor merged 11 commits into
dethcrypto:masterfrom
zemse:440-export-events

Conversation

@zemse

@zemse zemse commented Aug 22, 2021

Copy link
Copy Markdown
Contributor

Usage

import type { Transfer_TypedEvent } from '../typechain/MyErc20Token';

// overloaded events
import type { Execute_bytes32_TypedEvent, Execute_address_bytes_TypedEvent } from '../typechain/MyMultisig';

// ...

const parsed = contract.interface.parseLog(log) as TransferEvent;
parsed.args.from // string
parsed.args.to // string
parsed.args.value // BigNumber

#440

@changeset-bot

changeset-bot Bot commented Aug 22, 2021

Copy link
Copy Markdown

馃 Changeset detected

Latest commit: b1d079f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@typechain/ethers-v5 Minor

Not sure what this means? Click here to learn what changesets are.

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

@zemse
zemse force-pushed the 440-export-events branch from c410820 to af89cba Compare August 22, 2021 18:26
@zemse
zemse force-pushed the 440-export-events branch from af89cba to e4edd2c Compare August 22, 2021 18:35

@krzkaczor krzkaczor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Does this implementation work with overloaded events (events with the same name but different signatures)? I don't think so as there would be a name conflict. I think we need to dump the whole signature into the type name if we detect multiple events with the same name (I think that there is already a function that does this).

  2. I think it makes sense to use these pregenerated exported types in the event filters code here: https://github.com/ethereum-ts/TypeChain/pull/457/files#diff-a783a7ebd19f6659f4d15eafcb5014e2881776e245845089383b47e5b4758f99R231

Comment thread .changeset/good-starfishes-appear.md Outdated
@@ -0,0 +1,6 @@
---
'@typechain/ethers-v5': minor
'@typechain/ethers-v5-test': minor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't version -test packages as they are not published. You can simply remove this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this implementation work with overloaded events (events with the same name but different signatures)?

Just noticed that it doesn't generate filters properly due to events overloading.

I think it makes sense to use these pregenerated exported types in the event filters code here https://github.com/ethereum-ts/TypeChain/pull/457/files#diff-a783a7ebd19f6659f4d15eafcb5014e2881776e245845089383b47e5b4758f99R231

I am not able to see the highlighted code from this PR link above. Do you see it or can you pls give me link from the repo code instead of from the PR diff?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry. New link: https://github.com/ethereum-ts/TypeChain/blob/master/packages/target-ethers-v5-test/types/Events.d.ts#L211

Basically this return type and extracted Event type represent the same event. I wonder if these could be somehow reused?

[BigNumber] & { value1: BigNumber }
>;

export type Event1_TypedEvent = TypedEvent<

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we simply use AnonEvent1Event instead of AnonEvent1_TypedEvent in a happy case? Where there are overloads let's use Event3_bool_uint256_Event instead of Event3_bool_uint256_TypedEvent. So basically just drop Typed.

filters: {
"AnonEvent1(uint256)"(
value1?: BigNumberish | null
): TypedEventFilter<[BigNumber], { value1: BigNumber }>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here it would be perfect to reuse the already generated type ie:
TypedEventFilter<AnonEvent1Event>; but I think it's not that simple so we can ship initial implementation without this and later improve.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree we should, that may involve some refactor to the TypedEventFilter generics. Maybe we can clean up the code in another PR.

@krzkaczor
krzkaczor merged commit 0c2b42c into dethcrypto:master Sep 2, 2021
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.

2 participants