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

feat: Multichain: Migrate getCurrentChainId to use page context #27710

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Oct 8, 2024

Description

This PR makes getCurrentChainId return the dapp context chainId when connected to a dapp, falling back to the global chainId when not connected to a dapp.

** TODO **

  • Based on this, we should initialize state.metamask.domains to always have metamask entry of whatever the default chainId is a MetaMask install (Mainnet for prod, Sepolia for dev)

Open in GitHub Codespaces

Related issues

Fixes: #27832

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@darkwing darkwing requested a review from a team as a code owner October 8, 2024 22:10
Copy link
Contributor

github-actions bot commented Oct 8, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@darkwing darkwing marked this pull request as draft October 8, 2024 22:11
@darkwing darkwing changed the title Multichain: Migrate getCurrentChainId to use page context feat: Multichain: Migrate getCurrentChainId to use page context Oct 8, 2024
ui/selectors/selectors.js Outdated Show resolved Hide resolved
@darkwing darkwing force-pushed the multichain-getCurrentChainId branch 2 times, most recently from 38d67a3 to d6c59e9 Compare October 18, 2024 19:37
// TODO: Fallback chainID should be from Settings, NOT getProviderConfig
const FALLBACK_CHAIN_ID = CHAIN_IDS.MAINNET;

if (state.metamask.domains === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Worth double-checking why this is undefined, this doesn't seem like it should be possible

Copy link
Member

Choose a reason for hiding this comment

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

At a glance this doesn't seem possible. Maybe caused by a bad fixture.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a possibility when the autoswitch toggle is off 🤔

@darkwing darkwing force-pushed the multichain-getCurrentChainId branch from 516919e to 94f4e23 Compare January 10, 2025 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multichain: Make getCurrentChainId Selector Contextual
4 participants