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: Currently extension supporty connect to OneKey via Trezor, but we dont have metric to log this when imported the accounts. #27296

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

dawnseeker8
Copy link
Contributor

@dawnseeker8 dawnseeker8 commented Sep 20, 2024

Description

Currently extension supporty connect to OneKey via Trazor, but we dont have metric to log this when imported the accounts.

This PR is created for this purpose, and i have confirmed with Alex that we should log OneKey via Trezor in account_hardware_type field in our AccountAdded metric.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/586

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

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.

@dawnseeker8 dawnseeker8 marked this pull request as ready for review September 20, 2024 03:25
@dawnseeker8 dawnseeker8 requested a review from a team as a code owner September 20, 2024 03:25
@dawnseeker8 dawnseeker8 added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 20, 2024
@legobeat legobeat changed the title feat: Currently extension supporty connect to OneKey via Trazor, but we dont have metric to log this when imported the accounts. feat: Currently extension supporty connect to OneKey via Trezor, but we dont have metric to log this when imported the accounts. Oct 3, 2024
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

Lint failing

@legobeat legobeat marked this pull request as draft October 8, 2024 03:31
@dawnseeker8 dawnseeker8 marked this pull request as ready for review October 8, 2024 08:04
@metamaskbot
Copy link
Collaborator

Builds ready [6297cf3]
Page Load Metrics (1886 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22820791716499240
domContentLoaded16492112185312158
load16922137188612861
domInteractive17146442813
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 346 Bytes (0.01%)
  • ui: 163 Bytes (0.00%)
  • common: 163 Bytes (0.00%)

Copy link

sonarcloud bot commented Oct 8, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [02b8df2]
Page Load Metrics (1737 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23724451658380183
domContentLoaded15342346170217685
load15622455173720297
domInteractive26112512311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 346 Bytes (0.01%)
  • ui: 149 Bytes (0.00%)
  • common: 329 Bytes (0.00%)

@dawnseeker8
Copy link
Contributor Author

Lint failing

This have been resolved.

@vivek-consensys
Copy link

Metric verified on Mixpanel using locally deployed build.
image.png

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA Passed team-hardware-wallets
Projects
Status: Needs more work from the author
Development

Successfully merging this pull request may close these issues.

4 participants