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

Jump to bottom

Add python3Packages.ckcc-protocol package, and list it as a dependency for electrum. #66516

Merged
merged 2 commits into from Dec 14, 2019
Merged

Add python3Packages.ckcc-protocol package, and list it as a dependency for electrum. #66516

merged 2 commits into from Dec 14, 2019

Conversation

hkjn
Copy link
Contributor

@hkjn hkjn commented Aug 12, 2019 •
edited

Motivation for this change

The https://coldcardwallet.com/ is a hardware wallet for Bitcoin. This change adds the python3Packages.ckcc-protocol package, and lists it as a dependency for electrum.

Similar to keepkey, trezor and btchip, ckcc-protocol is a plugin,
allowing electrum to communicate with the https://coldcardwallet.com/.

Also add hkjn to maintainers, and list hkjn as initial maintainer for ckcc-protocol. (Would be happy to hand over to official Coldcard maintainers at any point, if they are willing.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

N/A

@hkjn hkjn requested a review from FRidh as a code owner August 12, 2019 08:38
@hkjn hkjn mentioned this pull request Aug 18, 2019
Closed
@hkjn
Copy link
Contributor Author

hkjn commented Sep 12, 2019

Ping @FRidh. Let me know if there's anything needed from me here to help with the review process and get this closer to being merged.

@hkjn hkjn changed the title Add python3Packages.ckcc-protocol package, and list is as a dependency for electrum. Add python3Packages.ckcc-protocol package, and list it as a dependency for electrum. Sep 20, 2019
@hkjn
Copy link
Contributor Author

hkjn commented Oct 2, 2019

cc @mmahut @worldofpeace.

Sorry for semi-randomly pulling both of you into this PR, but I wanted to check for your advice on how to proceed with this ~1.5 months old PR to add a new package and list it as a dependency for electrum, to be able to use Coldcard devices with Electrum. I haven't received a review so far from @FRidh, who was auto-added as code owner.

Thanks in advance!

@mmahut
Copy link
Member

mmahut commented Oct 27, 2019

I'm sorry for missing this mention.

@GrahamcOfBorg build pythonPackages.ckcc-protocol python3Packages.ckcc-protocol

@mmahut
Copy link
Member

mmahut commented Oct 27, 2019

Looks like ckcc-protocol requires python 3.6+, please use something like disabled = ! isPy3k; in the package.

@hkjn hkjn requested a review from jonringer as a code owner October 27, 2019 15:15
@hkjn
Copy link
Contributor Author

hkjn commented Oct 27, 2019

Thanks for the review @mmahut and @jb55, please take another look!

Looks like ckcc-protocol requires python 3.6+, please use something like disabled = ! isPy3k; in the package.

I added the following to ckcc-protocol package, using other Python packages as a reference:

disabled = ! pythonAtLeast "3.6";

I've also bumped the package version to 0.8.0, which was published between the time I sent this PR out and at time of writing.

@ofborg ofborg bot requested review from ehmry, np and joachifm October 27, 2019 16:39
@hkjn
Copy link
Contributor Author

hkjn commented Oct 27, 2019

Thanks for the review @jonringer. I've addressed your suggestions at this time, please take another look.

@mmahut
Copy link
Member

mmahut commented Oct 27, 2019

@GrahamcOfBorg build python3Packages.ckcc-protocol electrum

@hkjn
python3Packages.ckcc-protocol: init at 0.8.0
187c3c3 
The ckcc-protocol package installs
https://github.com/Coldcard/ckcc-protocol, which
allows communication with the https://coldcardwallet.com/
hardware wallet.
@hkjn
electrum: Add ckcc-protocol
9f483af 
Similar to keepkey, trezor and btchip, ckcc-protocol is a plugin,
allowing electrum to communicate with the https://coldcardwallet.com/
hardware wallet.
@hkjn
Copy link
Contributor Author

hkjn commented Dec 14, 2019

Ping @jonringer @mmahut @jb55. Any chance for a review so this ~4 month old PR can move towards merging?

Thanks in advance!

@jonringer
Copy link
Contributor

the rebase, "Showing 4,569 changed files with 111,966 additions and 116,126 deletions.". That's a lot of changes in a few months O.o

@hkjn
Copy link
Contributor Author

hkjn commented Dec 14, 2019

the rebase, "Showing 4,569 changed files with 111,966 additions and 116,126 deletions.". That's a lot of changes in a few months O.o

Agreed, although with 200k+ commits in the repo, I guess this is to be expected? https://github.com/NixOS/nixpkgs/commits/master

Or did I misunderstand, and there's some issue with the two commits I'd like to merge here, or the way I'm rebasing on top of recent commits in upstream's master?

@jonringer
Copy link
Contributor

you're good, it's just the force push "brings along all the changes" in the force-push view. Your PR looks good though, reviewing now

jonringer
jonringer approved these changes Dec 14, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM
commits LGTM
ckcc shows usage
electrum starts up just fine

only a very small difference in closure size:

$ nix path-info -Sh ./result
/nix/store/if5bzxc9ls6wmrql4fqkfsq3hbv0h1d8-electrum-3.3.8	 654.4M
[nix-shell:/home/jon/.cache/nix-review/pr-66516]$ nix path-info -Sh ./results/electrum
/nix/store/nmc3fraynkghqjm0cysgf38l9f50yz4m-electrum-3.3.8	 654.5M

[5 built, 29 copied (17.1 MiB), 10.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66516
3 package were built:
electrum python37Packages.ckcc-protocol python38Packages.ckcc-protocol

@jonringer
Copy link
Contributor

@GrahamcOfBorg build electrum python37Packages.ckcc-protocol python38Packages.ckcc-protocol

@jonringer jonringer merged commit 9ace40c into NixOS:master Dec 14, 2019
@hkjn hkjn deleted the 20190812-add-ckcc branch December 25, 2019 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jb55 jb55 jb55 approved these changes

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@ehmry ehmry Awaiting requested review from ehmry

@np np Awaiting requested review from np

@joachifm joachifm Awaiting requested review from joachifm

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hkjn @mmahut @jonringer @jb55 @Janik-Haag