Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Wookong hardware wallet support #3777

Closed
wants to merge 8 commits into from
Closed

Conversation

gavofyork
Copy link
Member

Rework (mostly a rewrite) of #3667.

@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 8, 2019
@gavofyork
Copy link
Member Author

@chesterliliang i'm having trouble getting this to work on linux. are there specific drivers needed? here's the output:

gav@pop-os:~/Core/substrate$ sudo ./target/release/subkey --wookong inspect
[sudo] password for gav: 
U��1---------init--g_dev=0------
                                ----------!multi part!------------
                                                                  ----------!multi part!------------
                                                                                                    ----------!multi part!------------
              ----------!multi part!------------
                                                ----------!multi part!------------
                                                                                  ----------!multi part!------------
                                                                                                                    Public Key URI `` is account:
                           Network ID/version: substrate
                                                          Public key (hex): 0x0000000000000000000000000000000000000000000000000000000000000000
                        Address (SS58): 5C4hrfjw9DjXZTzV3MwzrrAr9P1MJhSrvWGWqi1eSuyUpnhM

note that it leave the terminal in a broken state after running, too.

@chesterliliang
Copy link

@gavofyork Linux should have CP2102 driver. The port could be listed by 'mesg | grep ttyS*'
Here is the C code to init uart in solo crate

int uart_init(int *dev)
{
    int fd; 
#ifdef __linux__git
    fd = open("/dev/ttyUSB0", O_RDWR | O_NOCTTY | O_NONBLOCK); 
#elif TARGET_OS_MAC
    fd = open("/dev/tty.SLAB_USBtoUART", O_RDWR | O_NOCTTY | O_NONBLOCK); 
#else
#   //error "Unsupport platform for uart"
#endif

I will try to figure out why.

@gavofyork
Copy link
Member Author

it looks a bit like it's opening & writing to the standard i/o instead of the UART.

@gavofyork
Copy link
Member Author

gav@pop-os:~/Core/substrate/scripts$ mesg | grep ttyS*
gav@pop-os:~/Core/substrate/scripts$ 

@chesterliliang
Copy link

Device not in the list.. well, do you use ubuntu 18.x? And just for a remind, plz plug the usb connector to the blue board. I put it into another one sometimes..

@gavofyork
Copy link
Member Author

ubuntu 19.04

@chesterliliang
Copy link

Here is a reference to see if the device works on linux:
https://www.silabs.com/community/interface/knowledge-base.entry.html/2018/07/31/how_to_check_if_the-5tUc
and try to install the driver may help:
https://www.silabs.com/products/development-tools/software/usb-to-uart-bridge-vcp-drivers

The product will switch to USB-HID finally, so it will not need driver for every platform. I thought UART is simple enough for early use but it seems it's not ture. T_T

@chesterliliang
Copy link

@gavofyork is your 19.04 the desktop or server version?

@gavofyork
Copy link
Member Author

desktop; i'm back to working on macos and it's ok again

impl Wallet for Wookong {
type Pair = sr25519::Pair;
fn derive_public(&self, _path: &[DeriveJunction]) -> Result<Public> {
// TODO: pass through _path and _password to mutate the key on the device accordingly.
Copy link
Member Author

Choose a reason for hiding this comment

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

@chesterliliang this needs implementing

Choose a reason for hiding this comment

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

Okay. I am on it. But for the password.. the hardware wallet always has one password for all accounts and the device will keep the password for verify. Device api functions like

fn verify_password(password);
fn change_password(old, new);

could be provided. And we could call verify function here. But the derive will not depend on password as salt but just the path. Is that OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah - sorry the _password is old and doesn't need to be implemented at all.

}
}
fn sign(&self, _path: &[DeriveJunction], message: &[u8]) -> Result<Signature> {
// TODO: pass through _path and _password to mutate the key on the device accordingly.
Copy link
Member Author

Choose a reason for hiding this comment

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

@chesterliliang this needs implementing

Choose a reason for hiding this comment

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

Should we verify password as well when do the sign?

Copy link
Member Author

Choose a reason for hiding this comment

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

no - can ignore _password - it's part of an old design and not relevant any more.

}

/// Unit type representing the Wookong hardware wallet.
pub struct Wookong;
Copy link
Member Author

@gavofyork gavofyork Oct 10, 2019

Choose a reason for hiding this comment

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

@chesterliliang if the device can be made to support multiple root keys (which would be nice, i think) then this could be changed thus:

pub struct Wookong {
  account: sr25519::Public,
}
impl Wookong {
  fn with_account(account: sr25519::Public) -> Self { Wookong { account } }
}

the Wallet functions for it would then feed self.account through to the hardware to ensure it operated on the right account.

Choose a reason for hiding this comment

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

If they are root keys(seed? before derive), we could find them by index, it seems hardware can not know which one is the right by account.

Copy link
Member Author

@gavofyork gavofyork Oct 11, 2019

Choose a reason for hiding this comment

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

hmm, then they can be enumerated which isn't so great for privacy.

couldn't it just store the public keys along with the roots and cycle through those that it know until it either finds the correct one or returns some KeyNotFound error?

Choose a reason for hiding this comment

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

It's possible but must under the same derive path or same rule to manage the path. Seems not a big trouble. Will try to give an implementation for further discussion.

@burdges
Copy link

burdges commented Oct 12, 2019

Just fyi @chesterliliang we'll eventually "repair" the missuse of i and the chain code by SURI. See w3f/schnorrkel#45 and #3396 We'll play some tricks to keep all current derivations the same, which requires a coordinated upgrade of both subkey and schnorrkel, but I wanted to warn you. :)

@chesterliliang
Copy link

@burdges Thanks Jeff. Don't worry about us since we just start to work on derive part. Could you plz provide one, just one test vector if the issue is fixed?

@burdges
Copy link

burdges commented Oct 12, 2019

We'll avoid currently allowable test vectors changing. Yet, subkey will continue to not use chaincodes for the questionable purpose for which bip32 added them, so maybe some future wallets will use it in incompatible ways. I'm just warning about this future interface change on schnorrkel

@gnunicorn
Copy link
Contributor

is this still being worked on?

@chesterliliang
Copy link

No... Team has financial problem.. plz close it...

@gnunicorn gnunicorn closed this Feb 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants