Skip to content

Add MAX_SAFE_CHAIN_ID and refactor chain ID validation#10224

Merged
rekmarks merged 5 commits intodevelopfrom
max-safe-chain-id
Jan 20, 2021
Merged

Add MAX_SAFE_CHAIN_ID and refactor chain ID validation#10224
rekmarks merged 5 commits intodevelopfrom
max-safe-chain-id

Conversation

@rekmarks
Copy link
Copy Markdown
Member

  • Adds the MAX_SAFE_CHAIN_ID constant to shared/constants
  • Adds shared/modules/utils.js
    • Add isSafeChainId function to this file
    • Move isPrefixedFormattedHexString to this file
  • Add isSafeChainId validation to UI chain ID validation
  • Add hex string and isSafeChainId validation calls to network controller
    • We were previously only validating in the UI. setRpcTarget is called in a number of places, and for the safety of our users, we should ensure they can't switch to networks without valid chain IDs.
    • We're probably safe with just the validation in the UI, but the checks are cheap, so why not?

Adds chainId validation to NetworkController.setRpcTarget.
Currently we validate custom network chainIds in the UI, but we would do
well to add some redundancy in the background.

Also updates some network controller error messages.
@rekmarks rekmarks requested a review from a team as a code owner January 20, 2021 22:14
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [b150a5a]
Page Load Metrics (537 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint397852105
domContentLoaded34570753614067
load34671153714067
domInteractive34470653514067

Copy link
Copy Markdown
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM

@rekmarks rekmarks merged commit 4fef2b7 into develop Jan 20, 2021
@rekmarks rekmarks deleted the max-safe-chain-id branch January 20, 2021 23:37
@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2021
sufficientBalance,
isPrefixedFormattedHexString,
} from '../../../app/scripts/lib/util'
import { isPrefixedFormattedHexString } from '../../../shared/modules/utils'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: These tests should be moved to a different file 😅

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants