Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

feat(272): add chainId to Keyring API requests (transaction/typed message) #231

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Mar 1, 2024

Description

This is the first step of adding a CAIP-2 scope to internal requests.

It's a "first step" as for now, we cannot get/extract the chainId for some methods. This would require a much bigger refactor (which will come later).

signTransaction requests

The chainId is part of the transaction request, so we extract it from there and attach it to the internal request.

signTypedData (when applicable) requests

The chainId is part of the domain field (as defined by EIP-712), however, some older versions of the eth_signTypedData_vX might not have it! In this case, we do not provide the scope.

As of today, only those methods do not have the domain:

  • eth_signTypedData
  • eth_signTypedData_v1

Related issues

Testing

Manual testing

Prerequisites

  1. Use the SSK
  2. Create a snap account (or use an existing one)
  3. Enable "Use Synchronous Approval"

1. Transaction

  1. Send a transaction using your snap account
  2. DO NOT APPROVE YET
  3. List requests using the SSK
  4. You should see a scope like: eip155:${chainId} (depending on which chain you are using)

2. Typed data

  1. Use the E2E dapp
  2. Connect to your snap account
  3. Sign typed data (using any version you want)
  4. DO NOT APPROVE YET
  5. List requests using the SSK
  6. You should see a scope like: eip155:${chainId} (depending on which chain you are using)

Test coverage report

Coverage remains at 100%

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 100 100 100 100
CaseInsensitiveMap.ts 100 100 100 100
DeferredPromise.ts 100 100 100 100
KeyringSnapControllerClient.ts 100 100 100 100
SnapIdMap.ts 100 100 100 100
SnapKeyring.ts 100 100 100 100
caip.ts 100 100 100 100
index.ts 100 100 100 100
types.ts 100 100 100 100
util.ts 100 100 100 100

Jest

Added a new test for CAIP helpers:

yarn jest src/caip.test.ts

Updated test regarding transaction flow:

yarn jest src/SnapKeyring.test.ts

@ccharly ccharly requested a review from a team as a code owner March 1, 2024 10:40
@ccharly ccharly force-pushed the feature/272/chain-id-keyring-api branch from 6e77c23 to d4dd5c7 Compare March 1, 2024 10:53
@gantunesr gantunesr added the team-accounts This should be handled by the Accounts Team label Mar 1, 2024
src/caip.test.ts Outdated Show resolved Hide resolved
src/caip.test.ts Outdated Show resolved Hide resolved
src/SnapKeyring.test.ts Outdated Show resolved Hide resolved
For some reason, the encoding of `type` and `data` are different from
what we have.

I didn't track down this behavior, so it could be a small "mistake" on
our side, or there is different encoding being used somewhere in the
flow.
@ccharly ccharly force-pushed the feature/272/chain-id-keyring-api branch from d4dd5c7 to b09a4ea Compare March 5, 2024 14:10
@danroc danroc changed the title feat(272): add chainId to keyring api requests (transaction/typed message) feat(272): add chainId to keyring api requests (transaction/typed message) Mar 5, 2024
@danroc danroc changed the title feat(272): add chainId to keyring api requests (transaction/typed message) feat(272): add chainId to Keyring API requests (transaction/typed message) Mar 5, 2024
@danroc danroc added this pull request to the merge queue Mar 5, 2024
Merged via the queue into main with commit 8318c8f Mar 5, 2024
19 checks passed
@danroc danroc deleted the feature/272/chain-id-keyring-api branch March 5, 2024 16:28
k-g-j added a commit to MetaMask/snap-account-abstraction-keyring that referenced this pull request Mar 12, 2024
## Description

PR adds more unit tests for specific flows and error messages with the
`keyring.ts` file. There is a minor refactor in the createAccount method
to check for duplicate accounts.

* Fixes
[#254](https://app.zenhub.com/workspaces/metamask-accounts-team-v2-64c91cbeaa9d1c00126621fd/issues/gh/metamask/accounts-planning/254)
* See:
[#244](https://app.zenhub.com/workspaces/metamask-accounts-team-v2-64c91cbeaa9d1c00126621fd/issues/gh/metamask/accounts-planning/244)

## Pre-requisites

- [ ] https://github.com/MetaMask/accounts-planning/issues/308
  * [x] MetaMask/eth-snap-keyring#231
  * [ ] https://github.com/MetaMask/accounts-planning/issues/307

## Test Coverage Report

### Summary

The test coverage has been updated as part of the efforts to increase
the code coverage for the `snap-account-abstraction-keyring` module. The
tests were executed with `yarn test:coverage --no-cache`, ensuring that
cached results did not influence the outcomes. The coverage for the
`keyring.ts` file is now above 95%.

- **Test Suites**: 1 passed, 1 total
- **Tests**: 35 passed, 35 total

### Coverage Overview

```plaintext
---------------------|---------|----------|---------|---------|-------------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s       
---------------------|---------|----------|---------|---------|-------------------------
All files            |   87.58 |    60.63 |   82.45 |   87.75 |                         
 snap                |     100 |      100 |     100 |     100 |                         
  hardhat.config.ts  |     100 |      100 |     100 |     100 |                         
 snap/src            |   92.34 |    67.74 |    87.5 |   92.26 |                         
  keyring.ts         |   95.67 |    67.92 |     100 |   95.62 | 195,443-445,547-549,562 
  logger.ts          |      75 |    57.14 |   55.55 |      75 | 64,81-92,112-122        
  permissions.ts     |     100 |      100 |     100 |     100 |                         
  stateManagement.ts |    62.5 |      100 |      50 |    62.5 | 18-25                   
 snap/src/constants  |   93.75 |        0 |     100 |   93.75 |                         
  aa-factories.ts    |     100 |      100 |     100 |     100 |                         
  chain-ids.ts       |     100 |      100 |     100 |     100 |                         
  chainConfig.ts     |     100 |      100 |     100 |     100 |                         
  dummy-values.ts    |   88.88 |        0 |     100 |   88.88 | 15                      
  entrypoints.ts     |     100 |      100 |     100 |     100 |                         
 snap/src/utils      |      72 |    42.85 |   68.75 |    72.6 |                         
  ecdsa.ts           |     100 |      100 |     100 |     100 |                         
  util.ts            |   43.33 |        0 |      50 |   42.85 | 15-27,37-49,105         
  validation.ts      |   89.47 |       80 |     100 |   89.47 | 49,81-84,88             
---------------------|---------|----------|---------|---------|-------------------------

---------

Co-authored-by: Gustavo Antunes <[email protected]>
ccharly added a commit to MetaMask/utils that referenced this pull request Mar 14, 2024
This adds some new helpers regarding CAIP-2 chain IDs.

This is in regard to the on-going work of adding those chain-agnostics
IDs into our Snap keyring implementations.

Initially those helpers were living on
https://github.com/MetaMask/eth-snap-keyring repository, but it feels
more natural to have them here. Moreover, we might use them elsewhere.

## Related

- MetaMask/eth-snap-keyring#231

---------

Co-authored-by: Elliot Winkler <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants