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: wc2 invalid origin in analytics #7474

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Conversation

abretonc7s
Copy link
Contributor

@abretonc7s abretonc7s commented Oct 11, 2023

Description

Analytics sending invalid request source on wallet connect transaction because the origin wasn't parsed correctly.

fixes https://app.zenhub.com/workspaces/metamask-sdk-6359297a36f20292c13feca0/issues/gh/metamask/metamask-sdk/269

Manual testing steps

Use wallet connect v2 dapp to send a transaction to mobile wallet. https://github.com/WalletConnect/walletconnect-example-dapp

Before

 LOG  [MetaMask DEBUG]: Analytics 'trackEventWithParameters' - {"category": "Dapp Transaction Cancelled"} {"account_type": "MetaMask", "active_currency": "ETH", "asset_type": "ETH", "chain_id": "5", "dapp_host_name": "http://localhost:3000", "gas_estimate_type": undefined, "gas_mode": "Advanced", "request_source": "In-App-Browser", "speed_set": undefined} undefined undefined

After

 LOG  [MetaMask DEBUG]: Analytics 'trackEventWithParameters' - {"category": "Dapp Transaction Cancelled"} {"account_type": "MetaMask", "active_currency": "ETH", "asset_type": "ETH", "chain_id": "5", "dapp_host_name": "wc::http://localhost:3000", "gas_estimate_type": undefined, "gas_mode": "Advanced", "request_source": "WalletConnect", "speed_set": undefined} undefined undefined

@abretonc7s abretonc7s requested a review from a team as a code owner October 11, 2023 08:10
@github-actions
Copy link
Contributor

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.

@abretonc7s abretonc7s added needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-sdk SDK team WalletConnect WalletConnect related issue or bug labels Oct 11, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ad05f3f) 34.61% compared to head (9873a20) 34.59%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7474      +/-   ##
==========================================
- Coverage   34.61%   34.59%   -0.02%     
==========================================
  Files        1017     1019       +2     
  Lines       27165    27193      +28     
  Branches     2211     2212       +1     
==========================================
+ Hits         9402     9408       +6     
- Misses      17276    17297      +21     
- Partials      487      488       +1     
Files Coverage Δ
app/core/WalletConnect/WalletConnectV2.ts 2.26% <0.00%> (-0.02%) ⬇️

... and 9 files with indirect coverage changes

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

@sonarcloud
Copy link

sonarcloud bot commented Oct 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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@christopherferreira9 christopherferreira9 added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Oct 11, 2023
Copy link
Member

@andreahaku andreahaku 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. 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) labels Oct 19, 2023
@christopherferreira9
Copy link
Contributor

All good with testing.

Logs observed:

LOG  [MetaMask DEBUG]: Analytics 'trackEventWithParameters' - {"category": "Dapp Transaction Cancelled"} {"account_type": "MetaMask", "chain_id": "5", "dapp_host_name": "wc::https://react-app.walletconnect.com", "gas_estimate_type": undefined, "gas_mode": "Advanced", "request_source": "WalletConnect", "speed_set": undefined} undefined undefined
 LOG  [MetaMask DEBUG]: Analytics 'trackEventWithParameters' - {"category": "Dapp Transaction Cancelled"} {"account_type": "MetaMask", "active_currency": "ETH", "asset_type": "ETH", "chain_id": "5", "dapp_host_name": "wc::https://react-app.walletconnect.com", "gas_estimate_type": undefined, "gas_mode": "Advanced", "request_source": "WalletConnect", "speed_set": undefined} undefined undefined

@christopherferreira9 christopherferreira9 merged commit d796ca3 into main Oct 19, 2023
29 of 34 checks passed
@christopherferreira9 christopherferreira9 deleted the fix/wc2-origin branch October 19, 2023 09:02
@christopherferreira9 christopherferreira9 added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Oct 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 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.10.0 team-sdk SDK team WalletConnect WalletConnect related issue or bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants