Handle watch asset accept and reject using ApprovalController only #18829
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. |
Builds ready [f5af643]
Page Load Metrics (1493 ± 37 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [8228ba3]
Page Load Metrics (1624 ± 65 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| res.result = true; | ||
| return end(); | ||
| } catch (error) { | ||
| if (error.message === 'User rejected to watch the asset.') { | ||
| return end(ethErrors.provider.userRejectedRequest()); |
There was a problem hiding this comment.
This is no longer needed, since that's the error we are defining in the FE component for rejection.
Builds ready [4de3dde]
Page Load Metrics (1571 ± 30 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| @@ -13,7 +14,10 @@ import { getTokens } from '../../ducks/metamask/metamask'; | |||
| import ZENDESK_URLS from '../../helpers/constants/zendesk-url'; | |||
| import { isEqualCaseInsensitive } from '../../../shared/modules/string-utils'; | |||
| import { getSuggestedAssets } from '../../selectors'; | |||
There was a problem hiding this comment.
Presumably you'll have to repoint this selector since this state will be different as it no longer lives on the TokensController?
There was a problem hiding this comment.
Good catch, we have a new selector that uses approvals filtered by type. It'll definitely need to be used here.
Builds ready [7c3dac9]
Page Load Metrics (1575 ± 40 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov Report
@@ Coverage Diff @@
## develop #18829 +/- ##
===========================================
+ Coverage 69.94% 69.98% +0.04%
===========================================
Files 959 959
Lines 36822 36790 -32
Branches 9455 9452 -3
===========================================
- Hits 25753 25747 -6
+ Misses 11069 11043 -26
|
bergarces
changed the title
There was a problem hiding this comment.
Added basic E2E tests for the wallet_watchAsset RPC method.
bergarces
force-pushed
the
token-approval-promise-refactor-full
branch
from
2affd0b
to
6c006e6
Compare
There was a problem hiding this comment.
Changes from this patch are already included in the new version.
| @@ -504,7 +504,7 @@ export function getCurrentCurrency(state) { | |||
| export function getTotalUnapprovedCount(state) { | |||
| const { pendingApprovalCount = 0 } = state.metamask; | |||
|
|
|||
| return pendingApprovalCount + getSuggestedAssetCount(state); | |||
There was a problem hiding this comment.
suggested assets already were part of pending approvals from a previous PR, this can be removed.
Builds ready [723a04c]
Page Load Metrics (1734 ± 67 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [d405b0f]
Page Load Metrics (1725 ± 54 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| suggestedAssetID: string, | ||
| ): ThunkAction<void, MetaMaskReduxState, unknown, AnyAction> { | ||
| return async (dispatch: MetaMaskReduxDispatch) => { | ||
| dispatch(showLoadingIndication()); |
There was a problem hiding this comment.
For slower connections, do we lose the loading indicator now?
If so, maybe it's worth looking at a separate ticket to add this into the resolvePendingApproval and rejectPendingApproval actions.
There was a problem hiding this comment.
Up until now, resolving or rejecting an approval would happen very fast.
Now that we have the expectsResult option though, we should probably do what you suggest and add the loading indication for resolving actions.
| @@ -289,7 +289,6 @@ function defaultFixture() { | |||
| allTokens: {}, | |||
| detectedTokens: [], | |||
| ignoredTokens: [], | |||
| suggestedAssets: [], | |||
There was a problem hiding this comment.
I think I mentioned on the core PR also, but do we have a policy to remove old unused user data using migrations, or leave it to linger?
There was a problem hiding this comment.
I'm going to ask in one of the dev channels.
There was a problem hiding this comment.
Turns out a migration to get rid of the state in existing installations would be desirable in this case.
bergarces
changed the title
bergarces
force-pushed
the
token-approval-promise-refactor-full
branch
from
5a48e68
to
bb8d9df
Compare
bergarces
force-pushed
the
token-approval-promise-refactor-full
branch
from
bb8d9df
to
0fa00cc
Compare
There was a problem hiding this comment.
Added migration 087. Based on migration 085, which has a similar use case.
Builds ready [ac3b5a6]
Page Load Metrics (1659 ± 58 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
There was a problem hiding this comment.
Patches to TokensController (at least in terms of taking care of MetaMask/core#1261 and MetaMask/core#1268) look good.
Explanation
TokensController, which makes it so that only acceptance or rejection of the approval are triggered from the FE. It will no longer callTokensController.Fixes https://github.com/MetaMask/MetaMask-planning/issues/463
Manual Testing Steps
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.