-
Notifications
You must be signed in to change notification settings - Fork 364
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
Conversation
🦋 Changeset detectedLatest commit: b1d079f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
c410820
to
af89cba
Compare
af89cba
to
e4edd2c
Compare
There was a problem hiding this comment.
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)? 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).
-
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
.changeset/good-starfishes-appear.md
Outdated
@@ -0,0 +1,6 @@ | |||
--- | |||
'@typechain/ethers-v5': minor | |||
'@typechain/ethers-v5-test': minor |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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< |
There was a problem hiding this comment.
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 }>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Usage
#440