-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
chore(26921): bump @metamask/address-book-controller
to 6.0.0
#27107
base: develop
Are you sure you want to change the base?
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. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/[email protected] |
1cd2fed
to
b545ec1
Compare
@metamask/address-book-controller
to 6.0.0@metamask/address-book-controller
to 6.0.0
6ffe4c0
to
fea045f
Compare
@metamaskbot update-policies |
Policies updated |
@metamask/address-book-controller
to 6.0.0@metamask/address-book-controller
to 6.0.0
1e69873
to
e88a1d4
Compare
@metamaskbot update-policies |
Policies updated |
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #27107 +/- ##
========================================
Coverage 70.02% 70.02%
========================================
Files 1443 1443
Lines 50162 50165 +3
Branches 14039 14039
========================================
+ Hits 35124 35127 +3
Misses 15038 15038 ☔ View full report in Codecov by Sentry. |
Builds ready [a79d0b0]
Page Load Metrics (1700 ± 96 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
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
- Manually tested using steps in PR description
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.
It seems that we can update the as any
type assertion with Hex
in app/scripts/lib/AddressBookPetnamesBridge.ts.
@NiranjanaBinoy thanks for reviewing ! good catch !! The fix can be found here |
@metamaskbot update-policies |
@metamaskbot update-policies |
No policy changes |
1 similar comment
No policy changes |
@DDDDDanica, there are a couple more locations where the type assertion with |
@NiranjanaBinoy Hey good call ! fixed here |
Builds ready [f1411ab]
Page Load Metrics (1682 ± 60 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
// TODO: Replace `any` with type | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
entry.variation as any, | ||
entry.value as string, |
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.
entry.value as string, | |
entry.value, |
same as above
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
entry.variation as any, | ||
entry.value as string, | ||
entry.name as string, |
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.
entry.name as string, | |
entry.name, |
Type of entry.name
is already narrowed to string
in the type PetnameEntry
// TODO: Replace `any` with type | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const chainEntries = state.addressBook[chainId as any]; | ||
const chainEntries = state.addressBook[chainId as Hex]; |
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.
const chainEntries = state.addressBook[chainId as Hex]; | |
if(!isStrictHexString(chainId)){ | |
continue | |
} | |
const chainEntries = state.addressBook[chainId]; |
We can avoid the type assertions if we add a type guarding check for chainId as Hex.
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.
Good idea, captured here c1fe3e
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.
const chainEntries = state.addressBook[chainId as Hex]; | |
const chainEntries = state.addressBook[chainId]; |
we don't need remove this type assertion after adding the type guarding
c1fe3ed
to
c4d9e3f
Compare
// TODO: Replace `any` with type | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const entry = state.addressBook[chainId as any][address]; | ||
if (!isStrictHexString(chainId)) { |
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.
We need to add this type guarding outside this for loop.
We need to call it above const chainEntries = state.addressBook[chainId];
hi @NiranjanaBinoy thanks for reviewing, the types should all be fixed by now 🙏🏻 |
17bd20f
to
9190cf6
Compare
Description
Address-book is refactored to baseControllerV2, see more details in Changelog
Related issues
Fixes: #26921
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist