Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: sdk connection issues #7102

Merged
merged 11 commits into from
Sep 4, 2023
Merged

fix: sdk connection issues #7102

merged 11 commits into from
Sep 4, 2023

Conversation

abretonc7s
Copy link
Contributor

@abretonc7s abretonc7s commented Sep 1, 2023

Description

  • Fixes session error on when reconnecting on existing session (ios).
  • Also fixed indirect issue linked to connection stability.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Patch coverage: 15.21% and project coverage change: -0.02% ⚠️

Comparison is base (4a224c5) 32.86% compared to head (0506c46) 32.84%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7102      +/-   ##
==========================================
- Coverage   32.86%   32.84%   -0.02%     
==========================================
  Files        1001     1001              
  Lines       26743    26768      +25     
  Branches     2101     2107       +6     
==========================================
+ Hits         8788     8793       +5     
- Misses      17535    17555      +20     
  Partials      420      420              
Files Changed Coverage Δ
...onents/Views/SDKSessionsManager/SDKSessionItem.tsx 9.52% <0.00%> (-3.81%) ⬇️
app/core/DeeplinkManager.js 1.27% <0.00%> (-0.01%) ⬇️
app/core/SDKConnect/SDKConnect.ts 2.17% <4.16%> (-0.04%) ⬇️
app/core/SDKConnect/utils/wait.util.ts 17.50% <25.00%> (-0.69%) ⬇️
app/util/confirmation/signatureUtils.js 92.30% <100.00%> (+1.39%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

omridan159
omridan159 previously approved these changes Sep 1, 2023
elefantel
elefantel previously approved these changes Sep 1, 2023
Copy link
Contributor

@elefantel elefantel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@abretonc7s abretonc7s dismissed stale reviews from elefantel and omridan159 via 80a19a1 September 1, 2023 14:40
@abretonc7s abretonc7s changed the title feat: sdk updates fix: sdk connection issues Sep 1, 2023
@abretonc7s abretonc7s marked this pull request as ready for review September 1, 2023 15:04
@abretonc7s abretonc7s requested a review from a team as a code owner September 1, 2023 15:04
@abretonc7s abretonc7s added the team-sdk SDK team label Sep 1, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

15.3% 15.3% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@christopherferreira9 christopherferreira9 added the QA in Progress QA has started on the feature. label Sep 1, 2023
Copy link
Contributor

@elefantel elefantel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@christopherferreira9
Copy link
Contributor

Context:
in the flow Connect Dapp -> Approve connection in MMM -> Kill MMM -> Tap sign, the user was being prompt with a connect request instead of the sining prompt. This made that the user would need to approve the connection he did before making it be misleading into making the user think it is a new connection.
The second issue with this is that if the user taps cancel instead of connect in this resuming prompt the connection would be deleted and the Dapp would never know that so the user gets into a stale state.
The third issue occurs with the Signing feedback mentioned here that was indirectly caused by this reconnection flow making the appearance of this toaster forever hidden.

Error Video:

before_bug.mov

Acceptance Criteria:

  • After killing the MetaMask Wallet you're able to resume with sign (meaning that the user is able to tap sign on Dapp and get deeplinked to MetaMask)
  • After the MetaMask Wallet resumes the user is prompt with the sign approval
  • Approving the sign request presents the success message in MetaMask
  • Approving the sign request presents the rejection message in MetaMask

SDKs and Dapps tested:

  • Android SDK
  • Unity SDK
  • iOS SDK
  • Javascript SDK: Create-React-App; NextJS; ReactNative; ElectronJS; NodeJS

Video Fix for sign on resume:
Uploading ios_fix.mov…

Video fix for the signing feedback:

Screen.Recording.2023-09-04.at.11.27.46.mov

@christopherferreira9 christopherferreira9 removed the QA in Progress QA has started on the feature. label Sep 4, 2023
@christopherferreira9 christopherferreira9 added the QA Passed A successful QA run through has been done label Sep 4, 2023
@christopherferreira9 christopherferreira9 merged commit 02e4ed6 into main Sep 4, 2023
17 of 18 checks passed
@christopherferreira9 christopherferreira9 deleted the feat/sdk-update branch September 4, 2023 10:35
@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2023
@metamaskbot metamaskbot added the release-7.8.0 Issue or pull request that will be included in release 7.8.0 label Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.8.0 Issue or pull request that will be included in release 7.8.0 team-sdk SDK team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants