-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Export EIP-6963 Types #7270
Export EIP-6963 Types #7270
Conversation
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
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.
Benchmark
Benchmark suite | Current: 416db8f | Previous: facc2e6 | Ratio |
---|---|---|---|
processingTx |
22147 ops/sec (±6.10% ) |
22984 ops/sec (±5.69% ) |
1.04 |
processingContractDeploy |
38334 ops/sec (±8.19% ) |
38640 ops/sec (±12.90% ) |
1.01 |
processingContractMethodSend |
15502 ops/sec (±7.72% ) |
16387 ops/sec (±6.56% ) |
1.06 |
processingContractMethodCall |
25953 ops/sec (±9.11% ) |
28396 ops/sec (±6.55% ) |
1.09 |
abiEncode |
43174 ops/sec (±7.06% ) |
44603 ops/sec (±6.87% ) |
1.03 |
abiDecode |
29655 ops/sec (±7.46% ) |
32125 ops/sec (±5.97% ) |
1.08 |
sign |
1514 ops/sec (±0.88% ) |
1562 ops/sec (±3.66% ) |
1.03 |
verify |
363 ops/sec (±0.50% ) |
369 ops/sec (±0.44% ) |
1.02 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.x #7270 +/- ##
=======================================
Coverage 94.44% 94.44%
=======================================
Files 216 216
Lines 8463 8463
Branches 2339 2340 +1
=======================================
Hits 7993 7993
Misses 470 470
Flags with carried forward coverage won't be shown. Click here to find out more. |
export interface EIP6963ProviderInfo { | ||
uuid: string; | ||
name: string; | ||
icon: string; | ||
rdns: string; | ||
} | ||
|
||
export interface EIP6963ProviderDetail<API = Web3APISpec> { | ||
info: EIP6963ProviderInfo; | ||
provider: EIP1193Provider<API>; | ||
} | ||
|
||
export type EIP6963ProviderResponse = Map<string, EIP6963ProviderDetail>; | ||
|
||
export interface EIP6963ProvidersMapUpdateEvent extends CustomEvent { |
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.
why these types are moved from web3 to types package?
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.
Because I think they should be exported and it seemed like the best way to go about it. I can modify the PR export them directly from the web3
package, but this seemed like a better approach to me.
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.
usually we move types in web3-types package if these are used by 1+ package / are common types.
As web3-types package is a dependency of almost all other packages so we try to keep it light for usecase of other individual packages usage.
( same for utils package , for future changes)
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 believe since we created web3-types, it should encompass all the neccesary types to use the package. I think as well remain consistent, as we have EIP1193Provider types EIP6693 provider types should be included. This will allow these types to be exported through the main package which would make alot of since for most users, as I believe not very many users would know that EIP6693 types would be exported through providers namespace. We had a discussion on this a while back on this issue about doing this change #6952
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.
as I believe not very many users would know that EIP6693 types would be exported through providers namespace.
I agree on this, that these types should be directly exported from web3 package, instead of via namspace.
But regarding first point : only shared types shouldbe in web3-types package, and eip6963 types are used in web3 main package only? Same applies to other packages non-shared types like https://github.com/web3/web3.js/blob/4.x/packages/web3-eth-contract/src/types.ts
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 see what you are getting at, I think id still prefer it being within web3-types package as it feels intuitive for an eip type to be there, but it should be fine as long as this is documented and users are able to bring these types in properly
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.
also EIP-6963 types are exported under provider namespace:
export * from './web3_eip6963.js'; |
web3.js/packages/web3/src/index.ts
Line 353 in 6f9a485
export * as providers from './providers.exports.js'; |
but I think we may also export directly
Closes #6952 |
93cbb36
to
6a88b18
Compare
Export EIP-6963 types