Skip to content
This repository was archived by the owner on Jun 16, 2026. It is now read-only.

Remove networkId - #191

Merged
jiexi merged 7 commits into
mainfrom
jl/remove-network-id
Sep 27, 2023
Merged

Remove networkId#191
jiexi merged 7 commits into
mainfrom
jl/remove-network-id

Conversation

@jiexi

@jiexijiexi commented Aug 29, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Wallets shouldn't be directly concerned about the network ID as this more of a p2p concept for gossip. What wallets really care about is chain ID as that is the correct value to use to identify a chain, build transactions, etc. Although these two values usually match (ignoring hex/dec formatting), there are exceptions.

We want to remove usage of networkId from the SmartTransactionController.

@jiexi
jiexi requested a review from a teamAugust 29, 2023 21:03
jiexi
jiexi commented Aug 29, 2023
View reviewed changes
Comment threadpackage.json
@jiexi
jiexi requested a review from BelfordZAugust 29, 2023 21:07
@jiexi

jiexi commented Aug 29, 2023

Copy link
Copy Markdown
MemberAuthor

@metamaskbot publish-preview

@socket-security

socket-securityBot commented Aug 29, 2023•
edited
Loading

Copy link
Copy Markdown

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

PackagesVersionNew capabilitiesTransitivesSizePublisher
@metamask/network-controller10.3.1...13.0.0None+4/-51.81 MBmetamaskbot
@metamask/controller-utils4.2.0...5.0.0None+3/-41.59 MBmetamaskbot

mcmire
mcmire reviewed Aug 30, 2023
View reviewed changes
Comment threadsrc/SmartTransactionsController.ts
dan437
dan437 previously approved these changes Sep 8, 2023
View reviewed changes

@dan437dan437 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That worked!

shanejonas
shanejonas previously approved these changes Sep 11, 2023
View reviewed changes
@jiexi
jiexi dismissed stale reviews from shanejonas and dan437 via 68904a9September 11, 2023 17:02
@BelfordZ

BelfordZ commented Sep 11, 2023

Copy link
Copy Markdown

Needs the previews replaced with the releases, but looks good otherwise

@jiexi

jiexi commented Sep 13, 2023

Copy link
Copy Markdown
MemberAuthor

Blocked until MetaMask/core#1633 is merged and released

legobeat
legobeat reviewed Sep 25, 2023
View reviewed changes
Comment threadpackage.json Outdated
adonesky1
adonesky1 reviewed Sep 26, 2023
View reviewed changes
Comment threadCHANGELOG.md Outdated
adonesky1
adonesky1 previously approved these changes Sep 26, 2023
View reviewed changes

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

One non-blocking question

@jiexi
jiexi merged commit ded74e6 into mainSep 27, 2023
@jiexi
jiexi deleted the jl/remove-network-id branch September 27, 2023 16:45
@legobeatlegobeat mentioned this pull request Sep 27, 2023
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@BelfordZBelfordZAwaiting requested review from BelfordZ
5 more reviewers
@mcmiremcmiremcmire left review comments
@legobeatlegobeatlegobeat left review comments
@adonesky1adonesky1adonesky1 approved these changes
@shanejonasshanejonasshanejonas left review comments
@dan437dan437dan437 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jiexi@BelfordZ@mcmire@shanejonas@adonesky1@dan437@legobeat