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

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

Merged
merged 11 commits into from
Sep 2, 2021

Conversation

zemse
Copy link
Contributor

@zemse zemse commented Aug 22, 2021

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
Copy link

changeset-bot bot commented Aug 22, 2021

🦋 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

Copy link
Member

@krzkaczor krzkaczor left a comment

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

@@ -0,0 +1,6 @@
---
'@typechain/ethers-v5': minor
'@typechain/ethers-v5-test': minor
Copy link
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
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
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
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.

@@ -201,10 +227,22 @@ export class Events extends BaseContract {
};

filters: {
"AnonEvent1(uint256)"(
value1?: BigNumberish | null
): TypedEventFilter<[BigNumber], { value1: BigNumber }>;
Copy link
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
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