actions: Add ledger backend functions. #3869
Conversation
JoeGruffins
force-pushed
the
addledgeractions
branch
from
7278544
to
5ab2419
Compare
This comment was marked as outdated.
This comment was marked as outdated.
JoeGruffins
force-pushed
the
addledgeractions
branch
from
5ab2419
to
0a458f1
Compare
JoeGruffins
force-pushed
the
addledgeractions
branch
from
0a458f1
to
b1c1525
Compare
JoeGruffins
force-pushed
the
addledgeractions
branch
2 times, most recently
from
e18069f
to
32050eb
Compare
JoeGruffins
force-pushed
the
addledgeractions
branch
from
32050eb
to
afece2a
Compare
|
I changed the helper method a bit so need to fix the tests then should be good. |
JoeGruffins
force-pushed
the
addledgeractions
branch
3 times, most recently
from
6e1d68c
to
aed9703
Compare
app/helpers/ledger.js
Outdated
| // bitshift right (>>>) does not seem to throw away the lower half, so | ||
| // subtracting. | ||
| const upper = n - lower; |
There was a problem hiding this comment.
wait, this doesnt work. hmm..
There was a problem hiding this comment.
Division with the max uint32 works correct? What is the correct syntax to bitshift right though? Is it not n>>32 or n>>>32?
Not using Buffer.writeUint64LE as this version of Buffer does not seem to have it.
JoeGruffins
force-pushed
the
addledgeractions
branch
from
aed9703
to
bfdb28b
Compare
|
@matheusd ok, I'll look into it... |
|
@matheusd it looks like we might need electron/electron#36289 We are using whats in the package.json And of course it breaks if I try to update. |
|
Definitely working with electron v23.3.8. I guess I will put up a pr increasing the version and see what happens. #3884 |
JoeGruffins
force-pushed
the
addledgeractions
branch
from
bfdb28b
to
632d455
Compare
|
Now using "@ledgerhq/hw-transport-webusb" and everything looks alot better. Glad we took this route. |
app/actions/LedgerActions.js
Outdated
| // TODO: Enable on mainnet. | ||
| const isTestnet = true; | ||
| try { | ||
| const payload = await getAddress("44'/42'/0'"); |
There was a problem hiding this comment.
Instead of btc.getWalletPublicKey() (which is what this call cashes out into), maybe you could use btc.getWalletXpub()?
I haven't thoroughly checked the correctness of the next line's call to pubkeyToHDPubkey(), but I'd be very afraid a mistake in there is going to render the resulting wallet either unusable or incompatible to other software, making recovery of the funds using other software impossible.
There was a problem hiding this comment.
Totally missed getWalletXpub, thank you. Now using that. It uses a sha256 checksum so replacing that.
JoeGruffins
force-pushed
the
addledgeractions
branch
from
632d455
to
77583e9
Compare
| // fixPubKeyChecksum replaces the sha256 checksum, or last four bytes, of a | ||
| // pubkey with a blake256 checksum. | ||
| export function fixPubKeyChecksum(pubKey) { | ||
| const buff = bs58.decode(pubKey).slice(0, -4); |
There was a problem hiding this comment.
This will make it into a valid xpub, ok.
However, this means you can't go from seed to wallet using the standard decred software or my BIP0039 -> raw seed tool.
My concern here is that, if the user (say) gets its ledger stolen and wants to withdraw all funds as fast as possible using only software, there will be no fully compatible software to do so, because this particular path from mnemonic seed -> raw seed bytes -> account is not replicated on any software.
At the very least we'll need a tool that can go from the ledger mnemonic seed to either a raw seed or account xprivs that can be imported into decrediton/dcrwallet so that we can accurately recreate the wallet in software only if needed.
There was a problem hiding this comment.
However, this means you can't go from seed to wallet using the standard decred software or my BIP0039 -> raw seed tool.
Where was the tool again? In the decred repo? I'm unsure why it would not work but I trust you are correct.
There was a problem hiding this comment.
@matheusd I was able to use the tool at https://github.com/matheusd/bip39-to-dcr-seed to restore a decred wallet from seed.
I made a mainnet wallet with decrediton and ledger. Send it a small amount of funds. After restoring using your tool, the seed words, and dcrwallet I was able to access the funds. It works!
There was a problem hiding this comment.
I should mention that the tool will not work on testnet because the ledger testnet app forces cointype 42' although dcrwallet wants 1'. This is fine for mainnet though.
There was a problem hiding this comment.
Does that testnet cointype restriction apply even if you change the following line:
?
There was a problem hiding this comment.
Change "44'/42'/0'" to "44'/1'/0'"? Ledger will give me the error "Ledger device: Internal error, please report (0x6f15)".
JoeGruffins
force-pushed
the
addledgeractions
branch
from
77583e9
to
f615386
Compare
|
Just removed the origin here and it is working for the package https://github.com/decred/decrediton/compare/77583e96a8774c61c8ee2a3c4891216e93b24410..f6153862fef4af2b2622e264916cce7abf636059 |
| if (numOuts > 2) { | ||
| throw "more than two outputs is not expected"; | ||
| } |
There was a problem hiding this comment.
It's ok if we don't support more than two outputs in the first version, but we really should eventually support it (unless ledger itself limits this somehow).
There was a problem hiding this comment.
| buff.set(writeUint16LE(out.version), i); | ||
| i += 2; | ||
| // TODO: Clean this up for production? Should use smarter logic to get varInts? | ||
| buff[i] = out.script.length; // varInt for 23/25 bytes |
There was a problem hiding this comment.
Indeed, this won't be a problem for standard outputs, because the varints for 23/25 bytes is itself, but could be an issue in the future.
There was a problem hiding this comment.
Yeah. Leaving for now...
JoeGruffins
force-pushed
the
addledgeractions
branch
from
f615386
to
74284d2
Compare
part of #3865