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

electrum: add support for LedgerHQ hardware wallet #30347

Closed
wants to merge 2 commits into from

Conversation

k0001
Copy link
Contributor

@k0001 k0001 commented Oct 12, 2017

Motivation for this change

Adding support for LedgerHQ hardware wallet in electrum by adding a dependency on btchip.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@k0001 k0001 requested a review from FRidh as a code owner October 12, 2017 11:29
@k0001
Copy link
Contributor Author

k0001 commented Oct 12, 2017

Could this be backported to 17.09 as well? Thanks.

meta = with stdenv.lib; {
description = "Python communication library for Ledger Hardware Wallet products";
homepage = "https://github.com/LedgerHQ/btchip-python";
license = licenses.apache2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use licenses.asl20. You can test meta eval by nix-instantiate --eval --strict -A foo.meta.

@oxzi
Copy link
Member

oxzi commented Oct 12, 2017

Is it necessary to add this plugin for each electrum-instance even if the majority won't use it? I feel quite uncomfortable to add unnecessary plugins in a Bitcoin-client.
Perhaps you can disable it by default and let the user enable it by overriding a list of additional plugins?

@k0001
Copy link
Contributor Author

k0001 commented Oct 12, 2017

@joachifm fixed.

@geistesk I don't know. An argument can be made in favor of having a fully functional Electrum out of the box. btchip doesn't have that many dependencies anyway, so the number of dependencies is presumably not a problem. Without btchip, the current situation is that if you try to link a hardware wallet from Electrum it just fails without any useful error message.

@k0001
Copy link
Contributor Author

k0001 commented Oct 12, 2017

@geistesk also, the Electrum we ship in nixpkgs already supports the Trezor hardware wallet.

sha256 = "16g9l3rpxpvvkdx08mgy0ligvsfcpzdrh4hplj104cpprrbsqd6v";
};

buildInputs = [ hidapi pyscard ecdsa ];
Copy link
Member

Choose a reason for hiding this comment

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

these are not needed during runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FRidh there are definitely references to these three libraries in the btchip code.

Copy link
Member

Choose a reason for hiding this comment

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

in order for Python modules to find each other, they need to be added as propagatedBuildInputs.

@joachifm
Copy link
Contributor

I'd be fine with making plugins a parameter to the package expression but we can do that in a separate feature.

@oxzi
Copy link
Member

oxzi commented Oct 13, 2017

@k0001 I fully understand the benefits of a "batteries-included" client. However, on the other side there will be a lot of additional code with possible security-flows in a Bitcoin-client (or any software at all). That's why I'd love to see an overrideable list of possible plugins.

@joachifm If we are going to introduce this feature later, we will probably end up with some "legacy"-dependencies. But I'd like to look into this subject and eventually create a PR.

@nyanloutre
Copy link
Member

nyanloutre commented Apr 29, 2018

Is this still on hold ? What work needs to be done for this to get merged ?

@joachifm
Copy link
Contributor

Seems to me that @FRidh's feedback has not been adressed

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.

5 participants