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

Update chain ID format #1367

Merged
merged 5 commits into from
May 18, 2023
Merged

Update chain ID format #1367

merged 5 commits into from
May 18, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 12, 2023

Description

The chain ID is now formatted internally as a 0x-prefixed hex string. This was chosen as the internal format for these reasons:

  • It allows arbitrarily large chain IDs, unlike number
  • It can be validated at runtime, unlike decimal strings
  • It can be disambiguated from decimal strings and numbers at compile-time using the Hex type, unlike decimal strings
  • It is serializable as JSON, unlike BigInt

However, to improve readability we will use the toHex helper function rather than string literals, letting us read chain IDs in the source code as decimal numbers or strings.

Changes

@metamask/address-book-controller

  • BREAKING: The addressBook state property is now keyed by Hex chain ID rather than string, and the chainId property of each address book entry is also Hex rather than string.
    • This requires a state migration
  • BREAKING: The methods delete and set now except the chain ID as Hex rather than as a decimal string
  • CHANGED: Add @metamask/utils dependency

@metamask/assets-controllers

  • BREAKING: Update @metamask/network-controller peerDependency
  • BREAKING: Change format of chain ID in state to 0x-prefixed hex string
    • The functions isTokenDetectionSupportedForNetwork and formatIconUrlWithProxy now expect a chain ID as type Hex rather than as a decimal string
    • The assets contract controller now expects the chainId configuration entry and constructor parameter as type Hex rather than decimal string
    • The NFT controller now expects the chainId configuration entry and constructor parameter as type Hex rather than decimal string
    • The NFT controller methods addNft, checkAndUpdateSingleNftOwnershipStatus, findNftByAddressAndTokenId, updateNft, and resetNftTransactionStatusByTransactionId now expect the chain ID to be type Hex rather than a decimal string
    • The NFT controller state properties allNftContracts and allNfts are now keyed by address and Hex chain ID, rather than by address and decimal string chain ID
      • This requires a state migration
    • The NFT detection controller now expects the chainId configuration entry and constructor parameter as type Hex rather than decimal string
    • The token detection controller now expects the chainId configuration entry as type Hex rather than decimal string
    • The token list controller now expects the chainId constructor parameter as type Hex rather than decimal string
    • The token list controller state property tokensChainsCache is now keyed by Hex chain ID rather than by decimal string chain ID.
      • This requires a state migration
    • The token rates controller now expects the chainId configuration entry and constructor parameter as type Hex rather than decimal string
    • The token rates controller chainId setter now expects the chain ID as Hex rather than as a decimal string
    • The tokens controller now expects the chainId configuration entry and constructor parameter as type Hex rather than decimal string
    • The tokens controller addDetectedTokens method now accepts the chainId property of the detectionDetails parameter to be of type Hex rather than decimal string.
    • The tokens controller state properties allTokens, allIgnoredTokens, and allDetectedTokens are now keyed by chain ID in Hex format rather than decimal string.
      • This requires a state migration

@metamask/controller-utils

  • BREAKING: The isSafeChainId parameter chainId is now type Hex rather than number
  • BREAKING: The ChainId enum and the GANACHE_CHAIN_ID constant are now formatted as 0x-prefixed hex strings rather than as decimal strings.

@metamask/ens-controller

  • BREAKING: Update @metamask/network-controller peerDependency
  • BREAKING: The ensEntries state property is now keyed by Hex chain ID rather than string, and the chainId property of each ENS entry is also Hex rather than string.
    • This requires a state migration
  • BREAKING: The methods get, set, and delete have been updated to accept and return chain IDs as 0x-prefixed hex strings, rather than decimal strings.

@metamask/gas-fee-controller

  • BREAKING: Update @metamask/network-controller peerDependency
  • BREAKING: The getChainId constructor parameter now expects a Hex return type rather than a decimal string
  • CHANGED: Add @metamask/utils dependency

