mirror of
https://github.com/kremalicious/metamask-extension.git
synced 2024-10-22 03:12:42 +02:00
Adding documentation for confirmation code cleanup (#17975)
This commit is contained in:
Jyoti Puri
GitHub
parent
13f4295287
commit
599bef9dc5
13
docs/confirmation-refactoring/README.md
Normal file
13
docs/confirmation-refactoring/README.md
Normal file
@ -0,0 +1,13 @@
|
||||
# Confirmation Pages Refactoring
|
||||
|
||||
The following pages document the ongoing refactoring efforts of confirmation pages. They describe the current (2023) code and proposed changes.
|
||||
|
||||
1. [Signature Request Pages](./signature-request/README.md)
|
||||
|
||||
2. [Confirmation Pages Routing](./confirmation-pages-routing/README.md)
|
||||
|
||||
3. [Confirmation Page Structure](./confirmation-page=structure/README.md)
|
||||
|
||||
4. [Confirmation State Management](./confirmation-state-management/README.md)
|
||||
|
||||
5. [Confirmation Backend Architecture](./confirmation-backend-architecture/README.md)
|
||||
@ -0,0 +1,32 @@
|
||||
# Confirmation Background Architecture and Code Cleanup
|
||||
|
||||
## Current Implementation:
|
||||
|
||||
Current confirmation implementation in the background consists of following pieces:
|
||||
|
||||
1. `TransactionController` and utility, helper classes used by it:
|
||||
`TransactionController` is very important piece in transaction processing. It is described [here](https://github.com/MetaMask/metamask-extension/tree/develop/app/scripts/controllers/transactions#transaction-controller). It consists of 4 important parts:
|
||||
- `txStateManager`: responsible for the state of a transaction and storing the transaction
|
||||
- `pendingTxTracker`: watching blocks for transactions to be include and emitting confirmed events
|
||||
- `txGasUtil`: gas calculations and safety buffering
|
||||
- `nonceTracker`: calculating nonces
|
||||
2. `MessageManagers`:
|
||||
There are 3 different message managers responsible for processing signature requests. These are detailed [here](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/signature-request#proposed-refactoring).
|
||||
3. `MetamaskController `:
|
||||
`MetamaskController ` is responsible for gluing together the different pieces in transaction processing. It is responsible to inject dependencies in `TransactionController`, `MessageManagers`, handling different events, responses to DAPP requests, etc.
|
||||
|
||||
## Areas of Code Cleanup:
|
||||
|
||||
1. Migrating to `@metamask/transaction-controller`. `TransactionController` in extension repo should eventually get replaced by core repo [TransactionController](https://github.com/MetaMask/core/tree/main/packages/transaction-controller). This controller is maintained by core team and also used in Metamask Mobile App.
|
||||
2. Migrating to `@metamask/message-manager`. Message Managers in extension repo should be deprecated in favour of core repo [MessageManagers](https://github.com/MetaMask/core/tree/main/packages/message-manager).
|
||||
3. Cleanup Code in `MetamaskController`. [Metamaskcontroller](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/metamask-controller.js) is where `TransactionController` and different `MessageManagers` are initialized. It is responsible for injecting required dependencies. Also, it is responsible for handling incoming DAPP requests and invoking appropriate methods in these background classes. Over the period of time lot of code that should have been part of `TransactionController` and `MessageManagers` has ended up in `MetamaskController`. We need to cleanup this code and move to the appropriate classes.
|
||||
- Code [here](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3097) to check if `eth_sign` is enabled in preferences and perform other validation on the incoming request should be part of [MessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/message-manager.js)
|
||||
- Method to sign messages [signMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3158), [signPersonalMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3217), [signTypedMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3470) can be simplified by injecting `KeyringController` into `MessageManagers`.
|
||||
- There are about 11 different methods to `add`, `approve`, `reject` different types of signature requests. These can probably be moved to a helper class, thus reducing lines of code from `MetamaskController `.
|
||||
- This [code](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L959) can better be placed in `TransactionController`.
|
||||
- A lot of other methods in `MetamaskController` which are related to `TransactionController` and the state of `TransactionController` can be moved into `TransactionController` itself like [method1](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L1179), [method2](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3570), [method3](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L4349), etc.
|
||||
|
||||
### Using ApprovalController for Confirmations
|
||||
|
||||
[ApprovalController](https://github.com/MetaMask/core/tree/main/packages/approval-controller) is written as a helper to `PermissionController`. Its role is to manage requests that require user approval. It can also be used in confirmation code to launch UI. Thus the use of `showUserConfirmation` function in `MetamaskController ` can be removed.
|
||||
But `ApprovalController` will need some changes to be able to use it for confirmations, for example, it does not support multiple parallel requests from the same origin.
|
||||
@ -0,0 +1,67 @@
|
||||
# Confirmation Pages Structure
|
||||
|
||||
### Current Implementation
|
||||
|
||||
Currently we have following confirmation pages mapping to confirmation routes:
|
||||
|
||||
1. `pages/confirm-deploy-contract`
|
||||
2. `pages/confirm-send-ether`
|
||||
3. `pages/confirm-send-token`
|
||||
4. `pages/confirm-approve`
|
||||
5. `pages/confirm-token-transaction-base`
|
||||
6. `pages/confirm-contract-interaction`
|
||||
|
||||
|
||||
|
||||
`confirm-page-container` component helps to define a structure for confirmation pages it includes:
|
||||
|
||||
1. `header`
|
||||
2. `content` - transaction details and tabs for hexdata and insights if available
|
||||
3. `footer`
|
||||
4. `warnings`
|
||||
|
||||
`confirm-transaction-base` component is responsible for checking transaction details and pass required details like `gas-details`, `hex-data`, etc and passing over to `confirm-page-container`.
|
||||
|
||||
Other confirmation components listed above map to different types of transactions and are responsible for passing over to `confirm-transaction-base` values / components specific to their transaction type. For instance, `confirm-deploy-contract` passes data section to `confirm-transaction-base`.
|
||||
|
||||
## Areas of Refactoring:
|
||||
|
||||
1. ### [confirm-transaction-base](https://github.com/MetaMask/metamask-extension/tree/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js) cleanup:
|
||||
The `confirm-transaction-base` component is huge 1200 lines component taking care of lot of complexity. We need to break it down into smaller components and move logic to hooks or utility classes. Layout related part can be moved to `confirm-page-container`.
|
||||
- Extract out code to render data into separate component from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L641).
|
||||
- Extract out component to render hex data from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L675).
|
||||
- Extract out code to render title [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L894) into separate component.
|
||||
- Extract out code to render sub-title [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L921). It should return null if hideSubtitle is true.
|
||||
- Extract out code to render gas details [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L444), this code can be used [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js#L171) and [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/send/gas-display/gas-display.js#L161) also.
|
||||
- Extract renderDetails from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L309) into a separate component. Function `setUserAcknowledgedGasMissing` can also be moved to it.
|
||||
- Code to get error key [getErrorKey](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L230) can be moved to a util function.
|
||||
- As new component for gas selection popups is created this code [handleEditGas, handleCloseEditGas](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L276) can be moved to it.
|
||||
- Convert `confirm-transaction-base` into a functional components and extract out all of these functions into a hook - `handleEdit`, `handleCancelAll`, `handleCancel`, `handleSubmit`, `handleSetApprovalForAll`, etc.
|
||||
2. ### [confirm-transaction-base-container](https://github.com/MetaMask/metamask-extension/tree/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js) cleanup:
|
||||
This container is doing much work to query and get required transaction related values from state and pass over to `confirm-transaction-base` component. As we refactor state we should get rid of this component.
|
||||
- remove the use of `state.confirmTransaction` from the component
|
||||
- create hook to get values derived from metamask state and active transaction.
|
||||
State cleanup is detailed more in a separate document [here](https://github.com/MetaMask/metamask-extension/tree/develop/docs/confirmation-refactoring/confirmation-state-management).
|
||||
3. ### [confirm-page-container](https://github.com/MetaMask/metamask-extension/tree/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container) cleanup:
|
||||
As described we should continue to have `confirm-page-container` components taking care of layout. Also wherever possible more re-usable smaller layout components for different part of confirmation page like gas details, gas selection popover, etc should be added.
|
||||
`confirm-page-container` defines a layout which is used by most comfirmation pages, but some pages like new token allowance implementation for `ERC20` differ from this layout. We will be able to use more and more of these re-usable components for other confirmation pages layouts also.
|
||||
- Move code specific to transaction to their confirmation component, for instance code related to `ApproveForAll` should be moved to `/pages/confirm-approve`, code related to `hideTitle` can be moved to `/pages/confirm-contract-interaction` etc.
|
||||
- All header related code [here](https://github.com/MetaMask/metamask-extension/blob/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container/confirm-page-container.component.js#L191) should be moved to [confirm-page-container-header](https://github.com/MetaMask/metamask-extension/tree/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container/confirm-page-container-header)
|
||||
- All warnings related code can be moved to a new child component.
|
||||
- Props passing to `confirm-page-component` should be reduced. A lot of passed props like `origin`, `supportEIP1559` can be obtained directly using selectors. Props passing from `confirm-page-container` down to its child components should also be reduced.
|
||||
4. ### Edit gas popovers:
|
||||
|
||||
There are 2 different versions popovers for gas editing:
|
||||
|
||||
- Legacy gas popover - [component](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-popover)
|
||||
- EIP-1559 V2 gas popover - [component1](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-fee-popover), [component2](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/advanced-gas-fee-popover).
|
||||
Context [transaction-modal-context](https://github.com/MetaMask/metamask-extension/blob/develop/ui/contexts/transaction-modal.js) is used to show hide EIP-1559 gas popovers.
|
||||
|
||||
A parent component can be created for gas editing popover which will wrap both the legacy and EIP-1559 gas popover. Depending on the type of transaction appropriate gas popover can be shown. `transaction-modal-context` can be used to take care to open/close both popovers.
|
||||
This parent component can be added to `confirm-transaction-base` and `token-allowance` components and thus will be available on all confirmation pages using gas editing.
|
||||
Code [handleEditGas, handleCloseEditGas](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L276) can be moved to this new component.
|
||||
|
||||
5. ### Gas polling
|
||||
Gas polling related code in `/pages/confirm-transaction` can be moved into a hook and included in `pages/confirm-transaction-base`, `/app/token-allowance` as only those confirmation pages need gas estimates.
|
||||
|
||||
**Note:** This document **does not cover signature request pages** which are covered separately.
|
||||
Binary file not shown.
|
After Width: | Height: | Size: 51 KiB |
@ -0,0 +1,67 @@
|
||||
# Refactoring - Confirmation pages routing
|
||||
|
||||
This document details how routing to confirmation pages is currently done and the proposed improvements in routing.
|
||||
|
||||
## Current flow
|
||||
|
||||
The current flow of routing to confirmation pages is un-necessarily complicated and have issues.
|
||||
|
||||
|
||||
|
||||
- There are 2 ways in which confirmation pages can be opened:
|
||||
1. User triggers send flow from within Metamask
|
||||
- If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to the **`/confirm-transaction`** route.
|
||||
2. DAPP sends request to Metamask
|
||||
- If DAPP sends request to Metamask an unapproved transaction or signature request is created in background and UI is triggered open (if it is not already open).
|
||||
- The router by default renders `pages/home` component. The component looks at the state and if it finds an unapproved transaction or signature request in state it re-routes to **`/confirm-transaction`**.
|
||||
- For **`/confirm-transaction/`** route, the router renders `pages/confirm-transaction` component.
|
||||
- For **`/confirm-transaction`** route `pages/confirm-transaction` component renders `pages/confirm-transaction-switch` by default, for transactions with token methods it renders `pages/confirm-transaction/confirm-token-transaction-switch` which also open `pages/confirm-transaction-switch` by default.
|
||||
- `pages/confirm-token-switch` redirect to specific confirmation page route depending on un-approved transaction or signature request in the state.
|
||||
- For specific route **`/confirm-transaction/${id}/XXXXX`** routes again `pages/confirm-transaction` is rendered.
|
||||
- Depending on confirmation route `pages/confirm-transaction` and `pages/confirm-transaction/confirm-token-transaction-switch` renders the specific confirmation page component.
|
||||
|
||||
## Proposed flow
|
||||
|
||||
The proposed routing of confirmation pages looks like.
|
||||
|
||||
|
||||
|
||||
- There are 2 ways in which confirmation pages can be opened:
|
||||
1. User triggers send flow from within Metamask
|
||||
- If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to a specific transaction route, **`/confirm-transaction/${id}/XXXX`**, depending on the transaction type.
|
||||
2. DAPP sends request to Metamask
|
||||
- If DAPP send request to Metamask an unapproved transaction or signature request is created in background and UI is triggered to open (if it is not already open).
|
||||
- Instead of rendering `pages/home`, `pages/routes` finds the unapproved transaction in state and reroutes to **`/confirm-transaction`**.
|
||||
- Router renders `pages/confirm-transaction` component for **`/confirm-transaction`** route.
|
||||
- `pages/confirm-transaction` component redirect to specific confirmation page route depending on unapproved transaction or signature request in the state.
|
||||
- Again for specific route **`/confirm-transaction/${id}/XXXXX`** `pages/confirm-transaction` is rendered, it in-turn renders appropriate confirmation page for the specific route.
|
||||
|
||||
## Current Route component mapping
|
||||
|
||||
| Route | Component |
|
||||
| ------------------------------------------------- | -------------------------------------- |
|
||||
| `/confirm-transaction/${id}/deploy-contract` | `pages/confirm-deploy-contract` |
|
||||
| `/confirm-transaction/${id}/send-ether` | `pages/confirm-send-ether` |
|
||||
| `/confirm-transaction/${id}/send-token` | `pages/confirm-send-token` |
|
||||
| `/confirm-transaction/${id}/approve` | `pages/confirm-approve` |
|
||||
| `/confirm-transaction/${id}/set-approval-for-all` | `pages/confirm-approve` |
|
||||
| `/confirm-transaction/${id}/transfer-from` | `pages/confirm-token-transaction-base` |
|
||||
| `/confirm-transaction/${id}/safe-transfer-from` | `pages/confirm-token-transaction-base` |
|
||||
| `/confirm-transaction/${id}/token-method` | `pages/confirm-contract-interaction` |
|
||||
| `/confirm-transaction/${id}/signature-request` | `pages/confirm-signature-request.js` |
|
||||
|
||||
## Areas of code refactoring
|
||||
|
||||
Current routing code is complicated, it is also currently tied to state change in confirmation pages that makes it more complicated. State refactoring as discussed in this [document](https://github.com/MetaMask/metamask-extension/tree/develop/docs/confirmation-refactoring/confirmation-state-management) will also help simplify it.
|
||||
|
||||
- Any re-usable routing related code should be moved to [useRouting](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/useRouting.js) hook.
|
||||
- Logic to initially check state and redirect to `/pages/confirm-transaction` can be moved from `/pages/home` to `pages/routes`
|
||||
- All the route mapping code should be moved to `/pages/confirm-transaction`, this will require getting rid of route mappings in `/pages/confirm-transaction/confirm-token-transaction-switch`, `/pages/confirm-transaction-switch`.
|
||||
- `/pages/confirm-transaction-switch` has the code that checks the un-approved transaction / message in the state, and based on its type and asset redirect to a specific route, a utility method can be created to do this mapping and can be included in `/pages/confirm-transaction` component.
|
||||
- During the send flow initiated within metamask user can be redirected to specific confirmations route **`/confirm-transaction/${id}/XXXX`**
|
||||
- Confirmation components have lot of props passing which needs to be reduced. Values can be obtained from redux state or other contexts directly using hooks. Component [confirm-token-transaction-switch](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-token-transaction-switch.js) has a lot of un-necessary props passing which should be removed and will help to further refactor routing.
|
||||
|
||||
- **Routing to mostRecentOverviewPage**
|
||||
Across confirmation pages there is code to re-direct to `mostRecentOverviewPage`. `mostRecentOverviewPage` is equal to default route `/` or `/asset` whichever was last opened.
|
||||
Also a lot of components check for state update and as soon as state has `0` pending un-approved transaction or signature request redirect is done to `mostRecentOverviewPage`. This logic can be handled at `/pages/confirm-transaction` which is always rendered for any confirmation page.
|
||||
Also when the transaction is completed / rejected redirect is done to `mostRecentOverviewPage` explicitly which we should continue to do.
|
||||
|
Before Width: | Height: | Size: 135 KiB After Width: | Height: | Size: 135 KiB |
|
Before Width: | Height: | Size: 89 KiB After Width: | Height: | Size: 89 KiB |
@ -0,0 +1,47 @@
|
||||
# Confirmation Pages - Frontend State Management
|
||||
|
||||
State Management is very important piece to keep frontend confirmation code simplified. Currently state management is fragmented over places and is complicated. Following guidelines will be useful for designing State Magagement:
|
||||
|
||||
1. Use state obtained from backend (redux store `state.metamask`) as single source of truth
|
||||
2. For state derived from the backend state hooks can be written, these will internally use backend state
|
||||
3. For temporary UI state shared across multiple components React Context can be used, minimise the scope of the context to just the components that need them (this is useful to avoid un-necessary re-rendering cycles in the app)
|
||||
4. Confirmation React components fall into 2 categories:
|
||||
- Smart components: state access should go here
|
||||
- Dumb components: they are used for layout mainly and should ideally have required state data passed to them via props
|
||||
5. Redux state is a good candidate for implementing state machine on frontend, if require anywhere in confirmation pages. Though currently transient state is mostly confined to single component state machine may not be needed.
|
||||
|
||||
Refactorings:
|
||||
|
||||
- There are confirmations related ducks [here](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks):
|
||||
- [confirm-transaction](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/confirm-transaction): this is redundant and we should be able to get rid of it.
|
||||
- [gas](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/gas): this is not used anywhere and can be removed.
|
||||
- [send](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/send): this duck is important state machine for send flow and we should continue to maintain.
|
||||
- [gasFeeContext](https://github.com/MetaMask/metamask-extension/blob/develop/ui/contexts/gasFee.js) is huge context written on top of [gasFeeInput](https://github.com/MetaMask/metamask-extension/tree/develop/ui/hooks/gasFeeInput) hook. The context / hook provides about 20 different values used in different places in confirmation pages. We need to break this down:
|
||||
|
||||
- Context is required only to provide temporary UI state for confirmation pages which includes:
|
||||
|
||||
- `transaction` - active transaction on confirmation pages
|
||||
- `editGasMode` - cancel, speedup, swap or default, this is also temporary UI state
|
||||
|
||||
The context can be included in `/pages/confirm-transaction-base` and around `TokenAllowance` in `/pages/confirm-approve`.
|
||||
|
||||
- Hooks can be created for values derived from values derived from above context and metamask state. This include:
|
||||
- `maxFeePerGas`
|
||||
- `maxPriorityFeePerGas`
|
||||
- `supportEIP1559`
|
||||
- `balanceError`
|
||||
- `minimumCostInHexWei`
|
||||
- `maximumCostInHexWei`
|
||||
- `hasSimulationError`
|
||||
- `estimateUsed`
|
||||
- Values which can be obtained from metamask state using selectors should be removed from this context. This includes:
|
||||
- `gasFeeEstimates`
|
||||
- `isNetworkBusy`
|
||||
- `minimumGasLimitDec` is a constant value 21000 should be removed from the context, this can be moved to constants file.
|
||||
- Create separate hook for transaction functions [here](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/gasFeeInput/useTransactionFunctions.js), this hook can consume GasFeeContext.
|
||||
- Setters and manual update functions are only used by legacy gas component [edit-gas-fee-popover](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-popover). This component uses useGasFeeInputs hook. We need to create a smaller hook just for this component using the above context and hooks.
|
||||
|
||||
* [confirm-transaction-base.container.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js) and [confirm-transaction-base.component.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js) has a lot of code to derive values from state and selected transactions. This can be simplified by using hooks that will he created.
|
||||
* We will have a lot of hooks for transaction related fields, these can be grouped into same file / folder.
|
||||
|
||||
As we work on the components we will be able to identify more areas of improvement.
|
||||
@ -6,35 +6,35 @@ This document details the plan to refactor and cleanup Signature Request pages i
|
||||
|
||||
1. Simple ETH Signature
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/eth_sign.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/eth_sign.png" width="150"/>
|
||||
|
||||
1. Personal Signature
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/personal_sign.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/personal_sign.png" width="150"/>
|
||||
|
||||
1. Typed Data - V1
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/v1.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/v1.png" width="150"/>
|
||||
|
||||
1. Typed Data - V3
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/v3.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/v3.png" width="150"/>
|
||||
|
||||
1. Typed Data - V4
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/v4.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/v4.png" width="150"/>
|
||||
|
||||
1. SIWE Signature
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/siwe.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/siwe.png" width="150"/>
|
||||
|
||||
## The current flow of control for Signature Request looks like:
|
||||
|
||||
|
||||
|
||||
|
||||
## The proposed flow of control:
|
||||
|
||||
|
||||
|
||||
|
||||
## Proposed Refactoring:
|
||||
|
||||
@ -48,33 +48,9 @@ There are many areas in above flow where the code can be improved upon to cleanu
|
||||
- [PersonalMessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/personal-message-manager.js)
|
||||
- [TypedMessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/typed-message-manager.js)
|
||||
|
||||
Above message managers handle different types of message requests sent by DAPP. There is a lot of code duplication between the 3 classes. We can extract out a parent class and move duplicated code to it. Functions that can be moved to parent class:
|
||||
Above message managers handle different types of message requests sent by DAPP. There is a lot of code duplication between the 3 classes.
|
||||
|
||||
1. `constructor` - variable initialisation:
|
||||
|
||||
```
|
||||
this.messages = [];
|
||||
this.metricsEvent = metricsEvent;
|
||||
```
|
||||
|
||||
1. `unapprovedMsgCount`
|
||||
1. `getUnapprovedMsgs`
|
||||
1. `addUnapprovedMessageAsync` - partially
|
||||
1. `addUnapprovedMessage` - partially
|
||||
1. `addMsg`
|
||||
1. `getMsg`
|
||||
1. `approveMessage`
|
||||
1. `setMsgStatusApproved`
|
||||
1. `setMsgStatusSigned`
|
||||
1. `prepMsgForSigning`
|
||||
1. `rejectMsg`
|
||||
1. `errorMessage`
|
||||
1. `clearUnapproved`
|
||||
1. `_setMsgStatus`
|
||||
1. `_updateMsg`
|
||||
1. `_saveMsgList`
|
||||
|
||||
Much de-duplication can be achieved in message menagers.
|
||||
We should migrate to use `MessageManagers` from `@metamask/core` repo [here](https://github.com/MetaMask/core/tree/main/packages/message-manager).
|
||||
|
||||
2. ### Refactoring Routing to Signature Request pages:
|
||||
|
||||
@ -86,15 +62,13 @@ There are many areas in above flow where the code can be improved upon to cleanu
|
||||
|
||||
3. ### Refactoring in [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js)
|
||||
|
||||
- While fixing routing [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js) will be renamed to pages/confirm-signature-request component
|
||||
- We are getting rid of [confirm-transaction](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js) component from the flow. Thus, we need to ensure that any required logic from the component is extracted into a reusable hook and included in pages/confirm-signature-request.
|
||||
[This](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js#L158) check for valid transaction id should be made re-usable and extracted out.
|
||||
- The component can be converted to a function react component and use selectors to get state and get rid of `mapStatToProps`. [#17239](https://github.com/MetaMask/metamask-extension/issues/17239)
|
||||
- Various callbacks to sign message, cancel request, etc for different type of messaged can be moved to respective child components.
|
||||
- On component mount/update if there are no unapproved messages redirect to `mostRecentlyOverviewedPage` as [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L187).
|
||||
- Not pass values like [these](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L260) to child components which can be obtained in child components using selectors.
|
||||
- [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js) to be renamed to `pages/confirm-signature-request component`
|
||||
- Get rid of [confirm-transaction](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js) component from signature request routing. Thus, we need to ensure that any required logic from the component is extracted into a reusable hook and included in pages/confirm-signature-request.
|
||||
- Convert to functional react component and use selectors to get state and get rid of `mapStateToProps`. [#17239](https://github.com/MetaMask/metamask-extension/issues/17239)
|
||||
- Various callbacks to `sign message`, `cancel request`, etc for different types of messages can be moved to respective child components.
|
||||
- On component `mount/update` if there are no unapproved messages redirect to `mostRecentlyOverviewedPage` as [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L187).
|
||||
- Do not pass values like [these](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L260) to child components which can be obtained in child components using selectors.
|
||||
- Extract logic [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L218) to show success modal for previously confirmed transaction into separate hook.
|
||||
- **Question** - [this](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L241) check for message params look confusing - is it possible for a message request to not have message params or for other transactions to have message params. [Here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L185) we could have just checked for unapproved messages, why are we checking for pending transactions also ?
|
||||
|
||||
4. ### Refactoring component rendering Signature Request Pages
|
||||
|
||||
@ -110,28 +84,29 @@ There are many areas in above flow where the code can be improved upon to cleanu
|
||||
5. ### Refactoring in signature-request-original
|
||||
|
||||
- Rename, this component takes care of ETH sign, personal sign, sign typed data V1 requests. Let's rename it accordingly.
|
||||
- Get rid of container components and for other components migrate to functional react components.
|
||||
- Move this [metrics event](https://github.com/MetaMask/metamask-extension/blob/71a0bc8b3ff94478e61294c815770e6bc12a72f5/ui/app/components/app/signature-request-original/signature-request-original.component.js#L47) to pages/confirm-signature-request as it is applicable to all the signature requests types.
|
||||
- Get rid of container components
|
||||
- Migrate other classical components to functional react components.
|
||||
- Move this [metrics event](https://github.com/MetaMask/metamask-extension/blob/71a0bc8b3ff94478e61294c815770e6bc12a72f5/ui/app/components/app/signature-request-original/signature-request-original.component.js#L50) to pages/confirm-signature-request as it is applicable to all the signature requests types.
|
||||
- Header or we can say upper half of the page of all signature request pages (except SIWE) are very similar, this can be extracted into a reusable component used across both signature-request-original and signature-request:
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/header.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/header.png" width="150"/>
|
||||
|
||||
- [LedgerInstructions](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L308) can also be moved to the header.
|
||||
- [LedgerInstructions](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L312) can also be moved to the header.
|
||||
- Create a reuable footer component and use it across all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237)
|
||||
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/footer.png" width="150"/>
|
||||
<img src="https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/footer.png" width="150"/>
|
||||
|
||||
- Create a reuable component for Cancel All requests for use across signature request pages [Code](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L322).
|
||||
- Extract [getNetrowkName](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L56) into a reuable hook / utility method.
|
||||
- [msgHexToText](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L75) to be made a utility method.
|
||||
- Extract [renderBody](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L110) into a reusable component.
|
||||
- Create a reusable component for Cancel All requests for use across signature request pages [Code](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L326).
|
||||
- Extract [getNetrowkName](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L60) into a reusable hook / utility method.
|
||||
- [msgHexToText](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L79) to be made a utility method.
|
||||
- Extract [renderBody](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L114) into a reusable component.
|
||||
|
||||
6. ### Refactoring in signature-request
|
||||
|
||||
- Get rid of container components and for other components migrate to functional react components.
|
||||
- Reuse the Header component created for signature-request pages
|
||||
- Reuse the footer component created for confirmation pages.
|
||||
- Extract [formatWallet](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request/signature-request.component.js#L85) into a utility method.
|
||||
- Extract [formatWallet](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request/signature-request.component.js#L93) into a utility method.
|
||||
|
||||
7. ### Refactoring in signature-request-siwe
|
||||
- Footer component use `PageContainerFooter` can be converted into a footer component for all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237)
|
||||
- Footer component use `PageContainerFooter` can be used as footer component for all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237)
|
||||
|
Before Width: | Height: | Size: 54 KiB After Width: | Height: | Size: 54 KiB |
|
Before Width: | Height: | Size: 3.0 KiB After Width: | Height: | Size: 3.0 KiB |
|
Before Width: | Height: | Size: 14 KiB After Width: | Height: | Size: 14 KiB |
|
Before Width: | Height: | Size: 50 KiB After Width: | Height: | Size: 50 KiB |
|
Before Width: | Height: | Size: 90 KiB After Width: | Height: | Size: 90 KiB |
|
Before Width: | Height: | Size: 64 KiB After Width: | Height: | Size: 64 KiB |
|
Before Width: | Height: | Size: 45 KiB After Width: | Height: | Size: 45 KiB |
|
Before Width: | Height: | Size: 49 KiB After Width: | Height: | Size: 49 KiB |
|
Before Width: | Height: | Size: 57 KiB After Width: | Height: | Size: 57 KiB |
|
Before Width: | Height: | Size: 58 KiB After Width: | Height: | Size: 58 KiB |
@ -1,7 +0,0 @@
|
||||
# Confirmation Pages Refactoring
|
||||
|
||||
The document details about refactoring confirmation pages. It describes the current code and improved proposed architecture.
|
||||
|
||||
1. [Signature Request Pages](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/signature-request)
|
||||
|
||||
2. [Confirmation Pages Routing](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/confirmation-pages-routing)
|
||||
@ -1,65 +0,0 @@
|
||||
# Refactoring - Confirmation pages routing
|
||||
|
||||
This document details how routing to confirmation pages is currently done and the proposed improvements in routing.
|
||||
|
||||
## Current flow
|
||||
|
||||
The current flow of routing to confirmation pages is un-necessarily complicated and have issues.
|
||||
|
||||
|
||||
|
||||
- There are 2 ways in which confirmation pages can be opened:
|
||||
1. User triggers send flow from within Metamask
|
||||
- If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to the **/confirm-transaction** route.
|
||||
2. DAPP sends request to Metamask
|
||||
- If DAPP sends request to Metamask an un-approved transaction or signature request is created in background and UI is triggered open (if it is not already open).
|
||||
- The router by default renders `pages/home` component. The component looks at the state and if it finds an un-approved transaction or signature request in state it re-routes to **/confirm-transaction**.
|
||||
- For **/confirm-transaction/** route, the router renders `pages/confirm-transaction` component.
|
||||
- For **/confirm-transaction** route `pages/confirm-transaction` component renders `pages/confirm-transaction-switch` by default (for token methods it renders `pages/confirm-transaction/confirm-token-transaction-switch` which also open `pages/confirm-transaction-switch` by default).
|
||||
- `pages/confirm-token-switch` redirect to specific confirmation page route depending on un-approved transaction or signature request in the state.
|
||||
- For specific route **/confirm-transaction/${id}/XXXXX** routes also `pages/confirm-transaction` is rendered.
|
||||
- Depending on confirmation route `pages/confirm-transaction` and `pages/confirm-transaction/confirm-token-transaction-switch` renders specific confirmation page component.
|
||||
|
||||
## Proposed flow
|
||||
|
||||
The proposed routing of confirmation pages looks like.
|
||||
|
||||
|
||||
|
||||
- There are 2 ways in which confirmation pages can be opened:
|
||||
1. User triggers send flow from within Metamask
|
||||
- [changed] If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to a specific transaction route, **/confirm-transaction/${id}/XXXX**, depending on the transaction type.
|
||||
2. DAPP sends request to Metamask
|
||||
- If DAPP send request to Metamask an un-approved transaction or signature request is created in background and UI is triggered to open (if it is not already open).
|
||||
- [changed] Instead of rendering `pages/home`, `pages/routes` finds the unapproved transaction in state and reroutes to **/confirm-transaction**.
|
||||
- Router renders `pages/confirm-transaction` component for **/confirm-transaction** route.
|
||||
- `pages/confirm-transaction` component redirect to specific confirmation page route depending on un-approved transaction or signature request in the state.
|
||||
- Again for specific route **/confirm-transaction/${id}/XXXXX** `pages/confirm-transaction` is rendered, it in-turn renders appropriate confirmation page for the specific route.
|
||||
|
||||
## Current Route component mapping
|
||||
|
||||
| Route | Component |
|
||||
| ----------------------------------------------- | ------------------------------------ |
|
||||
| /confirm-transaction/${id}/deploy-contract | pages/confirm-deploy-contract |
|
||||
| /confirm-transaction/${id}/send-ether | pages/confirm-send-ether |
|
||||
| /confirm-transaction/${id}/send-token | pages/confirm-send-token |
|
||||
| /confirm-transaction/${id}/approve | pages/confirm-approve |
|
||||
| /confirm-transaction/${id}/set-approval-for-all | pages/confirm-approve |
|
||||
| /confirm-transaction/${id}/transfer-from | pages/confirm-token-transaction-base |
|
||||
| /confirm-transaction/${id}/safe-transfer-from | pages/confirm-token-transaction-base |
|
||||
| /confirm-transaction/${id}/token-method | pages/confirm-contract-interaction |
|
||||
| /confirm-transaction/${id}/signature-request | pages/confirm-signature-request.js |
|
||||
|
||||
## Areas of code refactoring
|
||||
- **Routing to mostRecentOverviewPage**
|
||||
Across confirmation pages there is code to re-direct to `mostRecentOverviewPage`. `mostRecentOverviewPage` is equal to default route `/` or `/asset` whichever was last opened.
|
||||
|
||||
Also a lot of components check for state update and as soon as state has `0` pending un-approved transaction or signature request redirect is done to `mostRecentOverviewPage`. This logic can be handled at `/pages/confirm-transaction` which is always rendered for any confirmation page.
|
||||
|
||||
Also when the transaction is completed / rejected redirect is done to `mostRecentOverviewPage` explicitly which we should continue to do.
|
||||
- Any re-usable routing related code should be moved to [useRouting](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/useRouting.js) hook.
|
||||
- Logic to initially check state and redirect to `/pages/confirm-transaction` can be moved from `/pages/home` to `pages/routes`
|
||||
- Confirmation components have lot of props passing which needs to be reduced. Values can be obtained from redux state or other contexts directly using hooks. Component [confirm-token-transaction-switch](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-token-transaction-switch.js) has a lot of un-necessary props passing which should be removed and will help to further refactor routing.
|
||||
- All the route mapping code should be moved to `/pages/confirm-transaction`, this will require getting rid of route mappings in `/pages/confirm-transaction/confirm-token-transaction-switch`, `/pages/confirm-transaction-switch`.
|
||||
- `/pages/confirm-transaction-switch` has the code that check the un-approved trancation / message in state and reditect to a specific route, a utility method can be create to do this mapping and can be included in `/pages/confirm-transaction` component.
|
||||
- During the send flow initiated within metamask user should be redirected to specific confirmations route **/confirm-transaction/${id}/XXXX**
|
||||
Loading…
Reference in New Issue
Block a user