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] Improves handling of missing WCv2 Project ID #6587

Merged
merged 5 commits into from
Jul 11, 2023

Conversation

andreahaku
Copy link
Member

@andreahaku andreahaku commented Jun 13, 2023

Improves handling of missing WCv2 Project ID.

This is to make sure that developers without a WalletConnect v2 Project ID can still work on MetaMask Mobile app in a stable way.

To QA test this we just need to make sure that the app works (a part from WalletConnect related) without problems. This is mainly for developers not having the chance to use a WCv2 project ID that is required for the app to work with WC.

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #???

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@andreahaku andreahaku marked this pull request as ready for review June 13, 2023 14:52
@andreahaku andreahaku requested a review from a team as a code owner June 13, 2023 14:52
@andreahaku andreahaku self-assigned this Jun 13, 2023
@andreahaku andreahaku added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Code Impact - Low Minor code change that can safely applied to the codebase Priority - High Task with high priority WalletConnect WalletConnect related issue or bug labels Jun 13, 2023
digiwand
digiwand previously approved these changes Jun 13, 2023
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

LGTM!

app/core/WalletConnect/WalletConnectV2.ts Show resolved Hide resolved
@andreahaku andreahaku added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jun 14, 2023
sethkfman
sethkfman previously approved these changes Jun 14, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@shanejonas
Copy link
Contributor

👍

@andreahaku andreahaku dismissed stale reviews from sethkfman and digiwand via 28cb1e0 July 7, 2023 12:44
@andreahaku andreahaku force-pushed the fix/improved_wcv2_missing_projectid branch from 71f27d6 to 28cb1e0 Compare July 7, 2023 12:44
@christopherferreira9 christopherferreira9 added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 7, 2023
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@christopherferreira9 christopherferreira9 added QA in Progress QA has started on the feature. QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA in Progress QA has started on the feature. labels Jul 7, 2023
@christopherferreira9
Copy link
Contributor

Looking good 🚀

@christopherferreira9 christopherferreira9 removed the request for review from sethkfman July 7, 2023 15:09
@christopherferreira9 christopherferreira9 removed the request for review from digiwand July 7, 2023 15:09
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 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.

@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 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 0 Code Smells

24.0% 24.0% Coverage
0.0% 0.0% Duplication

@christopherferreira9 christopherferreira9 merged commit 33b2e33 into main Jul 11, 2023
11 checks passed
@christopherferreira9 christopherferreira9 deleted the fix/improved_wcv2_missing_projectid branch July 11, 2023 16:29
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Code Impact - Low Minor code change that can safely applied to the codebase Priority - High Task with high priority QA Passed A successful QA run through has been done release-7.4.0 WalletConnect WalletConnect related issue or bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants