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

wip(feat): add trezor support #7

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

Conversation

rixcian
Copy link
Contributor

@rixcian rixcian commented Nov 17, 2023

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).
image

So, how could I query all user wallets from the IndexedDB storage?

Thanks in advance for any advice! ❤️

@rixcian
Copy link
Contributor Author

rixcian commented Nov 17, 2023

What I'm planning to do next:

  • Check that the user doesn't import already imported wallet
  • Add option to import wallet with on certain index (like it's done under the "Seeds" page)
  • Display correctly the wallet balances in the list (currently it's displaying just "0 ETH")
  • Refactor code & tweak the UI

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

Hey thanks for the help

To resolve a wallet you have to use useWallet(uuid)

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 🫡

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

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

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

"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

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

I partially merged

@rixcian
Copy link
Contributor Author

rixcian commented Nov 17, 2023

To resolve a wallet you have to use useWallet(uuid)

That's a problem as well. I'm gonna get all the wallet UUIDs via useWallets then I need to loop through those UUIDs and call useWallet, but I cannot call hooks inside functions. This is a good way when you want to render a component for each wallet, but not when you just want to get the information.



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)

I'll look at it by myself and try to re-implement it.



"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

Good point 👍

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

I'll look at it by myself and try to re-implement it.

Nice, thanks, you MAY start implementing it in libs just like I did with Ledger

That's a problem as well. I'm gonna get all the wallet UUIDs via useWallets then I need to loop through those UUIDs and call useWallet, but I cannot call hooks inside functions. This is a good way when you want to render a component for each wallet, but not when you just want to get the information.

You can use imperative style with getWallet(uuid) (Glacier 2.0 is still undocumented for now so feel free to ask questions)

Also, I just finished writing CONTRIBUTING.md let me know what you think

@rixcian
Copy link
Contributor Author

rixcian commented Nov 17, 2023

You can use imperative style with getWallet(uuid) (Glacier 2.0 is still undocumented for now so feel free to ask questions)

If I'm understanding it correctly, you have to still use the getWallet function inside a useQuery func, which is also a react hook (screenshot down below). So it doesn't fix the problem or am I missing something?

image



Also, I just finished writing CONTRIBUTING.md let me know what you think

I don't have any problems with the contribution guidelines, I've got it that this is a privacy/security aligned project.

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

If I'm understanding it correctly, you have to still use the getWallet function inside a useQuery func, which is also a react hook (screenshot down below). So it doesn't fix the problem or am I missing something?

The getWallet returns a query object, you can use that object directly

@hazae41
Copy link
Member

hazae41 commented Nov 17, 2023

I have good news: I just bought a Trezor so I will be able to help you

@rixcian
Copy link
Contributor Author

rixcian commented Nov 18, 2023

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.

@rixcian
Copy link
Contributor Author

rixcian commented Nov 18, 2023

Here are the constants (primarly VENDOR_ID) needed for filtering the devices (via navigator.usb.requestDevice()): https://github.com/trezor/trezor-suite/blob/f5286f61368a8970bf45fc78ccb6cd7a8712ab0c/packages/transport/src/constants.ts#L6

@hazae41
Copy link
Member

hazae41 commented Nov 18, 2023

Nice, we can install https://github.com/protobufjs/protobuf.js for protocol buffers

@Wjxfi
Copy link

Wjxfi commented May 23, 2024

Any news ?

@hazae41
Copy link
Member

hazae41 commented May 24, 2024

Any news ?

I will add soon :)

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.

None yet

3 participants