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

Analytics: track tx creations and safe views #2787

Merged
merged 15 commits into from
Nov 22, 2023
Merged

Analytics: track tx creations and safe views #2787

merged 15 commits into from
Nov 22, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Nov 9, 2023

What it solves

As per this doc, we want to track key events in a new way, and for that we were missing a transaction_created and safe_opened events.

So I've instrumented all of our tx flows to emit a new CREATED event + the corresponding tx type from a pre-defined list.

Also, a few changes:

  • Added an event when a distinct Safe Account is opened.
  • Restored a Simulate tx event that we lost in the tx flow redesign
  • Removed the unused Proposed tx
  • Renamed Execute transaction to Toggle execute transaction because it's actually triggered on the Execute checkbox

How to test it

Safe viewed event

  • Open a Safe, a GTM event should be triggerred
  • Switch to another Safe, the event should trigger again
  • Browsing pages within the same Safe should not trigger it

Transaction created event

  • Create all types of transactions possible via our UI
  • Each tx creation should be tracked along with its type from thsis list
  // Owner txs
  owner_add = 'owner_add',
  owner_remove = 'owner_remove',
  owner_swap = 'owner_swap',
  owner_threshold_change = 'owner_threshold_change',

  // Module txs
  guard_remove = 'guard_remove',
  module_remove = 'module_remove',
  spending_limit_remove = 'spending_limit_remove',
  spending_limit_add = 'spending_limit_add',

  // Safe txs
  safe_update = 'safe_update',

  // Transfers
  transfer_token = 'transfer_token',
  transfer_nft = 'transfer_nft',

  // Other
  batch = 'batch',
  rejection = 'rejection',
  typed_message = 'typed_message',
  safeapps = 'safeapps',
  walletconnect = 'walletconnect',

Copy link

github-actions bot commented Nov 9, 2023

Branch preview

✅ Deploy successful!

https://analytics--walletweb.review-wallet-web.5afe.dev

Copy link

github-actions bot commented Nov 9, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Nov 9, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.96% (+0.04% 🔼)
10019/13190
🔴 Branches
50.12% (-0.01% 🔻)
2040/4070
🔴 Functions
58.76% (+0.03% 🔼)
1496/2546
🟡 Lines
77.53% (+0.03% 🔼)
9054/11678
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / transactions.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / dispatch.ts
49.3% (-0.35% 🔻)
61.11% 32%
48.12% (-0.36% 🔻)
🔴
... / utils.ts
48.89% (-2.33% 🔻)
36.36%
26.67% (-1.9% 🔻)
50% (-1.72% 🔻)
🔴
... / SignForm.tsx
58.7% (-1.3% 🔻)
38.46% (-1.54% 🔻)
33.33%
59.09% (-1.37% 🔻)
🟢
... / ReviewSignMessageOnChain.tsx
82.26% (-2.22% 🔻)
50% (-8.82% 🔻)
80%
83.33% (-2.38% 🔻)
🟡
... / ReviewTokenTx.tsx
61.54% (-9.89% 🔻)
0% 0%
66.67% (-16.67% 🔻)

Test suite run success

1098 tests passing in 154 suites.

Report generated by 🧪jest coverage report action from cad088c

Copy link

@alicecha alicecha 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 to me.

@@ -12,14 +12,20 @@ export type SafeAppsTxParams = {
params?: SendTransactionRequestParams
}

