Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Fix 3380: Add AbstractProvider #3451

Merged
merged 7 commits into from
May 5, 2020
Merged

Fix 3380: Add AbstractProvider #3451

merged 7 commits into from
May 5, 2020

Conversation

odanado
Copy link
Contributor

@odanado odanado commented Apr 1, 2020

Description

Fixes #3380

The only method that provider should implement is the send function.
I'd like you to consider whether the interface of EventEmitter should be implemented or not.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@@ -410,9 +413,14 @@ export interface LogsOptions {

export type BlockNumber = string | number | BN | BigNumber | 'latest' | 'pending' | 'earliest' | 'genesis';

export interface AbstractProvider extends EventEmitter {
Copy link
Contributor Author

@odanado odanado Apr 1, 2020

Choose a reason for hiding this comment

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

I would like to consider the following points

  • Using the extends EventEmitter.
  • Remove extends EventEmitter (Leaving only the send function in the interface).
  • Remove extends EventEmitter and directly define the on, once, and removeListener functions as well as the WebsocketProviderBase.

Copy link

@smithki smithki Apr 3, 2020

Choose a reason for hiding this comment

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

My $0.02 is to separate the core functionality from the subscription functionality. Not all custom providers support subscription features by default. Classes can implement multiple interfaces in TypeScript, so it shouldn't be problematic to export two interfaces.

I am not an active contributor to this code-base. This is my opinion only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @odanado thanks for this PR :) It LGTM. For your 3 points, perhaps just on/once/removeListener is sufficient as the "bare minimum".

What are the types of WebsocketProviderBase?

Also, in light of recent updates to EIP-1193, there is a new request method that would be great to add. I don't think having sendAsync would hurt either even though it's deprecated it has more long term support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryanio Thank you for the comment.
I didn't know the request method of EIP-1193.

I thought that the interface of the AbstractProvider should be an only of send and sendAsync.
The reasons are as follows.

  • The status of EIP-1193 is a draft.
  • EIP-1193 is not yet fully implemented in Metamask

I think it should define a provider that has methods such as on/once/removeListener/request separately from AbstractProvider.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.048% when pulling 20491df on odanado:fix-3380 into 67ea9d0 on ethereum:1.x.

@cgewecke cgewecke added 1.x 1.0 related issues Types Incorrect or missing types labels Apr 7, 2020
@ryanio
Copy link
Collaborator

ryanio commented May 5, 2020

Thanks for adding sendAsync 🙌 looks like there is a small dtslint issue.

@odanado
Copy link
Contributor Author

odanado commented May 5, 2020

@ryanio I forgot that fix a test.
I updated the test.

Copy link
Collaborator

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.x 1.0 related issues Types Incorrect or missing types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web3 constructor does not accept custom/non-standard providers in TypeScript
5 participants