-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Disable hardware-wallets on platforms that don't support libusb
#8464
Conversation
That's a lot of extra noise for all that stuff. I think it's desirable for sure, but I'm not sure about where the tradeoff makes sense in terms of code ugliness/maintainability vs being able to disable it. I'm not sure what's possible theoretically in making this prettier? Ultimately going the way of geth and having a completely separate account mgmt binary (including hw wallets) might be a good idea. |
Right, I think it should be possible simply By using features in the crates that is using based on the target:
EDIT: |
ethcore/src/account_provider/mod.rs
Outdated
@@ -130,6 +137,7 @@ pub struct AccountProvider { | |||
/// Accounts unlocked with rolling tokens | |||
transient_sstore: EthMultiStore, | |||
/// Accounts in hardware wallets. | |||
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows", target_os = "android"))] | |||
hardware_store: Option<HardwareWalletManager>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to just change a type to a no-op for the unsupported platforms? Like so:
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows", target_os = "android"))]
mod hw {
pub use hardware_wallet::{Error as HardwareError, HardwareWalletManager, KeyPath, TransactionInfo};
}
#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows", target_os = "android")))]
mod hw {
pub enum HardwareError;
pub struct HardwareWalletManager;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would reduce the annotations a bit so I'm in favour!
d166498
to
fcb7740
Compare
ethcore/src/account_provider/mod.rs
Outdated
use super::fmt; | ||
|
||
#[derive(Debug)] | ||
pub enum HardwareError {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need docs
ethcore/src/account_provider/mod.rs
Outdated
|
||
#[derive(Debug)] | ||
pub enum HardwareError {} | ||
pub struct HardwareWalletManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need docs
Needs reviews. |
4ec90e7
to
b55b04d
Compare
|
||
use super::{fmt, Address}; | ||
|
||
pub struct WalletInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy-paste of hardware_wallet::WalletInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be a newtype of that?
rpc/src/lib.rs
Outdated
@@ -67,7 +67,10 @@ extern crate parity_version as version; | |||
extern crate rlp; | |||
extern crate stats; | |||
extern crate keccak_hash as hash; | |||
|
|||
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed android for now because I am not sure whether android compiles or not.
I need to grab the android NDK
in order to test it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easiest solution is docker.
Go to parity/docker/android
and create the docker image: docker build -t parity-android .
You can then compile Parity with it: docker run --rm -it -v
pwd:/usr/code -w /usr/code parity-android
and cargo build --target=armv7-unknown-linux-androideabi
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Pierre!
rpc/src/v1/helpers/dispatch.rs
Outdated
@@ -713,6 +717,7 @@ fn signature(accounts: &AccountProvider, address: Address, hash: H256, password: | |||
} | |||
|
|||
// obtain a hardware signature from the given account. | |||
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skipped this because it will require to implement alot of more dummy types!
b55b04d
to
88460f8
Compare
Hey @niklasad1 it would be good to add |
Hey @jtomanik thanks for the heads-up but |
@niklasad1 sorry misread your code, still getting used to Rust syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @niklasad1 that this is very brute-force
-ish. My personal feeling is that for a comparatively small win – compile on FreeBSD – we're cluttering our code significantly. I can't really speak to @folsen s suggestion to extract the hardware wallet stuff to its own binary, but I sure like the sound of teasing apart orthogonal concerns whenever I hear it.
Even without a full extraction it feels like it should be possible to do this in a cleaner way, grouping together everything HW related into a single trait and implement a shim for the BSDs? Not sure if @niklasad1 tried this already and ran into issues, but would love to hear more.
ethcore/src/account_provider/mod.rs
Outdated
.into_iter() | ||
.map(|a| a.address) | ||
.filter(|address| !self.blacklisted_accounts.contains(address)) | ||
.collect() | ||
) | ||
} | ||
|
||
/// Returns addresses of hardware accounts. | ||
pub fn hardware_accounts(&self) -> Result<Vec<Address>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making this private and return Error
? Seems like the only caller is hardware_accounts_info()
and it a little misleading as hardware_accounts()
doesn't actually ever fail (due to the map_or_else
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a great suggestion i.e, to return an Error
but it can't be private because rpc
actually calls this!
|
||
use super::{fmt, Address}; | ||
|
||
pub struct WalletInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be a newtype of that?
3b78b39
to
e03bc8d
Compare
e03bc8d
to
6e58c04
Compare
@dvdplm Totally agree I was lazy thought I needed to RE: own binary for hardware_wallet requires some refactoring for example if you want sign a transaction via
It is probably pretty easy to change to communicate via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
Needs a 2nd review. |
* Might consume slight more memory than pure conditional compilation flags
* Tested by it compiling succesfully with `cargo build --target=armv7--linux-androideabi` * The binary is ~66 MB ```bash $ size size target/armv7-linux-androideabi/release/parity text data bss dec hex filename 50676230 416200 31456 51123886 30c16ae target/armv7-linux-androideabi/release/parity ```
* Removes some conditional compilation flags * Introduces `fake-hardware-wallet` crate
f825a9a
to
2011226
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like this approach more than the previous, so LGTM.
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: parity: omit redundant last imported block number in light sync informant (openethereum#8962) Disable hardware-wallets on platforms that don't support `libusb` (openethereum#8464) Bump error-chain and quick_error versions (openethereum#8972) EVM benchmark utilities (openethereum#8944) parity: hide legacy options from cli --help (openethereum#8967) scripts: fix docker build tag on latest using master (openethereum#8952) Add type for passwords. (openethereum#8920)
* master: Refactor evm Instruction to be a c-like enum (#8914) Fix deadlock in blockchain. (#8977) snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984) Use local parity-dapps-glue instead of crate published at crates.io (#8983) parity: omit redundant last imported block number in light sync informant (#8962) Disable hardware-wallets on platforms that don't support `libusb` (#8464) Bump error-chain and quick_error versions (#8972)
* master: Refactor evm Instruction to be a c-like enum (#8914) Fix deadlock in blockchain. (#8977) snap: downgrade rust to revision 1.26.2, ref snapcraft/+bug/1778530 (#8984) Use local parity-dapps-glue instead of crate published at crates.io (#8983) parity: omit redundant last imported block number in light sync informant (#8962) Disable hardware-wallets on platforms that don't support `libusb` (#8464) Bump error-chain and quick_error versions (#8972)
Hey,
This PR tries to add functionality to build parity on platforms that don't support
libusb or is stable
by using conditional compilation i.e,#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))]
#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))]
This is really
brute-force
and the syntax is horrible which may bemessy
to read!However, I want to know if this is something we want before putting more time on this. Moreover, this makes the
--no-hardware-wallets
useless on these platform because it is already disabledarm-linux-androideabi
I have tested it by running (without errors):
If any
FreeBSD
user can test this I would appreciate it!/cc #1425