From afb3df9a8933e79385075186ab0a1fc495b10007 Mon Sep 17 00:00:00 2001 From: abretonc7s <107169956+abretonc7s@users.noreply.github.com> Date: Thu, 13 Jul 2023 00:03:31 +0800 Subject: [PATCH] feat: sdk protocol update (#6755) * feat: connection workflow improvement and prevent double goBack issue * feat: increase rpc timeout to 90s * chore: typing error * feat: update comm layer to 0.4.2 * feat: reset ready state on disconnect * feat: update comm layer to 0.5.0 * chore: remove extra logs * chore: comments * build: update to comm layer 0.5.0 --- app/core/SDKConnect/SDKConnect.ts | 129 +++++++++++++------------ app/core/SDKConnect/utils/wait.util.ts | 2 +- package.json | 12 +-- yarn.lock | 8 +- 4 files changed, 76 insertions(+), 75 deletions(-) diff --git a/app/core/SDKConnect/SDKConnect.ts b/app/core/SDKConnect/SDKConnect.ts index b7ba6da14bc..7f94be66779 100644 --- a/app/core/SDKConnect/SDKConnect.ts +++ b/app/core/SDKConnect/SDKConnect.ts @@ -213,7 +213,7 @@ export class Connection extends EventEmitter2 { this.setLoading(true); this.remote = new RemoteCommunication({ - platform: AppConstants.MM_SDK.PLATFORM, + platformType: AppConstants.MM_SDK.PLATFORM as 'metamask-mobile', communicationServerUrl: AppConstants.MM_SDK.SERVER_URL, communicationLayerPreference: CommunicationLayerPreference.SOCKET, otherPublicKey, @@ -258,6 +258,7 @@ export class Connection extends EventEmitter2 { this.initialConnection = false; this.otps = undefined; } + this.isReady = false; }); this.remote.on( @@ -287,6 +288,22 @@ export class Connection extends EventEmitter2 { this.approvalPromise = undefined; } + // Make sure we always have most up to date originatorInfo even if already ready. + if (!updatedOriginatorInfo) { + console.warn(`Connection - clients_ready - missing originatorInfo`); + return; + } + + this.originatorInfo = updatedOriginatorInfo; + updateOriginatorInfos({ + channelId: this.channelId, + originatorInfo: updatedOriginatorInfo, + }); + + if (this.isReady) { + return; + } + Logger.log( `Connection - clients_ready channel=${this.channelId} origin=${this.origin} initialConnection=${initialConnection} apiVersion=${apiVersion}`, updatedOriginatorInfo, @@ -296,11 +313,8 @@ export class Connection extends EventEmitter2 { !this.initialConnection && this.origin === AppConstants.DEEPLINKS.ORIGIN_QR_CODE ) { - // clear previous pending approval if (approvalController.get(this.channelId)) { - Logger.log( - `Connection - clients_ready - clearing previous pending approval`, - ); + // cleaning previous pending approval approvalController.reject( this.channelId, ethErrors.provider.userRejectedRequest(), @@ -347,21 +361,6 @@ export class Connection extends EventEmitter2 { this.sendAuthorized(true); } - // Make sure we only initialize the bridge when originatorInfo is received. - if (!updatedOriginatorInfo) { - return; - } - this.originatorInfo = updatedOriginatorInfo; - updateOriginatorInfos({ - channelId: this.channelId, - originatorInfo: updatedOriginatorInfo, - }); - - if (this.isReady) { - // Re-send otp message in case client didnd't receive disconnection. - return; - } - this.setupBridge(updatedOriginatorInfo); this.isReady = true; }, @@ -370,11 +369,7 @@ export class Connection extends EventEmitter2 { this.remote.on( EventType.MESSAGE, async (message: CommunicationLayerMessage) => { - // Wait for bridge to be ready before handling messages. - while (!this.isReady) { - await wait(1000); - } - + // TODO should probably handle this in a separate EventType.TERMINATE event. // handle termination message if (message.type === MessageType.TERMINATE) { // Delete connection from storage @@ -382,8 +377,9 @@ export class Connection extends EventEmitter2 { return; } - // ignore anything other than RPC methods. + // ignore anything other than RPC methods if (!message.method || !message.id) { + // ignore if it is protocol message console.warn( `Connection channel=${this.channelId} Invalid message format`, message, @@ -411,23 +407,22 @@ export class Connection extends EventEmitter2 { this.requestsToRedirect[message?.id] = true; } + // Wait for keychain to be unlocked before handling rpc calls. const keyringController = ( Engine.context as { KeyringController: KeyringController } ).KeyringController; await waitForKeychainUnlocked({ keyringController }); - // Check if channel is permitted + this.setLoading(false); + + // Wait for bridge to be ready before handling messages. + // It will wait until user accept/reject the connection request. try { await this.checkPermissions(message); - this.sendAuthorized(); - this.setLoading(false); - if (needsRedirect) { - // Special case for eth_requestAccount, doens't need to queue because it comes from apporval request. - this.rpcQueueManager.add({ - id: (message.id as string) ?? 'unknown', - method: message.method, - }); + while (!this.isReady) { + await wait(1000); } + this.sendAuthorized(); } catch (error) { // Approval failed - redirect to app with error. this.sendMessage({ @@ -442,6 +437,11 @@ export class Connection extends EventEmitter2 { return; } + this.rpcQueueManager.add({ + id: (message.id as string) ?? 'unknown', + method: message.method, + }); + // We have to implement this method here since the eth_sendTransaction in Engine is not working because we can't send correct origin if (message.method === 'eth_sendTransaction') { if ( @@ -509,7 +509,7 @@ export class Connection extends EventEmitter2 { // Always disconnect - this should not happen, DAPP should always init the connection. // A new channelId should be created after connection is removed. // On first launch reconnect is set to false even if there was a previous existing connection in another instance. - // To avoid hanging on the socket forever, we automatically close it after 5seconds. + // To avoid hanging on the socket forever, we automatically close it. this.removeConnection({ terminate: false }); }); } @@ -547,6 +547,7 @@ export class Connection extends EventEmitter2 { if (this.backgroundBridge) { return; } + this.backgroundBridge = new BackgroundBridge({ webview: null, isMMSDK: true, @@ -715,13 +716,12 @@ export class Connection extends EventEmitter2 { async sendMessage(msg: any) { const needsRedirect = this.requestsToRedirect[msg?.data?.id] !== undefined; - const rpcMethod = this.rpcQueueManager.getId(msg?.data?.id); - this.rpcQueueManager.remove(msg?.data?.id); + const queue = this.rpcQueueManager.get(); + + if (msg?.data?.id && queue[msg?.data?.id] !== undefined) { + this.rpcQueueManager.remove(msg?.data?.id); + } - Logger.log( - `Connection::sendMessage needsRedirect=${needsRedirect} rpcMethod=${rpcMethod}`, - msg, - ); this.remote.sendMessage(msg).catch((err) => { console.warn(`Connection::sendMessage failed to send`, err); }); @@ -734,26 +734,26 @@ export class Connection extends EventEmitter2 { if (this.origin === AppConstants.DEEPLINKS.ORIGIN_QR_CODE) return; - waitForEmptyRPCQueue(this.rpcQueueManager) - .then(async () => { - // Queue might not be empty if it timedout ---Always force clear before to go back - this.rpcQueueManager.reset(); + try { + await waitForEmptyRPCQueue(this.rpcQueueManager); + // Prevent double back issue android. (it seems that the app goes back randomly by itself) + if (wentBackMinimizer) { + // Skip, already went back. + return; + } - // Prevent double back issue android. (it seems that the app goes back randomly by itself) - if (wentBackMinimizer) { - // Skip, already went back. - return; - } + this.setLoading(false); + wentBackMinimizer = true; - this.setLoading(false); - Minimizer.goBack(); - }) - .catch((err) => { - console.warn( - `Connection::sendMessage error while waiting for empty rpc queue`, - err, - ); - }); + // Delay goback in order to display the feedback modal. + await wait(1000); + Minimizer.goBack(); + } catch (err) { + console.warn( + `Connection::sendMessage wait for empty rpc queue error`, + err, + ); + } } } @@ -1238,10 +1238,11 @@ export class SDKConnect extends EventEmitter2 { } this.paused = false; } else if (appState === 'background') { - // Reset wentBack state - wentBackMinimizer = true; - // Cancel rpc queue anytime app is backgrounded - this.rpcqueueManager.reset(); + // TODO: remove below comments but currently keep for reference in case android double back issue still happens. + // wentBackMinimizer = true; + // // Cancel rpc queue anytime app is backgrounded + // this.rpcqueueManager.reset(); + if (!this.paused) { /** * Pause connections after 20 seconds of the app being in background to respect device resources. diff --git a/app/core/SDKConnect/utils/wait.util.ts b/app/core/SDKConnect/utils/wait.util.ts index 2c2cc38b146..d5d56e076e5 100644 --- a/app/core/SDKConnect/utils/wait.util.ts +++ b/app/core/SDKConnect/utils/wait.util.ts @@ -18,7 +18,7 @@ export const waitForKeychainUnlocked = async ({ } }; -const MAX_QUEUE_LOOP = 50; // 50 seconds +const MAX_QUEUE_LOOP = 90; // seconds export const waitForEmptyRPCQueue = async (manager: RPCQueueManager) => { let i = 0; let queue = Object.keys(manager.get()); diff --git a/package.json b/package.json index f1a6b423bbb..b124c1c0941 100644 --- a/package.json +++ b/package.json @@ -168,7 +168,7 @@ "@metamask/permission-controller": "~3.0.0", "@metamask/phishing-controller": "^3.0.0", "@metamask/preferences-controller": "^2.1.0", - "@metamask/sdk-communication-layer": "0.3.0", + "@metamask/sdk-communication-layer": "^0.5.0", "@metamask/signature-controller": "^2.0.0", "@metamask/swaps-controller": "^6.8.0", "@metamask/transaction-controller": "4.0.0", @@ -333,11 +333,11 @@ "zxcvbn": "4.4.2" }, "devDependencies": { + "@actions/core": "^1.10.0", + "@actions/github": "^5.1.1", "@babel/core": "^7.20.0", "@babel/preset-env": "^7.20.0", "@babel/runtime": "^7.20.0", - "@actions/core": "^1.10.0", - "@actions/github": "^5.1.1", "@cucumber/message-streams": "^4.0.1", "@cucumber/messages": "^22.0.0", "@ethersproject/contracts": "^5.7.0", @@ -355,14 +355,14 @@ "@storybook/addon-ondevice-actions": "^5.3.23", "@storybook/addon-ondevice-knobs": "^5.3.25", "@storybook/react-native": "^5.3.25", - "@testing-library/react-hooks": "^8.0.1", "@testing-library/react": "14.0.0", + "@testing-library/react-hooks": "^8.0.1", "@testing-library/react-native": "12.1.2", "@types/enzyme": "^3.10.9", "@types/is-url": "^1.2.30", "@types/jest": "^27.2.0", - "@types/qs": "^6.9.7", "@types/node": "^17.0.21", + "@types/qs": "^6.9.7", "@types/react": "^17.0.11", "@types/react-native": "^0.64.10", "@types/react-native-background-timer": "^2.0.0", @@ -432,8 +432,8 @@ "regenerator-runtime": "0.13.9", "rn-nodeify": "10.3.0", "stack-beautifier": "1.0.2", - "typescript": "^4.4.2", "ts-node": "^10.5.0", + "typescript": "^4.4.2", "wdio-cucumberjs-json-reporter": "^4.4.3", "xml2js": "^0.5.0" }, diff --git a/yarn.lock b/yarn.lock index 37768bed461..86031491307 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5423,10 +5423,10 @@ resolved "https://registry.yarnpkg.com/@metamask/safe-event-emitter/-/safe-event-emitter-2.0.0.tgz#af577b477c683fad17c619a78208cede06f9605c" integrity sha512-/kSXhY692qiV1MXu6EeOZvg5nECLclxNXcKCxJ3cXQgYuRymRHpdx/t7JXfsK+JLjwA1e1c1/SBrlQYpusC29Q== -"@metamask/sdk-communication-layer@0.3.0": - version "0.3.0" - resolved "https://registry.yarnpkg.com/@metamask/sdk-communication-layer/-/sdk-communication-layer-0.3.0.tgz#ee7eac4f108bcf5d2ea0edfb9fedd97d1bf10360" - integrity sha512-Hubhl7nbXzsxUKCVZb0ZCoSqM9srAIUF3mZsoFvaGVeBRgRICmyutrjF/hyczgoQ4QDxgT8C13YgjJRXSdrdGQ== +"@metamask/sdk-communication-layer@^0.5.0": + version "0.5.0" + resolved "https://registry.yarnpkg.com/@metamask/sdk-communication-layer/-/sdk-communication-layer-0.5.0.tgz#a2333ab1f0d96c52aed9f749e7539e9e927f53db" + integrity sha512-lcA9KpOM332oPhFZVsdeJYF1OJAQP7LmHCSrCOyBOLrvgaRhNxCSsgMid9shGuN1PVozDPEQXljRvJQOyfRpsg== dependencies: cross-fetch "^3.1.5" date-fns "^2.29.3"