-
Notifications
You must be signed in to change notification settings - Fork 12
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
wip(feat): add trezor support #7
base: main
Are you sure you want to change the base?
Conversation
What I'm planning to do next:
|
Hey thanks for the help To resolve a wallet you have to use For the Trezor feature, for supply-chain security reasons we can't add 141 dependencies to our graph, someone needs to reimplement it with a better supply-chain I would have gladly reimplemented it but I can't since I do not own a Trezor (yet) :( The other stuff seems good I will check this out today Thanks for your contribution 🫡 |
Also I do not currently check if the address is duplicated when importing from Ledger or generating from seed or importing from private key, it's best to mimic that behavior for now |
"Display correctly the wallet balances in the list (currently it's displaying just "0 ETH")" You have to take extra care not to fetch all wallets at once, to avoid the bad guys from linking wallets together |
I partially merged |
That's a problem as well. I'm gonna get all the wallet UUIDs via
I'll look at it by myself and try to re-implement it.
Good point 👍 |
Nice, thanks, you MAY start implementing it in
You can use imperative style with Also, I just finished writing CONTRIBUTING.md let me know what you think |
If I'm understanding it correctly, you have to still use the
I don't have any problems with the contribution guidelines, I've got it that this is a privacy/security aligned project. |
The |
I have good news: I just bought a Trezor so I will be able to help you |
This is the lib where the communication with the Trezor is implemented: https://github.com/trezor/trezor-suite/tree/f5286f61368a8970bf45fc78ccb6cd7a8712ab0c/packages/transport The communication is described there (in README.md) from a high-level point of view. I guess that's something that we're looking for and what we want to re-implement. They're using protocol buffers for message standardization. Here's the example test, they have (for sending/receiving msgs): https://github.com/trezor/trezor-suite/blob/f5286f61368a8970bf45fc78ccb6cd7a8712ab0c/packages/transport/tests/abstractUsb.test.ts#L323 Here are the proto definitions for the messages (the link will redirect you directly to proto definition that we gonna use for getting eth address): https://github.com/trezor/trezor-suite/blob/f5286f61368a8970bf45fc78ccb6cd7a8712ab0c/packages/protobuf/messages.json#L3943 I hope you'll find this information helpful for further research/implementation. |
Here are the constants (primarly VENDOR_ID) needed for filtering the devices (via |
Nice, we can install https://github.com/protobufjs/protobuf.js for protocol buffers |
Any news ? |
I will add soon :) |
Hey @hazae41, I would like to add support for Trezor hardware wallets. The feature is almost fully done (the basic import as you can see on the video down below), I just need to handle all corner cases and tweak the UI.
I also changed the UX flow of "importing a wallet" from a hardware wallet. I know you're currently doing it through the "Seeds" page, but I think that from the user's point of view it's quite complicated, personally I didn't get it for the first time why there's the "Seeds" page at all. Now, I understand that the reason is to be able to generate different wallets from the seed that is saved on the hardware wallet.
So I decided to add the option of importing a hardware wallet directly from the "Wallets" page (as you can see on the video). Would like to know your opinion on this change.
brume_trezor_import.mp4
Right now I'm struggling with one UX thing. When the list of addresses (derived from seed on the hardware wallet) appears in the UI, I want to disable those, which are already imported in Brume.
To check which address is already imported into Brume, I need to get all Brume wallet addresses from the storage. I can use the
useWallets
hook, but it returns me just UUID for each wallet (I need the address of the wallet).This is a part of the code where I want to check if the address is already imported (
src/mods/foreground/entities/wallets/all/create/hardware.tsx
).So, how could I query all user wallets from the IndexedDB storage?
Thanks in advance for any advice! ❤️