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

add onekey device support #709

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

Conversation

somebodyLi
Copy link

No description provided.

@somebodyLi
Copy link
Author

somebodyLi commented Sep 17, 2023

A GHA run in my repo: https://github.com/OneKeyHQ/HWI/actions/runs/6212074159

@somebodyLi
Copy link
Author

somebodyLi commented Sep 19, 2023

@achow101 Please take care of this. Thxs

hwilib/devices/onekey.py Outdated Show resolved Hide resolved
hwilib/devices/onekey.py Outdated Show resolved Hide resolved
hwilib/devices/onekey.py Outdated Show resolved Hide resolved
test/README.md Outdated Show resolved Hide resolved
@somebodyLi somebodyLi force-pushed the gha branch 4 times, most recently from 4cf3346 to d7df45f Compare December 6, 2023 06:01
@somebodyLi
Copy link
Author

the newest passed GHA https://github.com/OneKeyHQ/HWI/actions/runs/7110790185

@424778940z
Copy link

Hello @achow101, we are still waiting to merge this PR, we have already rebased to latest commit and addressed questions and issues above. Could you kindly reveiw again base on our latest commit?

@brunoerg
Copy link
Contributor

brunoerg commented Sep 5, 2024

Can you please rebase it?

@somebodyLi somebodyLi force-pushed the gha branch 2 times, most recently from cdf99d9 to e4ac963 Compare October 15, 2024 07:17
@somebodyLi
Copy link
Author

Can you please rebase it?

Of course, But there seems to be a problem with the simulator's compilation dependency installation
image

@achow101
Copy link
Member

achow101 commented Dec 4, 2024

Please squash your commits.

@prusnak
Copy link
Collaborator

prusnak commented Dec 4, 2024

Looking at the code, it seems this PR is not adding anything substantial.

The Onekey devices are just clones of Trezor One and Trezor Model T. They even copied the internal names (T1B1 and T2T1). I am not sure it is really worth complicating the codebase and risking rendering of Trezor (and all its clones) support unusable (this PR modifies trezorlib too).

Since Onekey tries to be compatible with Trezor (their FAQ mentions "support all third-party clients supported by Trezor"), maybe we should not change HWI at all and let Onekey be detected as Trezor via HWI?

@somebodyLi somebodyLi force-pushed the gha branch 2 times, most recently from b6978af to 6847858 Compare December 5, 2024 04:05
@somebodyLi
Copy link
Author

Please squash your commits.

Done!

@somebodyLi
Copy link
Author

somebodyLi commented Dec 9, 2024

@brunoerg @achow101 I fixed the onekey_t emulator Segmentation fault issue due to the CI env. If possible, please rerun the CI. Thanks.

@424778940z
Copy link

424778940z commented Dec 16, 2024

The Onekey devices are just clones of Trezor One and Trezor Model T. They even copied the internal names (T1B1 and T2T1). I am not sure it is really worth complicating the codebase and risking rendering of Trezor (and all its clones) support unusable (this PR modifies trezorlib too).

OneKey Touch/Pro are using different MCU and SE than Trezor T/One, which means hardware and low-level codes are totally different. We also designed and built our own GUI since the screen resolution is different, and we have no interest to have GUI looks the same.

OneKey firmware was forked from Trezor, but I don't think building on top of a fork should count as a counterfeit, that's not how open-source projects work. Many great projects were based on a fork.

Since Onekey tries to be compatible with Trezor (their FAQ mentions "support all third-party clients supported by Trezor"), maybe we should not change HWI at all and let Onekey be detected as Trezor via HWI?

Due to the nature of initially forked from Trezor, our protocol is compatible with Trezor. However, it does not work the other way around, since OneKey has many things Trezor does not support. This is why we need a separate implementation to fully support our device, and adding OneKey support to HWI is the first step.

Looking at the code, it seems this PR is not adding anything substantial.

Introducing fewer changes possible was suggested by @achow101. Ideally, we want a separate library path.

@rayston92
Copy link

Looking at the code, it seems this PR is not adding anything substantial.

The Onekey devices are just clones of Trezor One and Trezor Model T. They even copied the internal names (T1B1 and T2T1). I am not sure it is really worth complicating the codebase and risking rendering of Trezor (and all its clones) support unusable (this PR modifies trezorlib too).

Since Onekey tries to be compatible with Trezor (their FAQ mentions "support all third-party clients supported by Trezor"), maybe we should not change HWI at all and let Onekey be detected as Trezor via HWI?

Hi @prusnak

I totally understand your worries.

I'm a big fan of Trezor—my very first hardware wallet was a Trezor back in 2014.

Yes, a lot of OneKey's firmware was indeed developed on the shoulders of Trezor. We strictly followed Trezor's open-source license and had no intentions beyond that.

It’s worth mentioning that all of OneKey’s app code is completely open-source. We do have plans to fully rebuild our firmware using the RTOS architecture, but that's going to take some time.

To draw an analogy, if Trezor is like Google, then OneKey would be like Xiaomi. Sundar Pichai doesn't mind other manufacturers using Android's code in an open-source manner, because it actually helps Google solidify its position and market share in mobile operating systems. Similarly, I believe OneKey genuinely helps promote Trezor’s communication standards, which is beneficial for Trezor.

And let’s be honest—there's a big, annoying elephant in the room. That’s the one we really need to aim to surpass, right?

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.

6 participants