fix: map the supported block explorers #25908
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. |
legobeat
changed the title
Builds ready [af53459]
Page Load Metrics (169 ± 199 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25908 +/- ##
===========================================
- Coverage 69.67% 69.67% -0.00%
===========================================
Files 1402 1403 +1
Lines 49652 49657 +5
Branches 13720 13720
===========================================
+ Hits 34594 34597 +3
- Misses 15058 15060 +2 ☔ View full report in Codecov by Sentry. |
| export const SUPPORTED_BLOCK_EXPLORERS = { | ||
| // ETHEREUM | ||
| [CHAIN_IDS.MAINNET]: { | ||
| url: 'https://etherscan.io', | ||
| name: 'Etherscan', | ||
| }, | ||
| // OPTIMISM | ||
| [CHAIN_IDS.OPTIMISM]: { | ||
| url: 'https://optimistic.etherscan.io', | ||
| name: 'Optimistic Etherscan', | ||
| }, | ||
| // BSC | ||
| [CHAIN_IDS.BSC]: { | ||
| url: 'https://bscscan.com', | ||
| name: 'BscScan', | ||
| }, | ||
| // POLYGON | ||
| [CHAIN_IDS.POLYGON]: { | ||
| url: 'https://polygonscan.com', | ||
| name: 'PolygonScan', | ||
| }, | ||
| // ARBITRUM | ||
| [CHAIN_IDS.ARBITRUM]: { | ||
| url: 'https://arbiscan.io', | ||
| name: 'Arbiscan', | ||
| }, | ||
| // AVALANCHE | ||
| [CHAIN_IDS.AVALANCHE]: { | ||
| url: 'https://snowtrace.io', | ||
| name: 'Snowtrace', | ||
| }, | ||
| // LINEA | ||
| [CHAIN_IDS.LINEA_MAINNET]: { | ||
| url: 'https://lineascan.build', | ||
| name: 'LineaScan', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
NOTE these will be ported to shared libraries.
Can we make a related PR in shared libraries?
There was a problem hiding this comment.
Sure. Ihave opened a ticket for us. However, I think it might be more practical to close the ticket like this for now
| defaultNetwork?.rpcPrefs?.blockExplorerUrl ?? blockExplorerConfig?.url; | ||
|
|
||
| const blockExplorerButtonText = blockExplorerConfig?.name | ||
| ? `${t('notificationItemViewOn')} ${blockExplorerConfig.name}` |
There was a problem hiding this comment.
This doesn't work with all languages. Instead use the I18n insert values.
View on $1
There was a problem hiding this comment.
I’m not sure it wouldn’t have worked, but I modified it anyway 👍
There was a problem hiding this comment.
Yeee my bad, it would've been fine in english. Other languages might have placed the text in the wrong area :)
There was a problem hiding this comment.
That's a good point!
|
What does "supported block explorer" mean in this context? We should support any block explorer that complies with https://github.com/ethereum/EIPs/blob/master/EIPS/eip-3091.md |
|
@Gudahtt good point, we should support any block explorer. @matteoscurati correct if wrong, but this fix is specifically for the |
|
We have a shared library for generating block explorer links already, which should support EIP-3091 as well: https://github.com/MetaMask/etherscan-link (apologies for the misleading name, EIP-3091 support was added after it was named) It has separate functions for Etherscan versus custom because EIP-3091 didn't support all Etherscan pages we link to. |
|
Probably the chosen name doesn’t help. Context: the object is meant to describe the block explorers of the chains currently supported by Notifications. This PR is intended as just a FIX for some bugs reported in the extension v12 release. As @Prithpal-Sooriya mentioned, the goal is to bring this change into the shared libraries that we will release (and that we are already using on mobile) instead of the controllers directly inserted into the extension repo. @Gudahtt , I’ll check the library you linked right away, thanks 👍 |
|
@Gudahtt , maybe I'm wrong, but I don’t see a method in the library you linked to also get the name of the block explorer (e.g., “BscScan”, etc.). It’s not a big problem; we can leave a generic “View it on the Block Explorer” in the UI, but maybe you know of a better solution :) |
|
@Gudahtt OOOH this would be perfect! Unfortunately I only see a limited set of chains supported. I would love this to be the SoT for the growing list of networks we will support |
|
Ok, for now, as a temporary fix for the reported bugs, I can change the name of the object. Next steps:
|
|
That function is only used for etherscan links. Not for custom block explorer links. There is no need to hard code as anything chain specific |
|
@Gudahtt ok that sucks, we'll keep as is then (we have many chains and domains to support) - especially since this is to fix v12.0.0+. We will refactor this into either the existing library you posted, or have this inside our notification controllers Edit: sadly |
|
To be precise, before this PR, we used this: Two problems:
|
...hain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx
Outdated
Show resolved
Hide resolved
app/_locales/en/messages.json
Outdated
| @@ -3397,6 +3397,9 @@ | |||
| "notificationItemUnStakingRequested": { | |||
| "message": "Unstaking requested" | |||
| }, | |||
| "notificationItemViewOn": { | |||
| "message": "View on $1" | |||
There was a problem hiding this comment.
Could we rename notificationTransactionSuccessView here instead of defining an additional, identical localized message? Or is there some difference between these?
If we do need a separate message for some reason, we should include a description entry to explain what the message is for and what the $1 means
Got it, OK. To clarify, what I meant to suggest originally is that we fix both issues you highlighted, which would include adding Arbitrum. It's a little unclear to me why changing this existing constant would be significantly more work, but I'm not too familiar with it. Coming back to this later sounds OK to me if you think it's easier. |
…S and improve type inference
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new object to map supported block explorers for better notification details.
app/_locales/en/messages.json: AddednotificationItemViewOnkey for dynamic block explorer names in notifications.ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx: ReplacednativeBlockExplorerUrlwithblockExplorerConfigand addedgetBlockExplorerButtonTextfunction.ui/helpers/constants/metamask-notifications/metamask-notifications.ts: IntroducedSUPPORTED_NOTIFICATION_BLOCK_EXPLORERSobject andBlockExplorerConfigtype.ui/helpers/utils/notification.util.ts: Updated logic to useSUPPORTED_NOTIFICATION_BLOCK_EXPLORERSand addedisKeytype guard function.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR introduces a new object to map supported block explorers for better notification details.
app/_locales/en/messages.json: RemovednotificationItemViewOnand updatednotificationTransactionSuccessViewdescription.ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx: Modified button text to usenotificationTransactionSuccessViewfor accurate block explorer descriptions.
These changes aim to fix issues with incorrect or missing block explorer links in notifications.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
matteoscurati
dismissed stale reviews from Gudahtt and Prithpal-Sooriya
via
ff2f9a6
|
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR introduces a new object to map supported block explorers for better notification details.
app/_locales/*/messages.json: UpdatednotificationTransactionSuccessViewdescriptions across multiple locales to clarify the inclusion of block explorer URLs.ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx: Modified to use the new block explorer mapping for accurate URLs.ui/pages/notifications/notification-components/erc20-sent-received/erc20-sent-received.tsx: Integrated the new block explorer mapping for ERC20 notifications.ui/pages/notifications/notification-components/swap-completed/swap-completed.tsx: Updated to utilize the new block explorer mapping for swap completion notifications.
These changes aim to fix issues with incorrect or missing block explorer links in notifications.
14 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR introduces a new object to map supported block explorers for better notification details.
app/scripts/controllers/user-storage/user-storage-controller.ts: ModifiedisProfileSyncingEnabledto allownullvalue for uninitialized state.app/scripts/migrations/120.1.ts: Added migration script to setisProfileSyncingEnabledtonulland initializeUserStorageControllerstate.test/e2e/tests/confirmations/signatures/siwe.spec.ts: Introduced E2E tests for Sign-In with Ethereum (SIWE) feature.test/e2e/webdriver/index.js: AddedconstrainWindowSizeparameter for controlled WebDriver test environments.ui/components/app/assets/auto-detect-nft/auto-detect-nft-modal.tsx: Updated modal footer button properties for improved UI.
16 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
metamaskbot
added
the
release-12.3.0
Builds ready [c89757b]
Page Load Metrics (149 ± 150 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Description
This PR introduces a new object to map the block explorers needed to display certain details of the notifications. The created object is intentionally as simple as possible. The goal of the notifications team is to define a more complete object in a shared library among the different clients to correctly render notifications where necessary.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist