Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Add method for OneKey device analysis #136

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

originalix
Copy link

Currently, there are many users who connect to OneKey hardware wallets via the trezor keyring because they use the same protocol. This PR adds a method to get the different vendor names for easy analysis.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@AlexJupiter
Copy link

@danjm this could be what we need to differentiate OneKey and Trezor devices here.

@rayston92
Copy link

Hey @AlexJupiter @danjm

Think we need approve to continue the workflow.

🙏🙏

@mcmire
Copy link

mcmire commented Sep 22, 2022

I pressed the "approve & run" button.

@originalix
Copy link
Author

originalix commented Sep 23, 2022

Hey @mcmire

require-additional-reviewer action failed, can you help to see what needs to be done ? 🙏

@originalix
Copy link
Author

originalix commented Sep 27, 2022

I pressed the "approve & run" button.

@mcmire This ci issue is a github api error, can you help me look at it? Thanks 🙏

@mcmire
Copy link

mcmire commented Sep 27, 2022

Yeah, this check regularly fails on forks. I'll need to get someone with admin access to merge this. I can do that tomorrow.

@originalix
Copy link
Author

Yeah, this check regularly fails on forks. I'll need to get someone with admin access to merge this. I can do that tomorrow.

Hey @mcmire, have you shown this pr to anyone with admin access today so that we can continue to push for this pr to be merged into 😃

Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

@originalix Seems I was a bit hasty in suggesting that this be merged. I am not sure that all of the stakeholders have had a chance to look at this. Additionally, there are some style suggestions I had below. Would you mind taking a look at these?

Also pinging @danjm again due to having worked on this, and @darkwing for having worked on Trezor-related stuff in general, would you mind reviewing this?

CHANGELOG.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/test-eth-trezor-keyring.js Outdated Show resolved Hide resolved
test/test-eth-trezor-keyring.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@AlexJupiter
Copy link

@mcmire @danjm I think we should do this data collection PR first MetaMask/metamask-extension#15630 to understand how many OneKey devices are using MetaMask. Then we can make a decision on what to do with this one.

originalix and others added 3 commits October 4, 2022 22:09
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
# Conflicts:
#	index.js
#	test/test-eth-trezor-keyring.js
@mcmire
Copy link

mcmire commented Oct 4, 2022

Aha. Sounds good!

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

Successfully merging this pull request may close these issues.

5 participants