Repository navigation
feat: add Ethereum MPC Money Keyring - #627
matthiasgeihs wants to merge 39 commits into
Conversation
Introduce a 2-party client/server MPC keyring with DKLS23 TSS, cloud backup sync, and vendored MFA wallet libs. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Replaces #440 with a cleaned history rebased onto |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Pass convertV so recovery id 27|28 becomes yParity or EIP-155 v. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Replace backupId with shareEpoch so create/rotate append and activate epochs only after backup readiness, matching the robust MPC architecture. Co-authored-by: Cursor <cursoragent@cursor.com>
Prevent mid-sign rotate/sync from being overwritten by stale Co-authored-by: Cursor <cursoragent@cursor.com> #applyKeyState snapshots from concurrent signing.
|
Do not merge yet: this PR still vendors MFA wallet packages as local |
Pass the create/rotate session nonce to storeKeyShareBackup so a delayed backup from an overwritten retry cannot attach to a newer share. Co-authored-by: Cursor <cursoragent@cursor.com>
<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:
* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
Are there any issues or other links reviewers should consult to
understand this pull request better? For instance:
* Fixes #12345
* See: #67890
-->
## Summary
- MPC cloud HTTP calls now send the profile token in an `Authorization:
Bearer <token>` header instead of including `token` in JSON request
bodies.
- The shared `postJson` helper accepts the token as a separate argument
and applies it consistently across all MPC backend requests.
- Token-only endpoints (`getNetId`, `checkKeyShareBackupId`,
`loadKeyShareBackup`) still use POST but now send an empty JSON body
(`{}`).
## Test plan
- [x] Run `packages/keyring-eth-mpc` unit tests (`cloud.test.ts`)
- [x] Verify `getNetId` requests include the Bearer header and send `{}`
as the body
- [x] Verify sign and backup helpers include the Bearer header and omit
`token` from the JSON payload
- [ ] Confirm MPC backend accepts Bearer authentication for affected
endpoints before merge
<!--
Are there any examples of this change being used in another repository?
When considering changes to the MetaMask module template, it's strongly
preferred that the change be experimented with in another repository
first. This gives reviewers a better sense of how the change works,
making it less likely the change will need to be reverted or adjusted
later.
-->
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Auth moves from JSON to Bearer headers across all MPC cloud calls;
clients and the backend must agree before deploy or requests will fail.
>
> **Overview**
> MPC cloud HTTP clients no longer put the profile `token` in JSON
bodies. **`postJson`** now takes the token separately and sets
**`Authorization: Bearer <token>`** on every backend POST.
>
> Endpoints that previously sent only `{ token }` (**`getNetId`**,
**`checkKeyShare`**, **`loadKeyShareBackup`**) now POST **`{}`** with
auth in the header. Create/sign/rotate/backup/register calls keep their
payloads but drop the **`token`** field. Unit tests in
**`cloud.test.ts`** assert the header and that **`body.token`** is
absent.
>
> The unreleased **CHANGELOG** entry is shortened to a single
initial-release line (detailed feature bullets removed).
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
400e5b1. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
---------
Co-authored-by: Matthias Geihs <matthias.geihs@consensys.net>
<!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> ## Summary - Add a shared `parseJsonResponse` helper in `packages/keyring-eth-mpc/src/cloud.ts` to centralize JSON parsing and non-OK error handling for cloud API responses. - Introduce `getJson` for Bearer-authenticated GET requests and refactor `postJson` to reuse `parseJsonResponse`. - Update `loadKeyShareBackup` to call `getJson` with GET instead of POST with an empty body. <!-- Are there any examples of this change being used in another repository? When considering changes to the MetaMask module template, it's strongly preferred that the change be experimented with in another repository first. This gives reviewers a better sense of how the change works, making it less likely the change will need to be reverted or adjusted later. --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes the HTTP method for loading encrypted key-share backups; behavior depends on the MPC backend accepting GET on that route. > > **Overview** > Refactors MPC cloud HTTP helpers and fixes how encrypted key-share backups are fetched. > > **`cloud.ts`** adds **`parseJsonResponse`** for shared non-OK handling and JSON/empty-body parsing, introduces **`getJson`** for Bearer-authenticated GETs, and routes **`postJson`** through the same parser (generic renamed to **`Result`**). **`loadKeyShareBackup`** now calls **`getJson`** on `/load-key-share-backup` instead of **`postJson`** with an empty body. > > **`cloud.test.ts`** asserts the backup load uses **GET** with **`Authorization: Bearer`** and no POST body. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2690e54. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Matthias Geihs <matthias.geihs@consensys.net> Co-authored-by: Cursor <cursoragent@cursor.com>
danroc
added
DO-NOT-MERGE
Co-authored-by: Cursor <cursoragent@cursor.com>
Run init inside #serializeOp with the initialized check inside the queue, so overlapping init calls run key generation or import once. The second call waits, sees the initialized state and returns. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 043a708. Configure here.
|
|
||
| if (hasInitializedFields(state)) { | ||
| throw new Error('Invalid state'); | ||
| } |
There was a problem hiding this comment.
Invalid state accepted as setup
Low Severity
deserialize checks SetupParamsStruct before hasInitializedFields, so a payload that has the initialized fields but fails the struct is treated as setup whenever mode is create or import. That drops the key material and later init can create or import a different key instead of rejecting the state.
Reviewed by Cursor Bugbot for commit 043a708. Configure here.
There was a problem hiding this comment.
Should be fixed in fef54748ebba63d98ca358492315d9b37a9c9880
The SDK dropped the -lib suffix and the util package. The keyring now uses the network package's identity serializer and scoped session id. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
Keep the V1 MPC keyring implementation together with the V2 adapter already on main. Co-authored-by: Cursor <cursoragent@cursor.com>
| /** | ||
| * Generate a session nonce: random bytes from the RNG, hex-encoded. | ||
| * | ||
| * @param rng - The random number generator. | ||
| * @returns Hex-encoded 32-byte random nonce. | ||
| */ | ||
| export function generateSessionNonce(rng: RandomNumberGenerator): Hex { | ||
| return bytesToHex(rng.generateRandomBytes(SESSION_NONCE_BYTE_LENGTH)); | ||
| } |
There was a problem hiding this comment.
Sorry, I meant cryptography.ts actually 🙈 Similarly to our new @metamask/cryptography.
No real benefits, just that we would keep general utils in util and other crypto-utils like, in their own files. So tests are separated etc.
Anyway, not really a blocker.
| /** | ||
| * Encrypt plaintext with AES-GCM. The IV is prepended to the ciphertext. | ||
| * | ||
| * @param key - 16- or 32-byte AES key. | ||
| * @param plaintext - Bytes to encrypt. | ||
| * @param iv - 12-byte IV. | ||
| * @returns `iv || ciphertext || tag`. | ||
| */ | ||
| export async function encryptBytes( | ||
| key: Uint8Array, | ||
| plaintext: Uint8Array, | ||
| iv: Uint8Array, | ||
| ): Promise<Uint8Array> { | ||
| if (iv.length !== AES_GCM_IV_LENGTH) { | ||
| throw new Error('Invalid IV length'); | ||
| } | ||
| if (key.length !== 16 && key.length !== 32) { | ||
| throw new Error('Invalid backup encryption key length'); | ||
| } | ||
|
|
||
| const cryptoKey = await globalThis.crypto.subtle.importKey( | ||
| 'raw', | ||
| key, | ||
| 'AES-GCM', | ||
| false, | ||
| ['encrypt'], | ||
| ); | ||
| const ciphertext = await globalThis.crypto.subtle.encrypt( | ||
| { name: 'AES-GCM', iv }, | ||
| cryptoKey, | ||
| plaintext, | ||
| ); | ||
| return concatBytes(iv, new Uint8Array(ciphertext)); | ||
| } | ||
|
|
||
| /** | ||
| * Decrypt a payload produced by {@link encryptBytes}. | ||
| * | ||
| * @param key - 16- or 32-byte AES key. | ||
| * @param payload - `iv || ciphertext || tag`. | ||
| * @returns The plaintext bytes. | ||
| */ | ||
| export async function decryptBytes( | ||
| key: Uint8Array, | ||
| payload: Uint8Array, | ||
| ): Promise<Uint8Array> { | ||
| if (payload.length <= AES_GCM_IV_LENGTH) { | ||
| throw new Error('Invalid ciphertext'); | ||
| } | ||
| if (key.length !== 16 && key.length !== 32) { | ||
| throw new Error('Invalid backup encryption key length'); | ||
| } | ||
|
|
||
| const iv = payload.slice(0, AES_GCM_IV_LENGTH); | ||
| const ciphertext = payload.slice(AES_GCM_IV_LENGTH); | ||
| const cryptoKey = await globalThis.crypto.subtle.importKey( | ||
| 'raw', | ||
| key, | ||
| 'AES-GCM', | ||
| false, | ||
| ['decrypt'], | ||
| ); | ||
| const plaintext = await globalThis.crypto.subtle.decrypt( | ||
| { name: 'AES-GCM', iv }, | ||
| cryptoKey, | ||
| ciphertext, | ||
| ); | ||
| return new Uint8Array(plaintext); | ||
| } |
There was a problem hiding this comment.
We could re-use @metamask/cryptography for this one now.
There was a problem hiding this comment.
Done 58428ae042879ecf40f744cf6c839a5a89801be9
| /** | ||
| * Convert a public key to an address. | ||
| * | ||
| * @param pubKey - The public key to convert. | ||
| * @returns The address. | ||
| */ | ||
| export function publicToAddressHex(pubKey: Uint8Array): Hex { | ||
| const addrBytes = publicToAddress(pubKey); | ||
| return bytesToHex(addrBytes); | ||
| } |
There was a problem hiding this comment.
That's a duplicate with publicKeyToAddressHex below (we could keep the latter, since it allows other public key format if needed).
There was a problem hiding this comment.
Done ba93edcd7999680c8bc999430af082948fdc7189
| /** | ||
| * Sign a transaction using the specified account. | ||
| * | ||
| * @param address - The address of the account. | ||
| * @param tx - The transaction to sign. | ||
| * @param _opts - The options for signing the transaction. | ||
| * @returns The signed transaction. | ||
| */ | ||
| async signTransaction( | ||
| address: Hex, | ||
| tx: TypedTransaction, | ||
| _opts = {}, | ||
| ): Promise<TypedTransaction> { | ||
| const message = tx.getHashedMessageToSign(); | ||
|
|
||
| const signature = await this.#signHash(address, message); | ||
|
|
||
| const { r, s, v } = parseEthSig(signature); | ||
|
|
||
| // convertV adapts the message-style recovery id (27|28) to typed-tx | ||
| // yParity (0|1) or legacy EIP-155 v, matching `TypedTransaction.sign()`. | ||
| const signedTx = tx.addSignature(v, r, s, true); | ||
| return signedTx; | ||
| } |
There was a problem hiding this comment.
Something to discuss: Worth discussing if we should implement this one or not. The MoneyKeyring is not, so we might want to "disable" it too maybe.
(Though, this keyring is mostly generic, so that makes sense to have it there, maybe just disabling this method on the KeyringAccount on the v2 wrapper would be enough, since the v2 wrapper would reject it)
There was a problem hiding this comment.
- Should we do what we did for the
MoneyKeyring, just have the "generic" part (e.g HD keyring) and then have a more "specific"/"scoped"/"restricted" keyring for the Money keyring with MPC support
tl;dr; We keep it generic + create a MoneyMpcKeyring just for the Money context
| this.#applyKeyState({ | ||
| keyShare, | ||
| netCreds, | ||
| serverNetId, | ||
| shareEpoch: INITIAL_SHARE_EPOCH, | ||
| tssSetup, | ||
| }); |
There was a problem hiding this comment.
Something to discuss: Similar comments than for rotateKeyShares and the potential missing client share (already discussed there)
| token: string; | ||
| }): Promise<PartyId> { | ||
| const data = await postJson<{ netId: string }>( | ||
| `${opts.baseURL}/net-id`, |
There was a problem hiding this comment.
Not sure what our other components are using for this...
Having it explicit makes it clear which version we are depending one, but it would also require a new release of the keyring if somehow we go with a new /v3. That being said, v3 would means breaking changes, so meaning we might have to update the keyring part regardless.
I'll keep this open for now and revisit once I checked our other patterns.
There was a problem hiding this comment.
Something to discuss: (improvements for later) maybe we could move this entire networking logic in a mpc-service package, which would use BaseDataService. This way, we have a proper service to map all backend endpoints + re-use internal networking tooling (network status, custom network policy, etc..)
Though, that means the keyring would depend on another package + use the messenger.
Probably worth discussing this further.
| async syncKeyShare(): Promise<void> { | ||
| return this.#serializeOp(async () => { | ||
| const state = this.#assertState(); | ||
| const token = await this.#getProfileToken({ twoFactor: true }); |
There was a problem hiding this comment.
NOTE: Do we want to use 1FA or 2FA? The reasoning is, if you have access to your SRP, then you can decrypt the device key share -> same level of access
There was a problem hiding this comment.
Probably switching to 1FA here. Will do in a follow up PR.
Changes required after the changes made to the MFA SDK.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
matthiasgeihs
removed
the
DO-NOT-MERGE
Map relative .js specifiers back to TypeScript sources so the MPC keyring tests can load. Also point the skipLibCheck note at the MFA network package. Co-authored-by: Cursor <cursoragent@cursor.com>
…etup mode A payload with invalid initialized fields and mode was accepted as setup because extra keys are ignored, which dropped the key material. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop publicToAddressHex and compare recovered keys with publicKeyToAddressHex, which accepts raw and SEC1 keys. Co-authored-by: Cursor <cursoragent@cursor.com>
Reuse the shared AES-GCM helpers for key-share backup encryption. Co-authored-by: Cursor <cursoragent@cursor.com>
…ounts (#669) ## Summary - Rename the package from `keyring-eth-mpc` / `@metamask/eth-mpc-keyring` to `keyring-eth-money-mpc` / `@metamask/eth-money-mpc-keyring`. - Rename the V1 and V2 keyring classes to `MpcMoneyKeyring`, and the controller type to `MPC Money Keyring`. - Update the related public types, source filenames, and monorepo references (README, tsconfig, Yarn constraints, PR title scope). ## Test plan - [ ] `yarn workspace @metamask/eth-money-mpc-keyring test` - [ ] Confirm the controller still registers the keyring under `MPC Money Keyring` - [ ] Confirm `KeyringType.Mpc` and `KeyringAccountEntropyTypeOption.Mpc` are unchanged Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
Money accounts do not sign transactions, so drop signTransaction and the helpers that only existed for it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Changelog check failed because we removed https://github.com/MetaMask/accounts/actions/runs/38461132928/job/115516282911?pr=627 ==> Disabling changelog check |
matthiasgeihs
changed the title
This branch has not been deployed
4 participants
Summary
@metamask/eth-mpc-keyring: a 2-party client/server MPC keyring with DKLS23 TSS, cloud backup sync, and vendored MFA wallet libsmainTest plan
yarn workspace @metamask/eth-mpc-keyring testyarn workspace @metamask/eth-mpc-keyring buildMade with Cursor