fix: Fix hardware wallet MMPay on EIP-7702 chains by gating 7702 paths on account keyring capability#8388
fix: Fix hardware wallet MMPay on EIP-7702 chains by gating 7702 paths on account keyring capability#8388
Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
| account: string, | ||
| ): boolean { | ||
| const { keyrings } = messenger.call('KeyringController:getState'); | ||
| const keyring = keyrings.find((k: { type: string; accounts: string[] }) => |
There was a problem hiding this comment.
Minor, could we return this with a some instead?
There was a problem hiding this comment.
Switched to some here. Also changed the fallback to return false when the keyring isn't found, since we'd rather be restrictive by default and not allow 7702 for accounts we can't resolve. Applied the same change to the pay controller duplicate.
| if (checkStatus(initialTx)) { | ||
| return; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Minor, it should always exist right as we're awaiting the initial addTransaction first?
There was a problem hiding this comment.
Removed as it's unnecessary
| hookTransactions.push(hookTransaction); | ||
| index += 1; | ||
|
|
||
| if (!alreadySigned) { |
There was a problem hiding this comment.
Can we remove this and rely just on waitForTransactionStatus for maximum simplicity and smallest diff?
There was a problem hiding this comment.
Agreed, no more alreadySigned prop and check
| ): Promise< | ||
| Omit<PublishBatchHookTransaction, 'signedTx'> & { type?: TransactionType } | ||
| > { | ||
| ): Promise<{ |
There was a problem hiding this comment.
My hope is we wouldn't have to change anything in this function, but just accept the submitted or confirmed state also, so we have minimum branches and edge cases?
| if (!alreadySigned) { | ||
| await waitForTransactionStatus( | ||
| String(hookTransaction.id), | ||
| TransactionStatus.signed, |
There was a problem hiding this comment.
Could this be an array including submitted and confirmed so it covers all the bases including the existing transaction path?
There was a problem hiding this comment.
Added these statuses as well
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
|
|
||
| getKeyringControllerStateMock.mockReturnValue({ |
There was a problem hiding this comment.
Minor, I guess this could mock the transaction controller action only?
There was a problem hiding this comment.
Removed the unnecessary mocks
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9048c72. Configure here.
## Explanation Release `942.0.0` with major version bumps for two packages: - **`@metamask/transaction-controller`** `64.4.0` → `65.0.0` - **`@metamask/transaction-pay-controller`** `19.3.0` → `20.0.0` ### `@metamask/[email protected]` **Breaking:** Adds `KeyringControllerGetStateAction` to `AllowedActions` to enable keyring-based EIP-7702 account compatibility checks in `addTransactionBatch`. Clients must add `KeyringController:getState` to the TransactionController messenger's allowed actions. ### `@metamask/[email protected]` **Breaking:** Fix mUSD conversion for hardware wallets on EIP-7702 chains by gating relay and Across 7702 paths on the account keyring type via `KeyringController:getState`. The `TransactionPayControllerMessenger` now requires `KeyringController:getState` permission. ### Dependency updates 14 packages had their `@metamask/transaction-controller` dependency range updated to `^65.0.0` with corresponding changelog entries under `[Unreleased]`. ## References - [MetaMask#8388](MetaMask#8388) — EIP-7702 keyring compatibility - [MetaMask#8592](MetaMask#8592) — Expose `wipeTransactions` via messenger - [MetaMask#8607](MetaMask#8607) — Rename `executeEnabled` → `gaslessEnabled` - [MetaMask#8577](MetaMask#8577) — Across EIP-7702 authorization list fallback ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [x] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Primarily a release/version bump, but it upgrades to `@metamask/[email protected]` and `@metamask/[email protected]`, which include **breaking** messenger permission/allowed-action changes that can break consumers at runtime if not updated. > > **Overview** > Bumps the monorepo version to `942.0.0` and releases **major** updates for `@metamask/transaction-controller` (`64.4.0` → `65.0.0`) and `@metamask/transaction-pay-controller` (`19.3.0` → `20.0.0`). > > Propagates `@metamask/transaction-controller` dependency range updates to `^65.0.0` across multiple packages (e.g. assets/bridge/network/shield/subscription/user-operation controllers) with corresponding `[Unreleased]` changelog entries, and updates `yarn.lock` accordingly. > > Documents breaking changes in `[email protected]` (requires `KeyringController:getState` allowed action for EIP-7702 batch compatibility checks) and `[email protected]` (requires `KeyringController:getState` to gate 7702 paths for hardware-wallet mUSD conversion). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit de55ba7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->

Explanation
Fix hardware wallet mUSD conversion on EIP-7702 chains by gating 7702 paths on account keyring capability, and fix batch transaction signing to prevent remaining hardware wallet prompts after a rejection.
Summary
@metamask/transaction-controllerKeyringControllerGetStateActiontoAllowedActions— clients must addKeyringController:getStateto the TransactionController messenger's allowed actionsdoesAccountSupportEIP7702ineip7702.ts— checks keyring type viaKeyringController:getState, returnstrueonly for HD Key Tree and Simple Key Pair keyrings, falls back totruewhen keyring is unresolvedaddTransactionBatch7702 path ondoesAccountSupportEIP7702— hardware wallet accounts fall back to sequential/hook batch flow instead of attempting EIP-7702 batch signingaddTransactionfor the next, preventing remaining Ledger prompts from appearing after a rejectionCollectPublishHook.waitForSignedCount(count)to support sequential signing gating in the batch loopabortTransactionSigning@metamask/transaction-pay-controllerKeyringControllerGetStateActiontoAllowedActions— clients must addKeyringController:getStateto the TransactionPayController messenger's allowed actionsaccountSupports7702utility inutils/7702.tsandKEYRING_TYPES_SUPPORTING_7702constant intypes.tsaccountSupports7702flag throughPayStrategyGetQuotesRequestandPayStrategyExecuteRequesttypesuseExecuteflag onaccountSupports7702— ensures the relay API receives non-7702 requests for hardware wallets and returns quotes with proper individual gas limitsaccountSupports7702fromTransactionPayPublishHookinto strategyexecute()callsaccountSupports7702from fiat-quotes into relay-quotesestimateQuoteGasLimitsBatchwhen batch simulation returns a combined 7702 gas limit but account doesn't support 7702References
Checklist
Note
Medium Risk
Touches transaction batching, signing flow control, and gas/quote estimation, plus introduces new messenger permissions; regressions could impact batch submission behavior especially across different keyring types.
Overview
Fixes EIP-7702 batching/quote behavior for hardware-wallet accounts by gating all 7702 paths on keyring capability. Both
TransactionControllerandTransactionPayControllernow requireKeyringController:getStatein messengerAllowedActions, and new helpers (doesAccountSupportEIP7702/accountSupports7702) treat onlyHD Key TreeandSimple Key Pairaccounts as 7702-capable.addTransactionBatchandestimateGasBatchnow skip the EIP-7702 path when the account can’t sign 7702 authorizations, falling back to per-transaction/sequential behavior. Batch signing via hook is also made sequential by waiting for each transaction to reach a signed/submitted state before proceeding, reducing “extra” hardware wallet prompts after a rejection.MMPay threads an
accountSupports7702flag through strategy request/execute types and uses it to disable Relay “execute” (7702) request shaping and to produce non-7702 gas limit outputs when needed; tests and changelogs are updated accordingly.Reviewed by Cursor Bugbot for commit 4fa203c. Bugbot is set up for automated code reviews on this repo. Configure here.