Conversation
|
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. |
| }, | ||
| }, | ||
| accountTokens: { | ||
| accountTokens: { // this state is stale |
There was a problem hiding this comment.
i don't see this referenced anywhere
| '0x9d0ba4ddac06032527b140912ec808ab9451b788': {}, | ||
| }, | ||
| accountHiddenTokens: { | ||
| accountHiddenTokens: { // this state is stale |
There was a problem hiding this comment.
i don't see this referenced anywhere
| 'NetworkController:stateChange', | ||
| ), | ||
| onTokenListStateChange: (tokenListState) => | ||
| console.log('onTokenListStateChange', tokenListState), // this is missing |
There was a problem hiding this comment.
This is now a required field by the TokensController.
It's used like this in mobile: https://github.com/MetaMask/metamask-mobile/blob/2326d8707aeb01480d765bc417553b0517ab8411/app/core/Engine.ts#L299
| /** The transaction status property is not considered sensitive and is now included in the non-anonymous event */ | ||
| let properties = { | ||
| chain_id: chainId, | ||
| chain_id: caipChainId, // what to do here? |
There was a problem hiding this comment.
probably have left a comment elsewhere, but are we okay with using CAIP chain ID values for the chain_id metrics property?
There was a problem hiding this comment.
fix dependencies once core is merged
| * dapp may specify any caipChainId. | ||
| */ | ||
| export type ChainId = (typeof CHAIN_IDS)[keyof typeof CHAIN_IDS]; | ||
| // export type ChainId = (typeof CAIP_CHAIN_IDS)[keyof typeof CAIP_CHAIN_IDS]; |
There was a problem hiding this comment.
any need to keep a type that includes all built caip chain IDs around? I'm thinking no
| build() { | ||
| this.fixture.meta = { | ||
| version: 74, | ||
| version: 91, |
There was a problem hiding this comment.
is this okay for us to bump? It's not clear to me what version this fixture has been left at. It's definitely not 74
| category: 'inpage_provider', | ||
| locale: 'en', | ||
| chain_id: '0x539', | ||
| chain_id: 'eip155:1337', // same |
There was a problem hiding this comment.
are we okay with chain_id metrics being changed to caip chain id?
| category: 'inpage_provider', | ||
| locale: 'en', | ||
| chain_id: '0x539', | ||
| chain_id: 'eip155:1337', // what should this be? |
There was a problem hiding this comment.
are we okay with chain_id metrics being changed to caip chain id?
There was a problem hiding this comment.
this spec seems partially broken for firefox on my local machine. can revert and recheck on CI later
| export default { | ||
| title: 'Components/App/AddNetwork', | ||
| argTypes: { | ||
| // this component doesn't take props. what are these? |
|
|
||
| const { address: from } = useSelector(getSelectedAccount); | ||
| const network = hexToDecimal(useSelector(getCurrentChainId)); | ||
| const network = toEthChainIdInt(useSelector(getCurrentCaipChainId)); // is this named wrong? |
| address, | ||
| error, | ||
| chainId, | ||
| caipChainId, // this isn't even used in the slice? |
There was a problem hiding this comment.
this file confuses me a little
| // await screen.findByText( | ||
| // 'URLs require the appropriate HTTP/HTTPS prefix.', | ||
| // ), | ||
| // ).toBeInTheDocument(); |
There was a problem hiding this comment.
Need to revert this and test in CI later. I believe nock is giving me grief locally
|
Postponed. Not sure if/when will be picked back up. |
Explanation
As part of the push for multichain, we want to move away from Eth specific references to chainId. We achieve this by replacing our usage of hex/dec chain IDs with CAIP-2 (chain agnostic, human readable) chain IDs.
chainIdtocaipChainIdSee: MetaMask/utils#116
See: MetaMask/core#1489
See: MetaMask/smart-transactions-controller#184
Screenshots/Screencaps
None. These change should should be minimally apparent to the end user. Users will continue to use and be shown hex/dec chainIds denoting an Ethereum based chain. The only caveat is for the Network Form:
Manual Testing Steps
These changes can be tested by using the wallet as you normally would. Everything should function exactly the same except for the caveat with Network Form explained above.
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.