-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fix: notification detail network fee broke application #25315
fix: notification detail network fee broke application #25315
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. |
@@ -27,7 +27,7 @@ export const NotificationDetailBlockExplorerButton = ({ | |||
|
|||
const chainIdHex = decimalToHex(chainId); | |||
const { nativeBlockExplorerUrl } = getNetworkDetailsByChainId( | |||
`0x${chainId}` as keyof typeof CHAIN_IDS, | |||
`0x${chainIdHex}` as keyof typeof CHAIN_IDS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was not using correct input.
I blame the type assertions - will fix and remove these type assertions in a future PR.
NETWORK_TO_NAME_MAP[chainId as keyof typeof NETWORK_TO_NAME_MAP] ?? ''; | ||
const nativeCurrencyName = fullNativeCurrencyName.split(' ')[0] ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safeguards to prevent the .split
issue.
I do want us to revisit this function in a future PR to make sure all other mappers are safe too.
|
||
// If rpc is found, return its URL. Otherwise, return a default URL based on the chainId. | ||
if (rpc) { | ||
return rpc.rpcUrl; | ||
} | ||
// Fallback RPC URLs based on the chainId | ||
switch (chainId) { | ||
case 'MAINNET': | ||
case CHAIN_IDS.MAINNET: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue! Above we gave a bad/false type of keyof typeof CHAIN_IDS
which is the keys. We actually want to test against the values/hex numbers of the chains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25315 +/- ##
========================================
Coverage 65.69% 65.70%
========================================
Files 1377 1377
Lines 54624 54622 -2
Branches 14314 14316 +2
========================================
+ Hits 35884 35885 +1
+ Misses 18740 18737 -3 ☔ View full report in Codecov by Sentry. |
I will add code coverage in a followup PR that tidies up types and functions. |
Builds ready [bf75bbd]
Page Load Metrics (46 ± 3 ms)
Bundle size diffs
|
Description
Yeah, due to some type assertions (evil), we did not receive a correct value expected. Due to this we ended up performing a
.split
on an undefined... then crash.This adds some safer logic and also fixes the area that call the function with the wrong inputs.
A separate PR will be ready that fixes these bad type assertions.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist