Skip to content

fix: false warnings for HyperEVM#19167

Merged
adonesky1 merged 12 commits intomainfrom
fix/hyperevm-warnings
Sep 10, 2025
Merged

fix: false warnings for HyperEVM#19167
adonesky1 merged 12 commits intomainfrom
fix/hyperevm-warnings

Conversation

@Julink-eth
Copy link
Copy Markdown
Contributor

@Julink-eth Julink-eth commented Sep 3, 2025

Description

Currently there are warnings when adding the HyperEVM network on the name/symbol because it uses the chain ID 999 which is the same as WAN CHAIN, this PR is intented to deal with those warnings as an exception specific to HyperEVM to remove those warnings.

Changelog

CHANGELOG entry: Removed warnings when adding HyperEVM as a custom network

Related issues

NA

Manual testing steps

  1. Go to https://chainlist.org/
  2. Search for hyperevm
  3. Click on connect wallet
  4. It should ask you to approve adding the network with all the details about the network
  5. No warnings should be shown
  6. Then go to the Networks menu
  7. Click on the 3 dots menu next to HyperEvm and click on Edit
  8. In the popup there should not be any warnings
  9. Check the Tokens View with the network selected being HyperEVM
  10. There should not be a warning next to the HYPE Token

Screenshots/Recordings

NA

Before

image image

After

image image

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@Julink-eth Julink-eth requested review from a team as code owners September 3, 2025 12:29
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 3, 2025

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.

@github-actions github-actions Bot added the size-S label Sep 3, 2025
@Julink-eth Julink-eth added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Run E2E Smoke Test and removed Run E2E Smoke Test labels Sep 3, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 3, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: ae05ef7
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5d755b45-78f2-46c6-bfe9-91bd9d081503

Note

  • You can rerun any failed steps by opening the Bitrise build, tapping Rebuild on the upper right then Rebuild unsuccessful Workflows
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@github-actions github-actions Bot added size-M and removed size-S labels Sep 3, 2025
cursor[bot]

This comment was marked as outdated.

Battambang
Battambang previously approved these changes Sep 3, 2025
@vivek-consensys
Copy link
Copy Markdown
Contributor

Tested with no warnings.

iOS:
image

image

Android:
image

image

@vivek-consensys vivek-consensys added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Sep 4, 2025
@github-project-automation github-project-automation Bot moved this to Needs dev review in PR review queue Sep 5, 2025
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.03%. Comparing base (e624efc) to head (64924bf).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #19167      +/-   ##
==========================================
- Coverage   76.03%   76.03%   -0.01%     
==========================================
  Files        3201     3201              
  Lines       75664    75708      +44     
  Branches    13441    13440       -1     
==========================================
+ Hits        57532    57565      +33     
- Misses      14409    14421      +12     
+ Partials     3723     3722       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jiexi
Copy link
Copy Markdown
Member

jiexi commented Sep 8, 2025

In app/core/RPCMethods/networkChecker.util.ts, checkSafeNetwork gets the safe chainlist from https://chainid.network/chains.json, which is technically a list of chains, meaning that it's already possible for multiple networks to exist on the same chainId, BUT we are only matching against the FIRST chain in the list via .find() a few lines below.

What are your thoughts on modifying checkSafeNetwork to append a constant of whitelisted networks to the https://chainid.network/chains.json result and then use .some() instead of .find() to ensure we check all possible matching chains?

There are other places that make a request to https://chainid.network/chains.json. I imagine it is important for Hyper EVM to also be included in those places too

@Julink-eth
Copy link
Copy Markdown
Contributor Author

checkSafeNetwork

That's a good idea, that should work, I'll try this.

@Julink-eth
Copy link
Copy Markdown
Contributor Author

Julink-eth commented Sep 9, 2025

checkSafeNetwork

That's a good idea, that should work, I'll try this.

Actually I just tried but some returns a boolean when there is a match not the actual chain + for this to work I think we would have to do the check on more than just the chain ID (Or replace entirely the chain returned by the chainId website by the whitelisted one) because again, 999 is currently the chain ID of wanchain which is the chain that is being returned by https://chainid.network/chains.json

Let me know what you think

@jiexi
Copy link
Copy Markdown
Member

jiexi commented Sep 9, 2025

sorry to clarify, not only does .find() need to be replaced with .some(), but all the checks inside the if (matchedChain) { block also need to be included in body of the fn passed to .some(). This will mean it skip when it sees the wanchain because the name/rpc/token will not match, but will return true when it finds our manually inserted hyper evm entry. Does that make sense?

import axios from 'axios';
import { BannerAlertSeverity } from '../../component-library/components/Banners/Banner';
import { strings } from '../../../locales/i18n';
import { PopularList } from '../../util/networks/customNetworks';

import { toHex } from '@metamask/controller-utils';

export type Alert = {
  alertError: string;
  alertSeverity: BannerAlertSeverity;
  alertOrigin: string;
}

const findPopularNetwork = (rpcUrl: string, chainId: string) =>
  PopularList.some((network) => {
    const { origin } = new URL(network.rpcUrl);
    return origin === rpcUrl && network.chainId === chainId;
  });

const findPopularNetworkName = (name: string, chainId: string) =>
  PopularList.some(
    (network) =>
      network.nickname.toLowerCase() === name?.toLowerCase() &&
      network.chainId === chainId,
  );

const findPopularNetworkSymbol = (symbol: string, chainId: string) =>
  PopularList.some(
    (network) => network.ticker === symbol && network.chainId === chainId,
  );

const checkSafeNetwork = async (
  chainIdDecimal: string,
  rpcUrl: string,
  nickname: string,
  ticker: string,
): Promise<Alert[]> => {
  const EVM_NATIVE_TOKEN_DECIMALS = 18;

  const response = await axios.get('https://chainid.network/chains.json');
  const safeChainsList = response.data;

  const matchedChains = safeChainsList.filter(
    (chain: { chainId: number }) => chain.chainId.toString() === chainIdDecimal,
  );

  if (!matchedChains.length) {
    return [{
      alertError: strings('add_custom_network.unrecognized_chain_id'),
      alertSeverity: BannerAlertSeverity.Error,
      alertOrigin: 'unknown_chain',
    }]
  }

  const alertsForMatchedChains = matchedChains.map((matchedChain) => {
    const alerts = [];
    const { origin } = new URL(rpcUrl);
    if (
      !matchedChain.rpc
        ?.map((rpc: string) => new URL(rpc).origin)
        .includes(origin) &&
      !findPopularNetwork(origin, toHex(chainIdDecimal))
    ) {
      alerts.push({
        alertError: strings('add_custom_network.invalid_rpc_url'),
        alertSeverity: BannerAlertSeverity.Error,
        alertOrigin: 'rpc_url',
      });
    }
    if (matchedChain.nativeCurrency?.decimals !== EVM_NATIVE_TOKEN_DECIMALS) {
      alerts.push({
        alertError: strings('add_custom_network.invalid_chain_token_decimals'),
        alertSeverity: BannerAlertSeverity.Warning,
        alertOrigin: 'decimals',
      });
    }
    if (
      matchedChain.name?.toLowerCase() !== nickname?.toLowerCase() &&
      !findPopularNetworkName(nickname, toHex(chainIdDecimal))
    ) {
      alerts.push({
        alertError: strings('add_custom_network.unrecognized_chain_name'),
        alertSeverity: BannerAlertSeverity.Warning,
        alertOrigin: 'chain_name',
      });
    }
    if (
      matchedChain.nativeCurrency?.symbol !== ticker &&
      !findPopularNetworkSymbol(ticker, toHex(chainIdDecimal))
    ) {
      alerts.push({
        alertError: strings('add_custom_network.unrecognized_chain_ticker'),
        alertSeverity: BannerAlertSeverity.Warning,
        alertOrigin: 'chain_ticker',
      });
    }

    return alerts;
  })

  // less alerts means a better match
  const bestMatchedAlerts = alertsForMatchedChains.reduce((best, current) => {
    return current.length < best.length ? current : best;
  }, alertsForMatchedChains[0])

  return bestMatchedAlerts;
};

export default checkSafeNetwork;

something like this?

Comment thread app/components/Views/Settings/NetworksSettings/NetworkSettings/index.test.tsx Outdated
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Sep 9, 2025

@github-project-automation github-project-automation Bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue Sep 9, 2025
Copy link
Copy Markdown
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Ideally I'd love for us to find a more durable pattern than a whitelist but this looks fine for now. Do we need to do this on extension too? And if so should we have a consolidated list shared across both clients?

@adonesky1 adonesky1 added this pull request to the merge queue Sep 10, 2025
Merged via the queue into main with commit 18fb4a0 Sep 10, 2025
87 checks passed
@adonesky1 adonesky1 deleted the fix/hyperevm-warnings branch September 10, 2025 15:20
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 10, 2025
@metamaskbot metamaskbot added the release-7.56.0 Issue or pull request that will be included in release 7.56.0 label Sep 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed QA testing has been completed and passed release-7.56.0 Issue or pull request that will be included in release 7.56.0 size-M team-network-enablement

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants