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

CAIP-2 Chain ID #1489

Closed
wants to merge 30 commits into from
Closed

CAIP-2 Chain ID #1489

wants to merge 30 commits into from

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jul 12, 2023

Explanation

  • Use CaipChainId type for internal chain representation
    • Convert back to hex/dec format only when required
  • Rename chainId to caipChainId
  • Add caip helpers to controller-utils for bridging the gap between our current usage of hex/dec chainId to caipChainId

See: MetaMask/utils#116
See: MetaMask/metamask-extension#19991

References

Changelog

NOTE: field change from chainId to caipChainId (or _CHAIN_ID to _CAIP_CHAIN_ID) also implies type change from Hex to CaipChainId

@metamask/address-book-controller

  • BREAKING: this.state.addressBook is now keyed by CaipChainId instead of Hex
  • BREAKING: this.state.addressBook[CaipChainId][address].chainId field changed to caipChainId
  • BREAKING: set() defaults the caipChainId arg (previously chainId) to eip155:1

@metamask/assets-controllers/AssetsContractController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: formatIconUrlWithProxy() expects params.chainId to be params.caipChainId
  • BREAKING: isTokenDetectionSupportedForNetwork expects chainId arg to be caipChainId

@metamask/assets-controllers/AssetsContractController

  • BREAKING: constructor expects options.chainId to be options.caipChainId

@metamask/assets-controllers/NFTController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: this.state. allNftContracts[userAddress] is now keyed by CaipChainId instead of Hex
  • BREAKING: this.state. allNfts[userAddress] is now keyed by CaipChainId instead of Hex
  • BREAKING: updateNestedNftState() expects passedConfig.chainId to be passedConfig.caipChainId
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to be providerConfig.caipChainId
  • BREAKING: addNft() expects accountParams.chainId to now be accountParams.caipChainId
  • BREAKING: checkAndUpdateSingleNftOwnershipStatus() expects accountParams.chainId to be accountParams.caipChainId
  • BREAKING: findNftByAddressAndTokenId() expects chainId arg to now be caipChainId
  • BREAKING: updateNft() expects chainId arg to be caipChainId
  • BREAKING: resetNftTransactionStatusByTransactionId() expects chainId arg to be caipChainId

@metamask/assets-controllers/NFTDetectionController

  • BREAKING: constructor expects options.chainId to be options.caipChainId

@metamask/assets-controllers/TokenDetectionController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to be providerConfig.caipChainId

@metamask/assets-controllers/TokenListController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to be providerConfig.caipChainId
  • BREAKING: this.state.tokensChainsCache is now keyed by CaipChainId instead of Hex

@metamask/assets-controllers/TokensRatesController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to now be providerConfig.caipChainId
  • BREAKING: findChainSlug() expects chainId arg to now be caipChainId
  • BREAKING: this.chainId changed to this.caipChainId

@metamask/assets-controllers/TokensController

  • BREAKING: constructor expects options.chainId to be options.caipChainId
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to now be providerConfig.caipChainId
  • BREAKING: this.state. allTokens is now keyed by CaipChainId instead of Hex
  • BREAKING: this.state.allIgnoredTokens is now keyed by CaipChainId instead of Hex
  • BREAKING: this.state.allDetectedTokens is now keyed by CaipChainId instead of Hex
  • BREAKING: addDetectedTokens() expects detectionDetails.chainId to now be detectionDetails.caipChainId

@metamask/controller-utils

  • ADDED: isEthCaipChainId() expects any value and returns true if the value is a valid CaipChainId with namespace eip155
  • ADDED: toEthCaipChainId() expects decimal or 0x prefixed hex string and returns CaipChainId
  • ADDED: toEthChainId() expects CaipChainId and returns reference as decimal string
  • ADDED: toEthChainIdHex() expects CaipChainId and returns reference as 0x prefixed hex string
  • ADDED: toEthChainIdInt()expects CaipChainId and returns reference as number
  • BREAKING: GANACHE_CHAIN_ID changed to GANACHE_CAIP_CHAIN_ID
  • BREAKING: BUILT_IN_NETWORKS[networkType].chainId field is now caipChainId
  • BREAKING: ChainId(hex values) changed to BuiltInCaipChainId
  • CHANGED: NetworkId renamed to InfuraNetworkId

@metamask/ens-controller

  • I'm not actually certain that this controller should be changed, but I have updated it. Can write changelog after confirming we are keeping these changes or not. I don't think we should though

@metamask/gas-fee-controller

  • BREAKING: constructor expects options.getChainId() to be options.getCaipChainId()
  • BREAKING: onNetworkStateChange() expects providerConfig.chainId to now be providerConfig.caipChainId
  • BREAKING: this.messagingSystem.call('NetworkController:getState') expects providerConfig.chainId to now be providerConfig.caipChainId

@metamask/message-manager/AbstractMessageManager

  • BREAKING: constructor expects options.getCurrentChainId() to be options.getCurrentCaipChainId()

@metamask/message-manager/TypedMessageManager

  • BREAKING: constructor expects options.getCurrentChainId() to be options.getCurrentCaipChainId()

@metamask/network-controller

  • BREAKING: ProviderConfig.chainId changed to ProviderConfig.caipChainId
  • BREAKING: NetworkConfiguration.chainId changed to NetworkConfiguration.caipChainId
  • BREAKING: this.state.providerConfig.chainId changed to this.state.providerConfig.caipChainId

@metamask/signature-controller

  • BREAKING: constructor expects options.getCurrentChainId() to be options.getCurrentCaipChainId()

@metamask/transaction-controller

  • BREAKING: Transaction.chainId changed to Transaction.caipChainId changed to
  • BREAKING: constructor expects options.getNetworkState() to return to include providerConfig.caipChainId instead of providerConfig.chainId

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@jiexi jiexi requested a review from a team as a code owner July 12, 2023 23:33
@jiexi jiexi marked this pull request as draft July 12, 2023 23:33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thoughts? I think we revert these changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having this controller take a CAIP chain ID makes sense from a consistency perspective. It also means that the consumer has to do less work in order to use it, even for an Ethereum network. However, we could update the constructor to pull the caipChainId from the network state in addition to the networkId and then notset a provider if it's not an Ethereum chain ID. This wouldn't prevent consumers from using set, get, etc., but they wouldn't do anything, because reverseResolveAddress wouldn't do anything, either. So, I feel like these changes are okay. What are your thoughts?

@jiexi jiexi marked this pull request as ready for review July 28, 2023 22:22
@jiexi jiexi requested a review from a team as a code owner August 1, 2023 20:09
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask/address-book-controller": "3.1.0-preview.19a3a61",
  "@metamask/announcement-controller": "4.0.0-preview.19a3a61",
  "@metamask/approval-controller": "3.5.0-preview.19a3a61",
  "@metamask/assets-controllers": "11.0.1-preview.19a3a61",
  "@metamask/base-controller": "3.2.0-preview.19a3a61",
  "@metamask/composable-controller": "3.0.0-preview.19a3a61",
  "@metamask/controller-utils": "4.3.1-preview.19a3a61",
  "@metamask/ens-controller": "4.1.0-preview.19a3a61",
  "@metamask/gas-fee-controller": "6.1.1-preview.19a3a61",
  "@metamask/keyring-controller": "7.0.0-preview.19a3a61",
  "@metamask/logging-controller": "1.0.0-preview.19a3a61",
  "@metamask/message-manager": "7.3.0-preview.19a3a61",
  "@metamask/network-controller": "12.0.0-preview.19a3a61",
  "@metamask/notification-controller": "3.1.0-preview.19a3a61",
  "@metamask/permission-controller": "4.1.0-preview.19a3a61",
  "@metamask/phishing-controller": "6.0.0-preview.19a3a61",
  "@metamask/preferences-controller": "4.2.0-preview.19a3a61",
  "@metamask/rate-limit-controller": "3.0.0-preview.19a3a61",
  "@metamask/signature-controller": "5.3.0-preview.19a3a61",
  "@metamask/transaction-controller": "8.0.1-preview.19a3a61"
}

@adonesky1
Copy link
Contributor

Needs to be a breaking changelog entry indicating that SupportedTokenDetectionNetworks is now CAIP formatted

Comment on lines -129 to +140
NetworkId,
InfuraNetworkId,
NetworkType
> = {
[NetworkId.goerli]: NetworkType.goerli,
[NetworkId.sepolia]: NetworkType.sepolia,
[NetworkId.mainnet]: NetworkType.mainnet,
[NetworkId['linea-goerli']]: NetworkType['linea-goerli'],
[NetworkId['linea-mainnet']]: NetworkType['linea-mainnet'],
[InfuraNetworkId.goerli]: NetworkType.goerli,
[InfuraNetworkId.sepolia]: NetworkType.sepolia,
[InfuraNetworkId.mainnet]: NetworkType.mainnet,
[InfuraNetworkId['linea-goerli']]: NetworkType['linea-goerli'],
[InfuraNetworkId['linea-mainnet']]: NetworkType['linea-mainnet'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for this change? How does it relate to the CAIP-2 migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see the name just changed for the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. unrelated to CAIP, but wanted to help disambiguate NetworkId and ChainId more by this naming. Happy to revert though

@jiexi
Copy link
Contributor Author

jiexi commented Aug 14, 2023

Needs to be a breaking changelog entry indicating that SupportedTokenDetectionNetworks is now CAIP formatted

SupportedTokenDetectionNetworks isn't exported, but it is used by isTokenDetectionSupportedForNetwork which is exported. Will add an entry for that Nevermind, there was already a line for this

@jiexi jiexi changed the title Jl/mmp 901/caip 2 CAIP-2 Aug 14, 2023
@jiexi
Copy link
Contributor Author

jiexi commented Aug 14, 2023

@metamaskbot publish-preview

@github-actions
Copy link
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask/address-book-controller": "3.1.0-preview.b4d7927",
  "@metamask/announcement-controller": "4.0.0-preview.b4d7927",
  "@metamask/approval-controller": "3.5.0-preview.b4d7927",
  "@metamask/assets-controllers": "11.0.1-preview.b4d7927",
  "@metamask/base-controller": "3.2.0-preview.b4d7927",
  "@metamask/composable-controller": "3.0.0-preview.b4d7927",
  "@metamask/controller-utils": "4.3.1-preview.b4d7927",
  "@metamask/ens-controller": "4.1.0-preview.b4d7927",
  "@metamask/gas-fee-controller": "6.1.1-preview.b4d7927",
  "@metamask/keyring-controller": "7.1.0-preview.b4d7927",
  "@metamask/logging-controller": "1.0.0-preview.b4d7927",
  "@metamask/message-manager": "7.3.0-preview.b4d7927",
  "@metamask/network-controller": "12.1.0-preview.b4d7927",
  "@metamask/notification-controller": "3.1.0-preview.b4d7927",
  "@metamask/permission-controller": "4.1.0-preview.b4d7927",
  "@metamask/phishing-controller": "6.0.0-preview.b4d7927",
  "@metamask/preferences-controller": "4.2.0-preview.b4d7927",
  "@metamask/rate-limit-controller": "3.0.0-preview.b4d7927",
  "@metamask/signature-controller": "5.3.0-preview.b4d7927",
  "@metamask/transaction-controller": "8.0.1-preview.b4d7927"
}

@jiexi jiexi changed the title CAIP-2 CAIP-2 Chain ID Aug 15, 2023
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Did a pass over this. I noticed some questions you asked in code comments as I was going over this and it's possible I didn't catch all of them. I will do another pass later.

One thing that stood out to me, though, was that we are renaming chainId across various pieces of state and configuration, and I wonder whether that is a good idea, or whether it would just be enough to change the names of the types. My concern is that this would spawn a ton of changes across extension and mobile when upgrading to newer versions of these packages (changing the names of the types will spawn enough changes as it is). The more changes we have to make, the more we risk introducing a bug.

So, I'm curious about the reasoning/context. Was it just to hammer home the fact that this is a CAIP chain ID and not a "legacy" chain ID? Was it to ensure that consumers are using the correct chain ID when they upgrade? My understanding is that we are moving to a world in which chain ID always means CAIP-2 chain ID, so when that happens, eventually the "Caip" part of the name will become redundant. But, are we at a point where we can't guarantee that a chain ID is always a CAIP-2 chain ID?

@@ -34,15 +33,15 @@ export enum AddressType {
* AddressBookEntry representation
* @property address - Hex address of a recipient account
* @property name - Nickname associated with this address
* @property chainId - Chain id identifies the current chain
* @property caipChainId - CAIP chain ID identifies the current chain
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since we're already saying "identifies" I wonder if we can make this a little more succinct:

Suggested change
* @property caipChainId - CAIP chain ID identifies the current chain
* @property caipChainId - CAIP-2 identifier for the current chain

@@ -31,7 +31,7 @@
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/utils": "^6.2.0"
"@metamask/utils": "^7.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

@@ -30,7 +30,7 @@
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

@@ -43,7 +43,7 @@
"@metamask/network-controller": "workspace:^",
"@metamask/preferences-controller": "workspace:^",
"@metamask/rpc-errors": "^5.1.1",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

import { toHex } from './util';

/**
* Checks whether the given value is a valid CAIP chain ID string for Ethereum.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't a CAIP chain ID always a string? If so I wonder whether this would still be understandable:

Suggested change
* Checks whether the given value is a valid CAIP chain ID string for Ethereum.
* Checks whether the given value is a valid CAIP chain ID for Ethereum.

@@ -30,7 +30,7 @@
},
"dependencies": {
"@metamask/base-controller": "workspace:^",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

@@ -32,7 +32,7 @@
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

@@ -33,7 +33,7 @@
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/message-manager": "workspace:^",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

@@ -36,7 +36,7 @@
"@metamask/controller-utils": "workspace:^",
"@metamask/eth-query": "^3.0.1",
"@metamask/network-controller": "workspace:^",
"@metamask/utils": "^6.2.0",
"@metamask/utils": "^7.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we note this in the changelog?

const key = `${transaction.nonce}-${
chainId ? convertHexToDecimal(chainId) : networkID
}-${new Date(time).toDateString()}`;
const networkChainId = caipChainId || networkID; // is this right?
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to keep the same behavior and convert the CAIP chain ID to decimal so that keys for existing transactions are the same. Otherwise transactions that are already in state might get accidentally dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Side note: it might be good to ask these questions in GitHub comments rather than code comments so they don't get accidentally committed.)

@jiexi
Copy link
Contributor Author

jiexi commented Aug 28, 2023

Postponed. Not sure if/when will be picked back up.

@jiexi jiexi closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants