feat: initial setup for not globally selected chain ID (useChainId hook)
#11753
Conversation
andreahaku
changed the title
andreahaku
force-pushed
the
feat/preparation_for_asset_chainId
branch
from
da78ed8
to
9c967a8
Compare
andreahaku
added
needs-dev-review
andreahaku
added
Priority - Medium
|
|
LGTM! |
andreahaku
changed the title
andreahaku
changed the title
useChainId hook)
|
| import { useSelector } from 'react-redux'; | ||
| import { selectChainId } from '../selectors/networkController'; | ||
|
|
||
| export const useChainId = () => { |
There was a problem hiding this comment.
I understand the convenience of having this as re-usable hook, but does this not still couple our UX to a global or semi-global chain ID at any one time?
What logic do we predict could populate this?
What if we want to render two components at once on alternate chain IDs for example?
I can't speak for the rest of the mobile UX, but the pattern we're using in confirmations is relying on the subject or data of the confirmation as a source of truth.
For example, since signatures and transactions are created on a specific chain, we can persist that in the controller state and read that chain where needed in the confirmation components.
There was a problem hiding this comment.
this will be able to return the selected chain ID from the of permitted chain ID (and the list of permitted chain IDs if needed) on the first phase.
When per-dapp network permissions will be ready, this will return the per-dapp selected chain ID and potentially also the list of the per-dapp permitted networks if needed.
This is the logic behind the creation of this hook
There was a problem hiding this comment.
it can return 'per-dapp' chain Id or the mobile app chain Id coming not a specific dapp but a general state
There was a problem hiding this comment.
and in theory components (like token lists and/or NFTs list) can iterate and show assets from all the permitted networks this way as what I've been doing is to have single elements of those lists have a "chainId" prop.
There was a problem hiding this comment.
But won't that excessively couple us to a dApp and a selected chain, where as in the final form of multi-chain, there won't be dApp selected chains, only requests on alternate chains.
If we treat the chain ID as a global or context based thing, does that not hinder the readability and dynamic nature of where the chain ID is coming from?
If we're looking at a signature or transaction, let's just look at its saved chain ID.
If we're viewing a NFT or ERC-20 token, they will have persisted chain ID also?
Why store it in a context / hook at all, if we can make it explicit with props or by reading it from the relevant state as needed?
Description
Initial test support for not globally selected chain ID applied to assets (token and NFTs) lists.
This PR creates a React hook (
useChainId) that replaces theselectChainIdselector used to get the globally selected chain Id.Currently, the network badge for assets is derived from the globally selected chain ID in the app. This global chain ID is shimmed via the new hook, effectively making it equal to the chain ID selected for the entire session or dapp. However, with the support of the hook and by adding the support of the chain ID at the single component (token and NFT) level we can move towards supporting a per-dapp chain ID.
Related issues
Fixes: https://github.com/orgs/MetaMask/projects/85/views/2?pane=issue&itemId=82943990
Related PRs
useChainIdhook) #11753 (this one)useChainIdto more token/nft components #11781useChainIdto components hooks #11811useChainIdcomponents views #11813useChainIdhook to all other UI components #11837Manual testing steps
Screenshots/Recordings
Before
As After
After
Simulator.Screen.Recording.-.iPhone.12.Pro.-.2024-10-11.at.14.26.31.mp4
Pre-merge author checklist
Pre-merge reviewer checklist