Skip to content
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

Multichain: Audit MetaMaskController's getApi usages for chain ID #28268

Open
9 tasks
darkwing opened this issue Nov 4, 2024 · 0 comments
Open
9 tasks

Multichain: Audit MetaMaskController's getApi usages for chain ID #28268

darkwing opened this issue Nov 4, 2024 · 0 comments
Labels
multichain-final-boss Issues related to the final stage of the multichain project.

Comments

@darkwing
Copy link
Contributor

darkwing commented Nov 4, 2024

What is this about?

Work was done months ago to bring Controllers to a multichain architecture. That said, there could be regression. All of the methods that app/scripts/metamask-controller.js 's getApi method should be audited to ensure they are passed a chainId and not assume the global chain Id.

Per Mark:

Back to your question, the general area of concern I had was "every background operation that assumes it's for the globally selected chain ID". I had mentioned in a previous meeting something about auditing controller method calls or background API calls. It should be the case that all controller operations pertaining to a given chain accept a chainId parameter (apart from TokenBalancesController maybe, which someone is handling). But in many cases it's optional, and the globally selected chain is used as a default. Once we enable multichain, that would be a bug. The chain ID needs to be passed through everywhere.

Some subset of them accept an optional chainId or networkClientId. We could read each one to come up with that smaller list, then look at the callsites to ensure that it's always passed in.

Scenario

No response

Design

No response

Technical Details

No response

Threat Modeling Framework

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

No response

@darkwing darkwing added the multichain-final-boss Issues related to the final stage of the multichain project. label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multichain-final-boss Issues related to the final stage of the multichain project.
Projects
None yet
Development

No branches or pull requests

1 participant