const SafeAppsTxFlow = ({ data }: { data: SafeAppsTxParams }) => {
const SafeAppsTxFlow = ({
Copy link
Member

@usame-algan usame-algan Nov 14, 2023

Choose a reason for hiding this comment

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

Not sure why but I didn't see a Transaction created event when I created one via CSV Airdrop safe app. Only Propose Transaction and On-chain interaction. Do we need to add it to the App Frame?

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Nov 14, 2023

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1.39 MB (🟡 +609 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Six Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/addOwner 3.59 KB (🟡 +37 B) 1.4 MB
/balances/nfts 18.68 KB (🟡 +48 B) 1.41 MB
/home 23.61 KB (🟢 -19 B) 1.41 MB
/settings/modules 9.9 KB (🟡 +63 B) 1.4 MB
/settings/setup 24.3 KB (🟡 +93 B) 1.42 MB
/settings/spending-limits 22.78 KB (🟡 +63 B) 1.41 MB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@francovenica
Copy link
Contributor

francovenica commented Nov 20, 2023

I compared GTM calls from this PR and dev to see what has changed/added and I have doubts on how the table in the doc is supposed to correlate with what it was done in this ticket given the "proposed event Name" column and "old event column"

Opening safe: There is a new event with an eventAction:"safe viewed". This seems to be added when comparing with dev.
Now in the doc it seems there is an event for that that should be called "safe_opened" and the Old event name it was "overview", which is still the eventCategory in this PR.
So, is this something it should change? should this event should have the "safe_opened" event category?
image

Same if you open a safe after going through the Add safe flow. The eventAction is Open safe and the eventCategory is load-safe which in the table is described as "Old event name", so should this one also be changed?
image

@francovenica
Copy link
Contributor

Issues:

Spending_limit_transfer: I don't know how to trigger this one. In the UI we don't have a way to transfer the spending limit to another address, we can just remove and add (even the "Editing" is an "spending limit add")

Batch:
The eventLabel "batch" is shown when the batch is signed/executed, so that's fine
An issue I have is that adding a tx to the batch with the "Add to batch" button is also triggering the same event as signing the tx; so "transfer_token" if you batch a Send funds transaction or "transfer_nft" if you batch a Send NFT tx. This is something that doesn't happen in dev.
image

Safe Apps:
Tested by using the Tx builder and a dummy contract with test methods.
Not sure how to trigger this one. I assumed they should trigger when you sign a tx that was done by a safe app.
I've checked and the calls in this PR are the same as the ones in dev, so it might be that I'm just looking in the wrong place.
The events I see when creating a tx through safe apps are these 2:
1 - eventAction: "Propose Transaction" eventCategory: "safe-apps"
2 - eventAction: "Submit transactions confirmed" eventCategory "safe-apps-analytics"

Notes:

I've noticed that we don't have an event for bulk execution.

For the WallectConnect event, I assume is when you execute a tx while connected to dApp through the WC pairing feature. I currently don't have funds so I'll try this when I can get more.

The ones that work fine (just listed here for the sake of documentation):

Beyond what I said in the previous comment, the "safe viewed" event only triggers on opening, not anymore as you navigate through the safe, so that's the expected behavior.
The "Simulate event" looks good
The "Toggle execute tx" looks good, with a true/false eventLabel depending on what the user chooses ("yes, execute" or "no, later")

Transactions:
These trigger when you sign the tx to propose it.

Owner adding: looks good. a new event comparing dev with this PR, with an eventAction: "Create Transaction" and an eventLabel: "owner_add"
Owner removal: Looks good, same as before with eventLabel: "owner_remove"
Owner swap: Looks good. eventLabel: "owner_swap"
Change Threshold: Looks good. eventLabel: "owner_threshold_change"

Guard_remove: Looks good. eventLable: 'guard_remove'. Safe with a guard for regression later: gor:0x24F8542Da0C5f267c177a224270442C73804EE80
Module_remove: Looks good. eventLabel: 'module_remove'
Spending_limit_remove: Looks good. eventLabel: 'spending_limit_remove',
Spending_limit_add: Looks good. eventLable: 'spending_limit_add',

Safe_update: looks good. eventLable: 'safe_update',

Transfer_token. Looks good. eventLabel: 'transfer_token',
Transfer_nft: Looks good. eventLabel: 'transfer_nft',

Rejection. Looks good. eventLabel: 'rejection',
Typed_message: looks good. eventLable: 'typed_message', (tested this by connecting to OpenSea with WC, which requires a off-chain signature of a message)

Copy link

github-actions bot commented Nov 20, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh
Copy link
Member Author

This PR doesn't include the new naming scheme, this will be done later.

I'm only adding Safe viewed and Transaction created using the old naming convention.

Fixed the batch and safeapps events. ✅

For the WallectConnect event, I assume is when you execute a tx while connected to dApp through the WC pairing feature. I currently don't have funds so I'll try this when I can get more.

You don't have to execute them, this new event tracks tx creation.

@katspaugh katspaugh changed the title Feat: track tx creations and safe views Analytics: track tx creations and safe views Nov 21, 2023
* Fix: remove deprecated mobile pairing (#2794)

* Fix: remove deprecated mobile pairing

* Fix accounts[0] and Delete Account

* Update onboard package

* Update env vars

* Fix: add help link in footer (#2822)

* Fix: add help link in footer

* External link

* Fix: double scroll bar in safe apps (#2829)

* Feat: track app version in all events

* Tests
Copy link

github-actions bot commented Nov 21, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh katspaugh added this to the 1.3.0 milestone Nov 21, 2023
@francovenica
Copy link
Contributor

The batch fixing is fine

For Safe apps I tried again and I still see the same calls I reported, they are the same as the ones I see in current Dev. So I dont see the `safeapps = 'safeapps' event.

I use the tx builder with a dummy contract to execute a test method.
image

Maybe the current calls are the ones that the tracking expects. let me know if that's the case

@francovenica
Copy link
Contributor

Same thing happens with WC.
I tried in this PR and in dev to create the same tx: using Curve to swap tokens in a polygon safe.
After creating the tx the GTM calls I had were the same in this PR and dev. So I didn't see the walletconnect = 'walletconnect', event
image

@katspaugh
Copy link
Member Author

@francovenica thanks! Fixed for both safeapps and walletconnect. ✅

@francovenica
Copy link
Contributor

Yes, the events are there now:

image

image

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh katspaugh merged commit 8a216e0 into dev Nov 22, 2023
15 checks passed
@katspaugh katspaugh deleted the analytics branch November 22, 2023 08:32
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants