Feat/smart tx txcontroller v13 – DO NOT REVIEW #9565
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/swappable-obj-proxy@2.1.0 |
|
@SocketSecurity ignore npm/@metamask/eth-json-rpc-infura@9.1.0 |
This was referenced May 7, 2024
|
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
| @@ -113,19 +113,65 @@ index 9d8a521..601fd18 100644 | |||
| exports.CHAIN_ID_TO_NAME_MAP = { | |||
| [exports.ETH_CHAIN_ID]: 'ethereum', | |||
| [exports.BSC_CHAIN_ID]: 'bsc', | |||
| diff --git a/node_modules/@metamask/swaps-controller/dist/swapsInterfaces.d.ts b/node_modules/@metamask/swaps-controller/dist/swapsInterfaces.d.ts | |||
There was a problem hiding this comment.
Same question and comments here as in https://github.com/MetaMask/swaps-controller/pull/229
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
legobeat
reviewed
May 7, 2024
@infiniteflower FYI |
legobeat
reviewed
May 7, 2024
app/core/Engine.ts
Outdated
Comment on lines
1059
to
1101
| const isSmartTransaction = getIsSmartTransaction(store.getState()); | ||
|
|
||
| return submitSmartTransactionHook({ | ||
| transactionMeta, | ||
| transactionController: this.txController, | ||
| smartTransactionsController: this.stxController, | ||
| isSmartTransaction, | ||
| approvalController, | ||
| featureFlags: getSwapsChainFeatureFlags(store.getState()), | ||
| }); |
There was a problem hiding this comment.
Since we know already here if isSmartTransaction, why can the hook not be skipped if false? Does it it trigger something that is also necessary for non-smart-transactions?
There was a problem hiding this comment.
The SmartTransactionHook will return { transactionHash: undefined } to the TransactionController.hooks.publish, which will then cause the transaction to be executed as a normal transaction.
It does make it a little harder to test since this logic moves outside of the class SmartTransactionHook.
There was a problem hiding this comment.
There's 2 places where we can fallback to a regular tx.
- If
isSmartTransactionisfalse: this can happen even if a user has opted in due to failing health checks on the API - If
/getFeesendpoint call fails
const useRegularTransactionSubmit = { transactionHash: undefined };
if (!this.#isSmartTransaction) {
return useRegularTransactionSubmit;
}
Logger.log(LOG_PREFIX, 'Started submit hook', this.#transactionMeta.id);
try {
const getFeesResponse = await this.#getFees();
// In the event that STX health check passes, but for some reason /getFees fails, we fallback to a regular transaction
if (!getFeesResponse) {
return useRegularTransactionSubmit;
}
Think it's best to encapsulate all fallbacks to regular transactions inside this hook to keep it in one place.
infiniteflower
added
Run Smoke E2E
|
infiniteflower
added
Run Smoke E2E
|
@SocketSecurity ignore npm/@metamask/smart-transactions-controller@10.0.0 |
|
infiniteflower
added
Run Smoke E2E
|
dbrans
changed the title
|
|
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 11, 2024
infiniteflower
added a commit
that referenced
this pull request
May 14, 2024
7 tasks
infiniteflower
added a commit
that referenced
this pull request
May 14, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers the strings from #9565. ## **Related issues** Split off from: #9565 ## **Manual testing steps** n/a ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> n/a ### **Before** <!-- [screenshots/recordings] --> n/a ### **After** <!-- [screenshots/recordings] --> n/a ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
infiniteflower
added a commit
that referenced
this pull request
May 15, 2024
infiniteflower
added a commit
that referenced
this pull request
May 15, 2024
infiniteflower
added a commit
that referenced
this pull request
May 15, 2024
infiniteflower
added a commit
that referenced
this pull request
May 15, 2024
infiniteflower
added a commit
that referenced
this pull request
May 16, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers a smaller subset of the files from #9565 for ease of reviewing. Please refer to that issue for videos. This PR is mostly for the core logical parts of Smart Transactions (STX). This PR can be merged as it won't change any existing behavior in the application without a follow up PR to actually use these utility functions. It covers the following areas 1. `/app/util` - except `app/util/test/initial-background-state.json`, `app/util/transactions/index.js`, `app/util/transactions/index.test.ts` since it causes tests to fail 3. `/app/images` 4. `/e2e/selectors` 5. `app/core/RPCMethods/RPCMethodMiddleware.ts` (to fix tests) 6. `package.json` (Added `smart-transactions-controller` to this PR to fix failing tests) 7. `app/components/Nav/Main/RootRPCMethodsUI.js` to fix failing tests 8. `@metamask+transaction-controller+13.0.0.patch` (https://github.com/MetaMask/core/compare/patch/mobile-transaction-controller-13-0-0-smart-transactions) 9. `yarn.lock` 10. `jest.config.js` ## **Related issues** - Broken out from #9565 - Functionality in this PR enabled in follow-up #9448 ## **Manual testing steps** Refer to #9565 ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> Refer to #9565 ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
Gudahtt
added a commit
that referenced
this pull request
May 16, 2024
…ller-v16 * origin/main: feat: Update signature controller v13 (#9625) chore(deps): remove unused react-native-v8 (#9619) feat: fix wait function not returning properly (#9633) feat: smart-tx-small-logic (#9442) New Crowdin translations by Github Action (#9428) feat: Add new privacy policy alert toast (#9204) chore: apply string changes from #9565 (#9629)
infiniteflower
added a commit
that referenced
this pull request
May 16, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers a smaller subset of the files from #9565 for ease of reviewing. Please refer to that issue for videos. This PR is mostly for Engine and the views for Smart Transactions (STX). Merging this PR will cause changes to the functionality and behavior of the app, effectively making the feature "live". It covers the following areas: 1. `app/components` 2. `app/core` 3. `app/reducers` 4. `app/selectors` 5. `app/util/test` 6. `patches` - `PreferencesController` ([PR here](MetaMask/core#3815) to apply update to `main`) - `SwapsController` ([PR here](MetaMask/swaps-controller#229) to apply updates to `main`) Also fixed an issue for network onboarding where it would never set onboarded to `true`. This needed to be fixed so we could properly show the STX Opt In Modal. We only show the Opt In modal if the user has onboarded onto the network, to prevent multiple modals from showing up at the same time. See discussion #9448 (comment) ## **Related issues** Fixes: n/a - Targeting: #9442 - Broken out from: #9565 ## **Manual testing steps** Refer to #9565 ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> Refer to #9565 ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com> Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
Andepande
pushed a commit
that referenced
this pull request
May 21, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers the strings from #9565. ## **Related issues** Split off from: #9565 ## **Manual testing steps** n/a ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> n/a ### **Before** <!-- [screenshots/recordings] --> n/a ### **After** <!-- [screenshots/recordings] --> n/a ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
Andepande
pushed a commit
that referenced
this pull request
May 21, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers a smaller subset of the files from #9565 for ease of reviewing. Please refer to that issue for videos. This PR is mostly for the core logical parts of Smart Transactions (STX). This PR can be merged as it won't change any existing behavior in the application without a follow up PR to actually use these utility functions. It covers the following areas 1. `/app/util` - except `app/util/test/initial-background-state.json`, `app/util/transactions/index.js`, `app/util/transactions/index.test.ts` since it causes tests to fail 3. `/app/images` 4. `/e2e/selectors` 5. `app/core/RPCMethods/RPCMethodMiddleware.ts` (to fix tests) 6. `package.json` (Added `smart-transactions-controller` to this PR to fix failing tests) 7. `app/components/Nav/Main/RootRPCMethodsUI.js` to fix failing tests 8. `@metamask+transaction-controller+13.0.0.patch` (https://github.com/MetaMask/core/compare/patch/mobile-transaction-controller-13-0-0-smart-transactions) 9. `yarn.lock` 10. `jest.config.js` ## **Related issues** - Broken out from #9565 - Functionality in this PR enabled in follow-up #9448 ## **Manual testing steps** Refer to #9565 ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> Refer to #9565 ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
Andepande
pushed a commit
that referenced
this pull request
May 21, 2024
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR only covers a smaller subset of the files from #9565 for ease of reviewing. Please refer to that issue for videos. This PR is mostly for Engine and the views for Smart Transactions (STX). Merging this PR will cause changes to the functionality and behavior of the app, effectively making the feature "live". It covers the following areas: 1. `app/components` 2. `app/core` 3. `app/reducers` 4. `app/selectors` 5. `app/util/test` 6. `patches` - `PreferencesController` ([PR here](MetaMask/core#3815) to apply update to `main`) - `SwapsController` ([PR here](MetaMask/swaps-controller#229) to apply updates to `main`) Also fixed an issue for network onboarding where it would never set onboarded to `true`. This needed to be fixed so we could properly show the STX Opt In Modal. We only show the Opt In modal if the user has onboarded onto the network, to prevent multiple modals from showing up at the same time. See discussion #9448 (comment) ## **Related issues** Fixes: n/a - Targeting: #9442 - Broken out from: #9565 ## **Manual testing steps** Refer to #9565 ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> Refer to #9565 ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com> Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
9 tasks
|
This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
This PR was closed because there has been no follow up activity in 7 days. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
DO NOT REVIEW – Not intended for review
PR for STX + TransactionController v13
Description
This PR does the following:
Related issues
Fixes: n/a
Manual testing steps
Since STX touches all transaction types, here are the major areas for testing:
Send
MM Swaps
Dapp
Screenshots/Recordings
Refer to #9565
Before
n/a
After
Screen.Recording.2024-05-15.at.11.00.18.PM.-.send.eth.mov
Screen.Recording.2024-05-15.at.11.01.39.PM.-.send.erc20.mov
Screen.Recording.2024-05-15.at.11.02.48.PM.-.swap.eth.mov
Screen.Recording.2024-05-15.at.11.03.48.PM.-.swap.usdc.revert.mov
Screen.Recording.2024-05-15.at.11.08.11.PM.-.swap.usdc.mov
Screen.Recording.2024-05-15.at.11.16.55.PM.-.dapp.uni.eth.mov
Screen.Recording.2024-05-15.at.11.21.00.PM.-.dapp.matcha.eth.mov
Screen.Recording.2024-05-15.at.11.23.36.PM.-.dapp.matcha.erc20.mov
Screen.Recording.2024-05-15.at.11.24.49.PM.-.dapp.matcha.erc20.approve.mov