@metamask/message-manager

  • BREAKING: The getCurrentChainId constructor parameter for each message manager now expects a Hex return type rather than a decimal string
    • Note that while every message manager class accepts this as a constructor parameter, it's only used by the TypedMessageManager at the moment.

@metamask/network-controller

  • BREAKING: The providerConfig type and state property have changed. The chainId property is now a 0x-prefixed hex string rather than a decimal string.
    • This requires a state migration
    • This affects the return value of the NetworkController:getProviderConfig and NetworkController:getState actions.
  • BREAKING: The NetworkConfiguration type and the networkConfigurations state property have changed. The chainId property on each configuration is now a 0x-prefixed hex string rather than a decimal string.
    • This requires a state migration
    • This change affects the upsertNetworkConfiguration method, which takes a network configuration as the first parameter
    • This affects the return value of the NetworkController:getState action

@metamask/signature-controller

  • BREAKING: The constructor option getCurrentChainId now expects a Hex return value rather than string
  • CHANGED: Add @metamask/utils dependency

@metamask/transaction-controller

  • BREAKING: Update @metamask/network-controller dependency
    • This affects the getNetworkState and onNetworkStateChange constructor parameters
  • BREAKING: Change format of chain ID in state to 0x-prefixed hex string
    • The chainId property of the Transaction type has been changed from a number to a 0x-prefixed hex string
    • The chainId property of the TransactionMeta type has been changed from a decimal string to a 0x-prefixed hex string, and the transaction property has been updated along with the Transaction type (as described above).
    • The state property transactions is an array of TransactionMeta objects, so it has changed according to the description above.
      • This requires a state migration: each entry should have the chainId property converted from a decimal string to a 0x-prefixed hex string, and the transaction.chainId property changed from a number to a 0x-prefixed hex string.
    • The addTransaction and estimateGas methods now expect the first parameter (transaction) to use the 0x-prefixed hex string format for the chainId property.
    • The updateTransaction method now expects the transactionMeta parameter to use the 0x-prefixed hex string format for the chainId property (and for the nested transaction.chainId property)
  • CHANGED: Add @metamask/utils dependency

References

Closes #1209

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@Gudahtt Gudahtt force-pushed the update-chain-id-format branch 11 times, most recently from 3bf31ba to 362dea8 Compare May 17, 2023 17:04
@Gudahtt Gudahtt marked this pull request as ready for review May 17, 2023 17:40
@Gudahtt Gudahtt requested a review from a team as a code owner May 17, 2023 17:40
@mcmire
Copy link
Contributor

mcmire commented May 17, 2023

Suggestions for the changelog:

@metamask/address-book-controller

  • The addressBook state property is not only keyed by Hex chain ID rather than string, but also each address book entry has a Hex chain ID instead of a string — not sure if that's worth pointing out

@metamask/assets-controller

  • NftController: Is addNft affected?

@metamask/ens-controller

  • Similar to AddressBookController — the entries have a Hex chainId as well as being keyed by them

@metamask/transaction-controller

  • The signature of the sign configuration option changed — the transaction argument must contain a Hex chain ID

packages/controller-utils/src/constants.ts Show resolved Hide resolved
packages/gas-fee-controller/src/GasFeeController.test.ts Outdated Show resolved Hide resolved
@@ -93,7 +94,7 @@ export function validateTypedSignMessageDataV1(
*/
export function validateTypedSignMessageDataV3V4(
messageData: TypedMessageParams,
currentChainId: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this still be undefined? I see this in TypedMessageManager.addUnapprovedMessageAsync:

      const currentChainId = this.getCurrentChainId?.();
      validateTypedSignMessageDataV3V4(messageParams, currentChainId);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! It's declared here as optional though.

Copy link
Contributor

@mcmire mcmire May 18, 2023

Choose a reason for hiding this comment

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

Undefined and optional are the same now, but I'm wondering whether they will be the same when we enable exactOptionalPropertyTypes (assuming we want to revive this PR).

That said, maybe it's the caller's responsibility to not pass this argument if it doesn't need to, so your change makes sense. Never mind!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. yeah OK, I can switch it back to | undefined instead to make that switch easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 4407595

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, maybe it's the caller's responsibility to not pass this argument if it doesn't need to, so your change makes sense. Never mind!

This was my first thought as well, but in this case it seems more ergonomic to always expect this to be passed in IMO.

mcmire
mcmire previously approved these changes May 17, 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.

Looks good, other than the suggestions I made for the changelog.

@Gudahtt
Copy link
Member Author

Gudahtt commented May 18, 2023

Great catch on the address book/ens entries, I have updated the changelog accordingly.

NftController: Is addNft affected?

Yes, it's affected via the fourth parameter, which contains a chain ID

The signature of the sign configuration option changed — the transaction argument must contain a Hex chain ID

I thought the same thing at first, but it's not affected. That accepts a TypedTransaction, which is a union of various types from within @ethereumjs/tx. One of them is called Transaction but it's different than the Transaction type we edited here.

That type has always accepted a 0x-prefixed hex string, so no change was needed.

Gudahtt and others added 4 commits May 18, 2023 10:47
The chain ID is now formatted internally as a `0x`-prefixed hex string.
This was chosen as the internal format for these reasons:
* It allows arbitrarily large chain IDs, unlike `number`
* It can be validated at runtime, unlike decimal strings
* It can be disambiguated from decimal strings and numbers at compile-
time using the `Hex` type, unlike decimal strings
* It is serializable as JSON, unlike BigInt

However, to improve readability we will use the `toHex` helper function
rather than string literals, letting us read chain IDs in the source
code as decimal numbers or strings.

Closes #1209
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
mcmire
mcmire previously approved these changes May 18, 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.

Looks good!

The `chainId` parameter is now `Hex | undefined` rather than an
optional `Hex`, to make it more compatible with the
`exactOptionalPropertyTypes` option (which we plan on enabling in the
future).
@Gudahtt Gudahtt merged commit 66e8232 into main May 18, 2023
@Gudahtt Gudahtt deleted the update-chain-id-format branch May 18, 2023 17:20
brad-decker pushed a commit to dragana8/core that referenced this pull request Jun 1, 2023
The chain ID is now formatted internally as a `0x`-prefixed hex string.
This was chosen as the internal format for these reasons:
* It allows arbitrarily large chain IDs, unlike `number`
* It can be validated at runtime, unlike decimal strings
* It can be disambiguated from decimal strings and numbers at compile-
time using the `Hex` type, unlike decimal strings
* It is serializable as JSON, unlike BigInt

However, to improve readability we will use the `toHex` helper function
rather than string literals, letting us read chain IDs in the source
code as decimal numbers or strings.

Closes MetaMask#1209

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The chain ID is now formatted internally as a `0x`-prefixed hex string.
This was chosen as the internal format for these reasons:
* It allows arbitrarily large chain IDs, unlike `number`
* It can be validated at runtime, unlike decimal strings
* It can be disambiguated from decimal strings and numbers at compile-
time using the `Hex` type, unlike decimal strings
* It is serializable as JSON, unlike BigInt

However, to improve readability we will use the `toHex` helper function
rather than string literals, letting us read chain IDs in the source
code as decimal numbers or strings.

Closes #1209

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
The chain ID is now formatted internally as a `0x`-prefixed hex string.
This was chosen as the internal format for these reasons:
* It allows arbitrarily large chain IDs, unlike `number`
* It can be validated at runtime, unlike decimal strings
* It can be disambiguated from decimal strings and numbers at compile-
time using the `Hex` type, unlike decimal strings
* It is serializable as JSON, unlike BigInt

However, to improve readability we will use the `toHex` helper function
rather than string literals, letting us read chain IDs in the source
code as decimal numbers or strings.

Closes #1209

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
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.

Update NetworkController to use 0x-prefixed hex string for chain ID
2 participants