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

feat(dapps): Adding dapp metrics #16962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(dapps): Adding dapp metrics #16962

wants to merge 1 commit into from

Conversation

alexjba
Copy link
Contributor

@alexjba alexjba commented Dec 13, 2024

What does the PR do

closes #16912

Sending the dapps events to mixpanel

Events structure: https://www.notion.so/Event-Tracking-Specification-Desktop-1518f96fb65c8026a3a5c8a0459f7f21?pvs=4#63de5b7c3b6c4a7cab492cff6aa95957

Events description: https://www.notion.so/Metrics-for-Wallet-Connect-d3d1e60e7ea24479bd35cbb11219540c?pvs=4#375a52965a98458eb45b2e2eea6a4bd5

New board created here: https://eu.mixpanel.com/s/3kYRpk

  • Moved the Connect dialog from dapps comboBox to the dappsWorkflow where the other modals are placed
  • Extend component with necessary signals/functions of collect metrics data
  • Update tests and storybook
  • Implement fully functional storybook metrics page to test the events with Mixpanel

Affected areas

dapps

Architecture compliance

@alexjba alexjba requested review from micieslak, a team and caybro as code owners December 13, 2024 13:38
@alexjba alexjba requested review from Cuteivist, Khushboo-dev-cpp, alaibe and saledjenic and removed request for a team December 13, 2024 13:38
@status-im-auto
Copy link
Member

status-im-auto commented Dec 13, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 591ba99 #1 2024-12-13 13:45:37 ~6 min macos/aarch64 🍎dmg
✔️ 591ba99 #1 2024-12-13 13:46:56 ~7 min tests/nim 📄log
591ba99 #1 2024-12-13 13:51:38 ~12 min tests/ui 📄log
✔️ 591ba99 #1 2024-12-13 13:55:27 ~16 min linux-nix/x86_64 📦tgz
✔️ 591ba99 #1 2024-12-13 13:57:21 ~18 min macos/x86_64 🍎dmg
✔️ 591ba99 #1 2024-12-13 14:01:06 ~22 min linux/x86_64 📦tgz
✔️ 591ba99 #1 2024-12-13 14:01:41 ~22 min windows/x86_64 💿exe
591ba99 #2 2024-12-18 13:06:56 ~12 min tests/ui 📄log
✔️ f32b9bb #2 2025-01-10 10:48:52 ~9 min macos/aarch64 🍎dmg
✔️ f32b9bb #2 2025-01-10 10:48:58 ~9 min tests/nim 📄log
✔️ f32b9bb #3 2025-01-10 10:51:54 ~12 min tests/ui 📄log
✔️ f32b9bb #2 2025-01-10 10:53:18 ~13 min macos/x86_64 🍎dmg
✔️ f32b9bb #2 2025-01-10 10:58:33 ~18 min linux/x86_64 📦tgz
✔️ f32b9bb #2 2025-01-10 10:59:54 ~20 min linux-nix/x86_64 📦tgz
✔️ f32b9bb #2 2025-01-10 11:03:36 ~23 min windows/x86_64 💿exe

Copy link
Contributor

@Cuteivist Cuteivist 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
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Wow, massive work 💯

Just got a question regarding enabling stuff by default

@@ -36,12 +36,10 @@ Item {

property bool dAppsEnabled: true
property bool dAppsVisible: true
property bool walletConnectEnabled: true
property bool browserConnectEnabled: true
Copy link
Member

Choose a reason for hiding this comment

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

No longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer needed here. The reason is that I've moved the connect popups living in DappsComboBox.qml (the dapps button found in the header) in the DAppsWorkflow where the other dapps popups are implemented. These flags were needed for the connect popups.

@@ -77,6 +83,9 @@ SQUtils.QObject {

property var formatBigNumber: (number, symbol, noSymbolOption) => console.error("formatBigNumber not set")

property bool walletConnectEnabled: true
property bool connectorEnabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Unconditionally on now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default is true. But the proper value is set in the AppMain, depending on the feature flags.

Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Nice work

@virginiabalducci
Copy link

I tested the metrics and they all work good except for:

flow=sign_received : a new sign/transaction request is accepted, method: eth_signTransaction: I cannot accept eth_signTransactions, when I try to sign them they get rejected
I tested this on these dApps:
https://metamask.github.io/test-dapp/
https://se-sdk-dapp.vercel.app/

I can't yet trigger these, I'm not sure if I don't know how to trigger them yet, or if it's really a bug.
wc-unavailable: I tried disabling both WC and BC flags and nothing happens. I'm unsure if there is any API or network call that can be blocked to trigger this.
wc-available : I believe this should trigger by default if WC - BC are enabled, but this is not happening.

closes #16912

Sending the dapps events to mixpanel

| Event Name | Properties | Possible Values | Notes |
| --- | --- | --- | --- |
| dapps-health | state | - wc_available
- wc_unavailable
- chains_down
- network_down
- pair_error
- connection_error
- sign_error |  |
|  | error |  | string description |
| dapps-navigation | flow | - dapps_list_opened
- connect_initiated
- disconnect_initiated
- pair_initiated |  |
|  | connector | - wallet_connect
- browser_connect |  |
| dapps-connection | flow | - proposal_received
- proposal_accepted
- proposal_rejected
- connected
- disconnected |  |
|  | networks | networks[string] - array of networks | Array of proposed or connected networks |
|  | methods | methods[string] - array of methods | Array of proposed methods |
|  | dapp | dapp uri |  |
|  | connector | - wallet_connect
- browser_connect |  |
|  | isSiwe | boolean |  |
| dapps-sign | flow | - sign_received
- sign_accepted
- sign_rejected |  |
|  | connector | - wallet_connect
- browser_connect |  |
|  | method | - personal_sign
- eth_sign
- eth_signTypedData_v4
- eth_signTypedData
- eth_signTransaction
- eth_sendTransaction |  |
|  | dapp | string - dapp uri |  |
|  | chainId | int - chain id |  |
@alexjba
Copy link
Contributor Author

alexjba commented Jan 10, 2025

I tested the metrics and they all work good except for:

flow=sign_received : a new sign/transaction request is accepted, method: eth_signTransaction: I cannot accept eth_signTransactions, when I try to sign them they get rejected I tested this on these dApps: https://metamask.github.io/test-dapp/ https://se-sdk-dapp.vercel.app/

For now we only support personalSign, sign, signTypedData_v4, signTypedData, sendTransaction.
It will be nice to implement the signTransaction at some point, but not sure when. There is this task for it
#16770 (comment)

I can't yet trigger these, I'm not sure if I don't know how to trigger them yet, or if it's really a bug. wc-unavailable: I tried disabling both WC and BC flags and nothing happens. I'm unsure if there is any API or network call that can be blocked to trigger this. wc-available : I believe this should trigger by default if WC - BC are enabled, but this is not happening.

wc-available seems to be triggered on every startup when the app successfully initiated the WC sdk.
Screenshot 2025-01-10 at 11 47 00

wc-unavailable is still a mystery to me as well. I've tried to play around with identifying and blocking some WC IPs, but it still works. There's a report from @anastasiyaig where WC cannot be initialized on some VPNs and this event should catch it. That's the purpose of it. @anastasiyaig Is it possible for you to test this build with your VPN where the WC sdk cannot be initialized?

@alexjba alexjba force-pushed the feat/dapps-metrics branch from 591ba99 to f32b9bb Compare January 10, 2025 10:39
@virginiabalducci
Copy link

virginiabalducci commented Jan 13, 2025

Edit:
I am able to see wc-available on accounts that have the metric collection enabled.

when this is toggled off, the wc-available metric does not trigger and this is the correct behavior

@alexjba
Copy link
Contributor Author

alexjba commented Jan 13, 2025

Edit: I am able to see wc-available on accounts that have the metric collection enabled.

when this is toggled off, the wc-available metric does not trigger and this is the correct behavior

nice! This means the only point to validate is the wc-unavailable?

@virginiabalducci
Copy link

Edit: I am able to see wc-available on accounts that have the metric collection enabled.
when this is toggled off, the wc-available metric does not trigger and this is the correct behavior

nice! This means the only point to validate is the wc-unavailable?

yes correct!

@anastasiyaig
Copy link
Contributor

gonna check it now

@anastasiyaig
Copy link
Contributor

i did my best to reproduce it but i could not sadly :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics] dApps metrics
6 participants