Skip to content

feat!: use Messenger instead of SnapsController#152

Merged
ccharly merged 3 commits intomainfrom
feat/use-messenger
Jan 21, 2025
Merged

feat!: use Messenger instead of SnapsController#152
ccharly merged 3 commits intomainfrom
feat/use-messenger

Conversation

@ccharly
Copy link
Contributor

@ccharly ccharly commented Jan 20, 2025 •
edited
Loading

Introducing the concept of Messenger (messaging system that we use in our core controllers) to break the runtime dependency we have with the SnapController.

We will also use this new Messenger later when the Snap keyring will re-forward some Snap account events to some other controllers.

This is BREAKING because of the removal of the KeyringSnapControllerClient and because the SnapKeyring's constructor now requires a Messenger object instead of the SnapController.

@ccharly ccharly force-pushed the feat/use-messenger branch 2 times, most recently from 763c88d to 8d2b166 Compare January 20, 2025 17:43
@ccharly ccharly force-pushed the feat/use-messenger branch from 8d2b166 to e3085e0 Compare January 20, 2025 20:35
@ccharly ccharly marked this pull request as ready for review January 21, 2025 11:16
@ccharly ccharly requested a review from a team as a code owner January 21, 2025 11:16
@ccharly ccharly changed the title feat!: use Messenger instead of SnapsController feat!: use Messenger instead of SnapsController Jan 21, 2025
@ccharly ccharly added this pull request to the merge queue Jan 21, 2025
Merged via the queue into main with commit 2bdc87a Jan 21, 2025
29 checks passed
@ccharly ccharly deleted the feat/use-messenger branch January 21, 2025 16:50
@owencraston owencraston mentioned this pull request Feb 13, 2025
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-mobile that referenced this pull request Feb 13, 2025
@owencraston
chore: Update accounts controller to v23 and related packages (transa…
42263b2 
…ction controller etc) (#13436)

<!--
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**

This PR updates the
[AccountsController](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
and the
[TransactionController](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510).
These are the two major updates however there are a few smaller peer dep
updates as well. Here is a description of the changes...

### "@metamask/accounts-controller": "^21.0.0" -> 23.1.0"
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
- Breaking changes:
- BREAKING: Now requires
SnapKeyring:account{AssetList,Balances,Transactions}Updated events to be
registered on the messenger (MetaMask/core#5190)
- BREAKING: Bump @metamask/snaps-controllers peer dependency from ^9.7.0
to ^9.19.0 (MetaMask/core#5265)
    - Bump @metamask/base-controller from ^7.1.1 to ^8.0.0
- Non breaking changes:
- Bump @metamask/keyring-api" from ^16.1.0 to ^17.0.0
(MetaMask/core#5280)
- Bump @metamask/eth-snap-keyring from ^9.1.1 to ^10.0.0
(MetaMask/core#5280)
- Bump @metamask/snaps-sdk from ^6.7.0 to ^6.17.1
(MetaMask/core#5220),
(MetaMask/core#5265)
- Bump @metamask/snaps-utils from ^8.9.0 to ^8.10.0
(MetaMask/core#5265)

### "@metamask/transaction-controller": "^43.0.0" -> 45.1.0
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510)
- Breaking changes:
- BREAKING: Bump @metamask/accounts-controller peer dependency from
^22.0.0 to ^23.0.0

### "@metamask/keyring-api": "^13.0.0" -> 17.0.0
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-api/CHANGELOG.md#1700)
- Breaking changes:
- BREAKING: Make CaipAssetType type more restritive
(MetaMask/accounts#150) It used to be a string
but it has been restricted with a template literal type that matches
CAIP-19 asset type.
- BREAKING: Make specific *AccountStruct.scopes more strict
(MetaMask/accounts#159)
- BREAKING: Remove CAIP redefinitions
(MetaMask/accounts#167) We now rely on CAIP
definitions coming @metamask/utils.
- BREAKING: Enforce that scopes contains CAIP-2 chain IDs
(MetaMask/accounts#165) Initially scopes
accepted CAIP-2 namespaces as well to address the EVM EOA accounts that
supports all EVM chains. This has been dropped in favor of eip155:0
scope.
- BREAKING: Rename *Scopes enums to *Scope
(MetaMask/accounts#165)
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186) This was missing from
SIP-26, but we expect this address to be CAIP-10 compliant.

### "@metamask/keyring-internal-api": "^2.0.0" -> 4.0.2
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-internal-api/CHANGELOG.md#402)
- Breaking changes
- BREAKING: Bump @metamask/keyring-api from ^14.0.0 to ^15.0.0
(MetaMask/accounts#160) The scopes from each
*AccountStruct types is now more strict which impact all
Internal*AccountStruct types.
- BREAKING: Bump @metamask/keyring-api from ^15.0.0 to ^16.0.0
(MetaMask/accounts#172)
The scopes from each *AccountStruct types is now more strict (remove
support of CAIP-2 namespaces) which impact all Internal*AccountStruct
types.
- Bump @metamask/keyring-api from ^16.1.0 to ^17.0.0
(MetaMask/accounts#192)

### "@metamask/eth-snap-keyring": "^7.1.0" -> 10.0.0
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-bridge/CHANGELOG.md)
- Breaking changes:
- BREAKING: Bump @metamask/keyring-api from ^12.0.0 to ^13.0.0
(MetaMask/accounts#101)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount and InternalAccount have a new required field (scopes)
and are part of the public API.
- BREAKING: Bump @metamask/keyring-internal-api from ^1.0.0 to ^2.0.0
(MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
InternalAccount extends KeyringAccount which has a new required field
(scopes) and is part of the public API.
- BREAKING: Bump @metamask/keyring-snap-internal-client from ^1.0.0 to
^2.0.0 (MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount has a new required field (scopes) and is part of the
public API.
- BREAKING: Use Messenger instead of SnapsController
(MetaMask/accounts#152)
This allows to break the runtime dependency we had with some snaps-*
pacakges.
- BREAKING: Make scopes more strict
(MetaMask/accounts#159)
We now use specific *AccountStucts when checking created/updated
accounts to make the scopes sent by the Snap are valid regarding their
account type definition.
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186)
This was missing from SIP-26, but we expect this address to be CAIP-10
compliant.

### "@metamask/snaps-sdk": "^6.13.0" -> 6.17.1
-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-sdk/CHANGELOG.md#6171)
- no breaking changes

### "@metamask/snaps-utils": "^8.6.1" -> ^8.10.0
-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-utils/CHANGELOG.md#8100)
- no breaking changes

## **Related issues**

Fixes: #13374
Fixes: #13377

## **Manual testing steps**

#### Testing snap accounts
1. ensure you are building metamask flask by setting the value of
`METAMASK_BUILD_TYPE` to `flask` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios
5. create/import a wallet
6. click on the selected account at the top of the home page
7. click on the Add account button
8. then click the Add Solana account (Beta) button
9. a popup with a suggested name should appeaer
10. click OK, the account should now be added to your account list. if
you have a balance on this address then it should show in the portfolio
view.
11. repeat the above steps with the Bitcoin and Bitcoin testnet account

#### Testing transactions
1. ensure you are NOT building flask by setting the value of
`METAMASK_BUILD_TYPE` to `main` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios 
5. import an account that has funds
6. click on the selected account at the top of the screen and the
account list should open
7. then click on the add account button, then click add account
8. a new eth account should appear in your wallet
9. click on main bottom tab navigator icon which should open a list of
actions, select Send.
10. the Send flow should open.
11. set your from account to the account that has funds and the to
account to your newly created account.
12. type in a valid amount then go through the rest of the send flow
13. in the end the transaction should go through.
14. the account balances on the home page should be updated

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

On-boarding and Account Creation Flow


https://github.com/user-attachments/assets/2ccd8fb4-4857-4a99-ab52-cdb071bd0447

Send flow


https://github.com/user-attachments/assets/da81d49c-b472-49a7-83c4-1d1046521072


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
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.
christopherferreira9 pushed a commit to MetaMask/metamask-mobile that referenced this pull request Feb 14, 2025
@owencraston @christopherferreira9
chore: Update accounts controller to v23 and related packages (transa…
ddf2fd4 
…ction controller etc) (#13436)

<!--
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**

This PR updates the
[AccountsController](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
and the
[TransactionController](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510).
These are the two major updates however there are a few smaller peer dep
updates as well. Here is a description of the changes...

### "@metamask/accounts-controller": "^21.0.0" -> 23.1.0"
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
- Breaking changes:
- BREAKING: Now requires
SnapKeyring:account{AssetList,Balances,Transactions}Updated events to be
registered on the messenger (MetaMask/core#5190)
- BREAKING: Bump @metamask/snaps-controllers peer dependency from ^9.7.0
to ^9.19.0 (MetaMask/core#5265)
    - Bump @metamask/base-controller from ^7.1.1 to ^8.0.0
- Non breaking changes:
- Bump @metamask/keyring-api" from ^16.1.0 to ^17.0.0
(MetaMask/core#5280)
- Bump @metamask/eth-snap-keyring from ^9.1.1 to ^10.0.0
(MetaMask/core#5280)
- Bump @metamask/snaps-sdk from ^6.7.0 to ^6.17.1
(MetaMask/core#5220),
(MetaMask/core#5265)
- Bump @metamask/snaps-utils from ^8.9.0 to ^8.10.0
(MetaMask/core#5265)

### "@metamask/transaction-controller": "^43.0.0" -> 45.1.0
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510)
- Breaking changes:
- BREAKING: Bump @metamask/accounts-controller peer dependency from
^22.0.0 to ^23.0.0

### "@metamask/keyring-api": "^13.0.0" -> 17.0.0
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-api/CHANGELOG.md#1700)
- Breaking changes:
- BREAKING: Make CaipAssetType type more restritive
(MetaMask/accounts#150) It used to be a string
but it has been restricted with a template literal type that matches
CAIP-19 asset type.
- BREAKING: Make specific *AccountStruct.scopes more strict
(MetaMask/accounts#159)
- BREAKING: Remove CAIP redefinitions
(MetaMask/accounts#167) We now rely on CAIP
definitions coming @metamask/utils.
- BREAKING: Enforce that scopes contains CAIP-2 chain IDs
(MetaMask/accounts#165) Initially scopes
accepted CAIP-2 namespaces as well to address the EVM EOA accounts that
supports all EVM chains. This has been dropped in favor of eip155:0
scope.
- BREAKING: Rename *Scopes enums to *Scope
(MetaMask/accounts#165)
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186) This was missing from
SIP-26, but we expect this address to be CAIP-10 compliant.

### "@metamask/keyring-internal-api": "^2.0.0" -> 4.0.2
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-internal-api/CHANGELOG.md#402)
- Breaking changes
- BREAKING: Bump @metamask/keyring-api from ^14.0.0 to ^15.0.0
(MetaMask/accounts#160) The scopes from each
*AccountStruct types is now more strict which impact all
Internal*AccountStruct types.
- BREAKING: Bump @metamask/keyring-api from ^15.0.0 to ^16.0.0
(MetaMask/accounts#172)
The scopes from each *AccountStruct types is now more strict (remove
support of CAIP-2 namespaces) which impact all Internal*AccountStruct
types.
- Bump @metamask/keyring-api from ^16.1.0 to ^17.0.0
(MetaMask/accounts#192)

### "@metamask/eth-snap-keyring": "^7.1.0" -> 10.0.0
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-bridge/CHANGELOG.md)
- Breaking changes:
- BREAKING: Bump @metamask/keyring-api from ^12.0.0 to ^13.0.0
(MetaMask/accounts#101)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount and InternalAccount have a new required field (scopes)
and are part of the public API.
- BREAKING: Bump @metamask/keyring-internal-api from ^1.0.0 to ^2.0.0
(MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
InternalAccount extends KeyringAccount which has a new required field
(scopes) and is part of the public API.
- BREAKING: Bump @metamask/keyring-snap-internal-client from ^1.0.0 to
^2.0.0 (MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount has a new required field (scopes) and is part of the
public API.
- BREAKING: Use Messenger instead of SnapsController
(MetaMask/accounts#152)
This allows to break the runtime dependency we had with some snaps-*
pacakges.
- BREAKING: Make scopes more strict
(MetaMask/accounts#159)
We now use specific *AccountStucts when checking created/updated
accounts to make the scopes sent by the Snap are valid regarding their
account type definition.
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186)
This was missing from SIP-26, but we expect this address to be CAIP-10
compliant.

### "@metamask/snaps-sdk": "^6.13.0" -> 6.17.1
-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-sdk/CHANGELOG.md#6171)
- no breaking changes

### "@metamask/snaps-utils": "^8.6.1" -> ^8.10.0
-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-utils/CHANGELOG.md#8100)
- no breaking changes

## **Related issues**

Fixes: #13374
Fixes: #13377

## **Manual testing steps**

#### Testing snap accounts
1. ensure you are building metamask flask by setting the value of
`METAMASK_BUILD_TYPE` to `flask` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios
5. create/import a wallet
6. click on the selected account at the top of the home page
7. click on the Add account button
8. then click the Add Solana account (Beta) button
9. a popup with a suggested name should appeaer
10. click OK, the account should now be added to your account list. if
you have a balance on this address then it should show in the portfolio
view.
11. repeat the above steps with the Bitcoin and Bitcoin testnet account

#### Testing transactions
1. ensure you are NOT building flask by setting the value of
`METAMASK_BUILD_TYPE` to `main` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios 
5. import an account that has funds
6. click on the selected account at the top of the screen and the
account list should open
7. then click on the add account button, then click add account
8. a new eth account should appear in your wallet
9. click on main bottom tab navigator icon which should open a list of
actions, select Send.
10. the Send flow should open.
11. set your from account to the account that has funds and the to
account to your newly created account.
12. type in a valid amount then go through the rest of the send flow
13. in the end the transaction should go through.
14. the account balances on the home page should be updated

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

On-boarding and Account Creation Flow


https://github.com/user-attachments/assets/2ccd8fb4-4857-4a99-ab52-cdb071bd0447

Send flow


https://github.com/user-attachments/assets/da81d49c-b472-49a7-83c4-1d1046521072


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
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.
MarioAslau pushed a commit to MetaMask/metamask-mobile that referenced this pull request Feb 14, 2025
@owencraston @MarioAslau
chore: Update accounts controller to v23 and related packages (transa…
f9e1dd1 
…ction controller etc) (#13436)

<!--
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.
-->

This PR updates the
[AccountsController](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
and the
[TransactionController](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510).
These are the two major updates however there are a few smaller peer dep
updates as well. Here is a description of the changes...

-
[changelog](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2310)
- Breaking changes:
- BREAKING: Now requires
SnapKeyring:account{AssetList,Balances,Transactions}Updated events to be
registered on the messenger (MetaMask/core#5190)
- BREAKING: Bump @metamask/snaps-controllers peer dependency from ^9.7.0
to ^9.19.0 (MetaMask/core#5265)
    - Bump @metamask/base-controller from ^7.1.1 to ^8.0.0
- Non breaking changes:
- Bump @metamask/keyring-api" from ^16.1.0 to ^17.0.0
(MetaMask/core#5280)
- Bump @metamask/eth-snap-keyring from ^9.1.1 to ^10.0.0
(MetaMask/core#5280)
- Bump @metamask/snaps-sdk from ^6.7.0 to ^6.17.1
(MetaMask/core#5220),
(MetaMask/core#5265)
- Bump @metamask/snaps-utils from ^8.9.0 to ^8.10.0
(MetaMask/core#5265)

-
[changelog](https://github.com/MetaMask/core/blob/main/packages/transaction-controller/CHANGELOG.md#4510)
- Breaking changes:
- BREAKING: Bump @metamask/accounts-controller peer dependency from
^22.0.0 to ^23.0.0

-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-api/CHANGELOG.md#1700)
- Breaking changes:
- BREAKING: Make CaipAssetType type more restritive
(MetaMask/accounts#150) It used to be a string
but it has been restricted with a template literal type that matches
CAIP-19 asset type.
- BREAKING: Make specific *AccountStruct.scopes more strict
(MetaMask/accounts#159)
- BREAKING: Remove CAIP redefinitions
(MetaMask/accounts#167) We now rely on CAIP
definitions coming @metamask/utils.
- BREAKING: Enforce that scopes contains CAIP-2 chain IDs
(MetaMask/accounts#165) Initially scopes
accepted CAIP-2 namespaces as well to address the EVM EOA accounts that
supports all EVM chains. This has been dropped in favor of eip155:0
scope.
- BREAKING: Rename *Scopes enums to *Scope
(MetaMask/accounts#165)
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186) This was missing from
SIP-26, but we expect this address to be CAIP-10 compliant.

-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-internal-api/CHANGELOG.md#402)
- Breaking changes
- BREAKING: Bump @metamask/keyring-api from ^14.0.0 to ^15.0.0
(MetaMask/accounts#160) The scopes from each
*AccountStruct types is now more strict which impact all
Internal*AccountStruct types.
- BREAKING: Bump @metamask/keyring-api from ^15.0.0 to ^16.0.0
(MetaMask/accounts#172)
The scopes from each *AccountStruct types is now more strict (remove
support of CAIP-2 namespaces) which impact all Internal*AccountStruct
types.
- Bump @metamask/keyring-api from ^16.1.0 to ^17.0.0
(MetaMask/accounts#192)

-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-bridge/CHANGELOG.md)
- Breaking changes:
- BREAKING: Bump @metamask/keyring-api from ^12.0.0 to ^13.0.0
(MetaMask/accounts#101)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount and InternalAccount have a new required field (scopes)
and are part of the public API.
- BREAKING: Bump @metamask/keyring-internal-api from ^1.0.0 to ^2.0.0
(MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
InternalAccount extends KeyringAccount which has a new required field
(scopes) and is part of the public API.
- BREAKING: Bump @metamask/keyring-snap-internal-client from ^1.0.0 to
^2.0.0 (MetaMask/accounts#135)
This change was not properly reported as breaking on the 7.1.0.
KeyringAccount has a new required field (scopes) and is part of the
public API.
- BREAKING: Use Messenger instead of SnapsController
(MetaMask/accounts#152)
This allows to break the runtime dependency we had with some snaps-*
pacakges.
- BREAKING: Make scopes more strict
(MetaMask/accounts#159)
We now use specific *AccountStucts when checking created/updated
accounts to make the scopes sent by the Snap are valid regarding their
account type definition.
- BREAKING: Use CaipAccountId for ResolvedAccountAddress.address
(MetaMask/accounts#186)
This was missing from SIP-26, but we expect this address to be CAIP-10
compliant.

-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-sdk/CHANGELOG.md#6171)
- no breaking changes

-
[changelog](https://github.com/MetaMask/snaps/blob/main/packages/snaps-utils/CHANGELOG.md#8100)
- no breaking changes

Fixes: #13374
Fixes: #13377

1. ensure you are building metamask flask by setting the value of
`METAMASK_BUILD_TYPE` to `flask` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios
5. create/import a wallet
6. click on the selected account at the top of the home page
7. click on the Add account button
8. then click the Add Solana account (Beta) button
9. a popup with a suggested name should appeaer
10. click OK, the account should now be added to your account list. if
you have a balance on this address then it should show in the portfolio
view.
11. repeat the above steps with the Bitcoin and Bitcoin testnet account

1. ensure you are NOT building flask by setting the value of
`METAMASK_BUILD_TYPE` to `main` in your `.js.env`
2. source .js.env
3. yarn setup
4. yarn start:ios
5. import an account that has funds
6. click on the selected account at the top of the screen and the
account list should open
7. then click on the add account button, then click add account
8. a new eth account should appear in your wallet
9. click on main bottom tab navigator icon which should open a list of
actions, select Send.
10. the Send flow should open.
11. set your from account to the account that has funds and the to
account to your newly created account.
12. type in a valid amount then go through the rest of the send flow
13. in the end the transaction should go through.
14. the account balances on the home page should be updated

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

On-boarding and Account Creation Flow

https://github.com/user-attachments/assets/2ccd8fb4-4857-4a99-ab52-cdb071bd0447

Send flow

https://github.com/user-attachments/assets/da81d49c-b472-49a7-83c4-1d1046521072

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
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.

- [ ] 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.
@cloudonshore cloudonshore mentioned this pull request Jan 29, 2026
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 30, 2026
@cloudonshore
fix(keyring-eth-ledger-bridge): normalize signature v value for prope…
f4f52c8 
…r recovery (#449)

## Summary

Ledger devices may return signature `v` as 0 or 1 (modern format), but
Ethereum's `ecrecover` expects 27 or 28. This causes `"The signature
doesnt match the right address"` errors in
[`signPersonalMessage`](https://github.com/MetaMask/accounts/blob/main/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts#L475)
and
[`signTypedData`](https://github.com/MetaMask/accounts/blob/main/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts#L475).

## Fix

```typescript
if (recoveryParam === 0 || recoveryParam === 1) {
  recoveryParam += 27;
}
```

## Why This Wasn't Fixed Before

**Original bug (pre-2022):** The code *subtracted* 27 from `v`:
```javascript
let v = payload.v - 27  // WRONG
```
This converted correct values (27/28) into incorrect ones (0/1).

**PR #152 (August 2022):** Changed to pass through the raw value:
```javascript
let v = parseInt(payload.v, 10)  // Just pass through
```
This fixed the case where Ledger returns 27/28, but assumed Ledger
*always* returns 27/28.

**The gap:** Ledger devices may return 0/1 depending on firmware,
signing method, or device model. PR #152 didn't normalize these values.

**Issue #10850 was closed prematurely** in January 2024 with "I expect
this was resolved in PR #152" — but PR #152 only fixed one direction of
the problem.

## References

This is a well-known issue with a standard fix across the ecosystem:

- [Frame
wallet](https://github.com/floating/frame/blob/0.6.10/main/provider/helpers.ts#L10)
- `v = v === 0 || v === 1 ? v + 27 : v`
- [Rabby
wallet](https://github.com/RabbyHub/Rabby/blob/develop/src/ui/utils/gnosis.ts)
- *"Metamask with ledger returns V=0/1 here too"*
- [Gnosis Safe
Android](https://github.com/safe-global/safe-android/blob/main/app/src/main/java/io/gnosis/safe/ui/transactions/details/viewmodel/TxReviewViewModel.kt)
- `if (v <= 1) v += 27`
- Related issues:
[eth-ledger-bridge-keyring#134](MetaMask/eth-ledger-bridge-keyring#134),
[eth-ledger-bridge-keyring#151](MetaMask/eth-ledger-bridge-keyring#151),
[metamask-extension#10850](MetaMask/metamask-extension#10850)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Changes signature formatting in `signPersonalMessage` and
`signTypedData`, which can affect signature verification and downstream
consumers if assumptions about `v` differ. Scope is small and covered by
new unit tests for multiple `v` cases.
> 
> **Overview**
> Fixes Ledger signing to **normalize the recovery param (`v`)** so
devices returning modern values `0/1` are converted to legacy `27/28`
before building the final signature in `ledger-keyring.ts`.
> 
> Adds focused tests covering `v` normalization for both
`signPersonalMessage` and `signTypedData`, and updates the
`CHANGELOG.md` entry accordingly (plus minor Jest coverage threshold
tweaks).
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
e756502. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danroc danroc danroc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ccharly @danroc