akumaigorodski / wallet

Bitcoin wallet for Android
https://sbw.app
Apache License 2.0
240 stars 73 forks source link

can't bind Keystone hardware wallet #111

Closed caixiao-QA closed 2 years ago

caixiao-QA commented 3 years ago

SBW app version: V2.4.18-86 Keystone hardware Wallet version:B-2.5, Watch-only Wallet:Generic Wallet

Add hardware wallet and scan Keystone hardware wallet,but cant bind.The screen recording is as follows. https://user-images.githubusercontent.com/37337093/142371365-a02ad7c1-6851-4793-b1f3-e64d6d25cfce.mp4

Keystone SYNC QRcode(Native Segwit): image

akumaigorodski commented 3 years ago

Can you please provide the following for this QR code:

This will simplify debugging as I don't have this HW on hand.

akumaigorodski commented 3 years ago

Alternatively, a first Bitcoin address generated by BIP84 account.

akumaigorodski commented 3 years ago

Also feel free to test with $ wget https://sbw.app/SBW-2.4.19.apk

caixiao-QA commented 3 years ago

Can you please provide the following for this QR code:

  • master key fingerprint (as Long or hex).
  • BIP84 account key as hex.

This will simplify debugging as I don't have this HW on hand.

Master fingerprint:5271c01 Derivation:m/84'/0'/0' zpub:zpub6rcabYFcdr41zyUNRWRyHYs2Sm86E5XV8RjjRzTFYsiCngteeZnkwaF2xuhjmM6kpHjuNpFW42BMhzPmFwXt48e1FhddMB7xidZzN4SF24K

caixiao-QA commented 3 years ago

Alternatively, a first Bitcoin address generated by BIP84 account. path:m/84'/0'/0'/0/0 address:bc1q9unqc738dxjg5mk8zqtz33zg59cahrj29s24lp

image

soralit commented 3 years ago

Hey @btcontract , I'm a developer from Keystone. I'm really excited to see such a smart, graceful(I really like Scala and FP) wallet supporting us! Just @ me if you have any problem in the integration work. 😄

BitcoinLixin commented 3 years ago

Hey @btcontract would you kindly use Simple Bitcoin Wallet's official twitter account to DM me so that I can send you a free sample device?

My twitter - https://twitter.com/BitcoinLixin

akumaigorodski commented 3 years ago

@BitcoinLixin thanks a lot, will do.

@caixiao-QA I've fixed it as far as I can tell. Once added all important data matches provided when scanning a reference QR code here. This can be tested in release 2.4.19: https://github.com/btcontract/wallet/releases/tag/2.4.19

soralit commented 3 years ago

Hey @btcontract , we just tested the release, also I have read the QR part of the code base, and have some recommendations: 😄

  1. it seems the arguments are passed in wrong order, the constructor of UREncoder is defined as public UREncoder(UR ur, int maxFragmentLen, int minFragmentLen, long firstSeqNum) https://github.com/btcontract/wallet/blob/master/app/src/main/java/com/btcontract/wallet/BaseActivity.scala#L539
  2. it would be good if the PSBT can be constructed as crypto-psbt, Keystone supports read PSBT from bytes just for compatibility, we will deprecate this way in the future.
  3. it would be good if the QR string can be uppercased which can reduce the QR data size, per https://github.com/BlockchainCommons/Research/blob/master/papers/bcr-2020-005-ur.md#introduction
akumaigorodski commented 3 years ago

@soralit thanks, indeed max/min values were displaced, I've fixed that and applied other suggestions in https://github.com/btcontract/wallet/commit/36e5dec30a0295f7090e310daa1a51996c322075.

akumaigorodski commented 3 years ago

I've tested and made sure everything works in https://github.com/btcontract/wallet/releases/tag/2.4.22. Guess feel free to close this if no issues are found on your side.