Skip to content

Commit

Permalink
fix: use hostname to fetch approvedhosts (#7489)
Browse files Browse the repository at this point in the history
## **Description**
This PR fixes an Android SDK bug whereby:
- The wallet was always returning the first permitted account instead of
the selected account when connecting the SDK to MetaMask
- Changing the selected account on MetaMask did not fire a
`metamask_accountsChanged` event so the SDK did not get selected account
updated.

This change implements the `getApprovedHosts` to fix this issue. 

## **Manual testing steps**

_1. Have more than one address in your metamask account and select any
address as active address
_2. Connect from Android Metamask SDK and ensure the active account is
the selected address on the SDK
_3. Switch to another account on the wallet, and ensure that the SDK
updates to the new address

## **Related issues**

_Fixes #???_

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained:
  - [x] What problem this PR is solving.
  - [x] How this problem was solved.
  - [x] How reviewers can test my changes.
- [ ] I’ve indicated what issue this PR is linked to: Fixes #???
- [ ] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [ ] 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)).
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] 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.
  • Loading branch information
elefantel authored Oct 17, 2023
1 parent 9a37d5a commit 13130e3
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 9 deletions.
7 changes: 6 additions & 1 deletion app/core/BackgroundBridge/BackgroundBridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,12 @@ export class BackgroundBridge extends EventEmitter {

notifySelectedAddressChanged(selectedAddress) {
if (this.isRemoteConn) {
if (!this.getApprovedHosts?.()?.[this.remoteConnHost]) return;
// Pass the remoteConnHost to getApprovedHosts as AndroidSDK requires it
if (
!this.getApprovedHosts?.(this.remoteConnHost)?.[this.remoteConnHost]
) {
return;
}
}
this.sendNotification({
method: NOTIFICATION_NAMES.accountsChanged,
Expand Down
4 changes: 2 additions & 2 deletions app/core/RPCMethods/RPCMethodMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ export const getRpcMethodMiddleware = ({
const getAccounts = (): string[] => {
const selectedAddress =
Engine.context.PreferencesController.state.selectedAddress?.toLowerCase();
const isEnabled = isWalletConnect || getApprovedHosts()[hostname];
const approvedHosts = getApprovedHosts(hostname) || {};
const isEnabled = isWalletConnect || approvedHosts[hostname];
return isEnabled && selectedAddress ? [selectedAddress] : [];
};

Expand Down Expand Up @@ -907,5 +908,4 @@ export const getRpcMethodMiddleware = ({
}
await rpcMethods[req.method]();
});

export default getRpcMethodMiddleware;
16 changes: 10 additions & 6 deletions app/core/SDKConnect/AndroidSDK/AndroidService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,11 @@ export default class AndroidService extends EventEmitter2 {

try {
if (!this.connectedClients?.[clientInfo.clientId]) {
await this.requestApproval(clientInfo);
this.setupBridge(clientInfo);
// Save session to SDKConnect
SDKConnect.getInstance().addAndroidConnection({
id: clientInfo.clientId,
lastAuthorized: Date.now(),
origin: AppConstants.MM_SDK.ANDROID_SDK,
originatorInfo: clientInfo.originatorInfo,
otherPublicKey: '',
Expand Down Expand Up @@ -331,11 +331,13 @@ export default class AndroidService extends EventEmitter2 {

const bridge = new BackgroundBridge({
webview: null,
isMMSDK: false,
isMMSDK: true,
url: PROTOCOLS.METAMASK + '://' + AppConstants.MM_SDK.SDK_REMOTE_ORIGIN,
isRemoteConn: true,
sendMessage: this.sendMessage.bind(this),
getApprovedHosts: () => false,
getApprovedHosts: (host: string) => ({
[host]: true,
}),
remoteConnHost:
clientInfo.originatorInfo.url ?? clientInfo.originatorInfo.title,
getRpcMethodMiddleware: ({
Expand All @@ -348,10 +350,12 @@ export default class AndroidService extends EventEmitter2 {
hostname:
clientInfo.originatorInfo.url ?? clientInfo.originatorInfo.title,
getProviderState,
isMMSDK: false,
isMMSDK: true,
navigation: null, //props.navigation,
getApprovedHosts: () => ({}),
setApprovedHosts: () => ({}),
getApprovedHosts: (host: string) => ({
[host]: true,
}),
setApprovedHosts: () => true,
approveHost: () => ({}),
// Website info
url: {
Expand Down

0 comments on commit 13130e3

Please sign in to comment.