Skip to content

[ethers-v5] Add events support - #321

Merged
krzkaczor merged 11 commits into
dethcrypto:kk/ethers-v5-add-event-supportfrom
zemse:303-ethers-v5-add-event-support
Feb 21, 2021
Merged

[ethers-v5] Add events support#321
krzkaczor merged 11 commits into
dethcrypto:kk/ethers-v5-add-event-supportfrom
zemse:303-ethers-v5-add-event-support

Conversation

@zemse

@zemse zemse commented Jan 29, 2021

Copy link
Copy Markdown
Contributor

This PR adds the missing events support to the ethers v5 target and intends to close #303 and close #249. Thanks @quezak for #303 (comment)

Usage

const filter = contract.filters.Transfer(null, null, null); // TypedEventFilter<>
const result = await contract.queryFilter(filter); // TypedEvent<>

result[0].args.from // type support for named event parameters
result[0].args[0] // type support by index

contract.on(filter, (from, to, value, event) => {
  from; // string
  to; // string
  value; // BigNumber
  event; // TypedEvent<>
})

To test this in your project

  1. git clone -b 303-ethers-v5-add-event-support https://github.com/zemse/TypeChain.git
  2. cd TypeChain
  3. yarn (you need yarn installed if you don't)
  4. yarn build (this generates dist dir which is imp)
  5. Link this custom target to your project by placing the appropriate path to the target: typechain --target ../TypeChain/packages/target-ethers-v5/ (Docs)

Just in case if that sounds too much, I have temporarily published it to npm for testing:

  1. npm install typechain-target-ethers-v5 --save-dev (later you can just replace the package with official one)
  2. typechain --target ethers-v5 (Docs)

@changeset-bot

changeset-bot Bot commented Jan 29, 2021

Copy link
Copy Markdown

鈿狅笍 No Changeset found

Latest commit: ca120c7

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

@zemse
zemse marked this pull request as draft January 29, 2021 17:06
zemse added 2 commits January 30, 2021 01:24
- addListener function was removed since it does not exist in v5
- listeners and off was added
@zemse

zemse commented Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

Hi @krzkaczor, there is the following error when running the typecheck script (failing in CI) but my typescript compiler (v4.1.3) does not complain about it and seems to work fine. Can you help how this issue can be resolved?

types/commons.ts(15,20): error TS1256: A rest element must be last in a tuple type.
error Command failed with exit code 1.

@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.

Some initial comments before I try this out on my contracts.

Random thought: is it a good idea to skip the whole typed listeners section if a contract has no defined events?

Comment on lines +68 to +80
listeners(eventName?: string): Array<Listener>;
off(eventName: string, listener: Listener): this;
on(eventName: string, listener: Listener): this;
once(eventName: string, listener: Listener): this;
removeListener(eventName: string, listener: Listener): this;
removeAllListeners(eventName?: string): this;

listeners<T, G>(eventFilter?: TypedEventFilter<T, G>): Array<TypedListener<T, G>>;
off<T, G>(eventFilter: TypedEventFilter<T, G>, listener: TypedListener<T, G>): this;
on<T, G>(eventFilter: TypedEventFilter<T, G>, listener: TypedListener<T, G>): this;
once<T, G>(eventFilter: TypedEventFilter<T, G>, listener: TypedListener<T, G>): this;
removeListener<T, G>(eventFilter: TypedEventFilter<T, G>, listener: TypedListener<T, G>): this;
removeAllListeners<T, G>(eventFilter: TypedEventFilter<T, G>): this;

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.

I think the more specific overloads should be first, since TS checks them top-to-bottom IIRC

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.

is it a good idea to skip the whole typed listeners section if a contract has no defined events?

It's a definitely great idea. Since even if these typed listener section lies there useless for contracts without events, I don't think it would affect normal functionality (lmk if it does). Would it be fine if we do that in a separate PR? If not I can do that here.

Comment thread packages/target-ethers-v5/src/index.ts Outdated
import { EventFilter, Event } from 'ethers'
import { Result } from '@ethersproject/abi'

export interface TypedEventFilter<T, G> extends EventFilter {}

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.

why are the T & G args here if they're not used?

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.

Actually the T & G contain type info utilised by queryFilter and other methods for output

Comment thread packages/target-ethers-v5/src/index.ts Outdated
@zemse
zemse marked this pull request as ready for review February 9, 2021 11:38
@krzkaczor

krzkaczor commented Feb 11, 2021

Copy link
Copy Markdown
Member

@zemse as for this TS error, it seems that it works only in TS 4.0+ (https://stackoverflow.com/questions/53356652/typescript-generic-tuples-error-rest-element)

You should get the same error if you use yarn.lock 馃 Anyway, it doesn't matter, we can bump TS to 4.0 (or even newer?). And bump peer dependency from "typescript": ">=3.8.0", to 4+ in packages/target-ethers-v5/package.json.

Soo, could you:

  1. Normalize all package.json dependencies on typescript to 4.0
  2. Bump peer dependency? in packages/target-ethers-v5/package.json

I will deploy it as major version of ethers-v5 to avoid breaking something.

Thanks!

@krzkaczor

krzkaczor commented Feb 14, 2021

Copy link
Copy Markdown
Member

@zemse can we add a test (can be the same thing as you put in this PR description) proofing that this works (and continue to work in future ;) )?

@zemse

zemse commented Feb 20, 2021

Copy link
Copy Markdown
Contributor Author

I've added some tests pls review

@krzkaczor
krzkaczor changed the base branch from master to kk/ethers-v5-add-event-support February 21, 2021 16:07
@krzkaczor
krzkaczor merged commit 297e320 into dethcrypto:kk/ethers-v5-add-event-support Feb 21, 2021
@krzkaczor

Copy link
Copy Markdown
Member

@zemse thanks for this! Released in: https://github.com/ethereum-ts/TypeChain/releases/tag/%40typechain%2Fethers-v5%406.0.0

@fubar

fubar commented Feb 25, 2021

Copy link
Copy Markdown

@zemse how is this supposed to work? Maybe I'm missing something but the first line in your example contract.filters.Transfer() is invalid because the declaration of that function is generated with arguments:

Transfer(
  from: string | null,
  to: string | null,
  value: null
): TypedEventFilter<
  [string, string, BigNumber],
  { from: string; to: string; value: BigNumber }
>;

Which means those 3 arguments are required.

@zemse

zemse commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

Oh right, you need to do the following, will update the description too.

contract.filters.Transfer(null, null, null)

As a side note: Actually ethers.js on it's core is fine without passing any args, so the args should be made optional in the target too. Will make a PR soon.

@fubar

fubar commented Feb 25, 2021

Copy link
Copy Markdown

@zemse sounds good, 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.

Add type support for ethers v5 queryFilter Adding event type for ethers

4 participants