vacuumlabs / adalite

A lightweight web wallet for Cardano cryptocurrency with Trezor, Ledger and BitBox02 support. Please note that the only valid domain for our wallet is adalite.io
https://adalite.io
235 stars 49 forks source link

deriveAddressWithHdNode 'Password must be a sting or a buffer' #68

Closed rahilzebpay closed 6 years ago

rahilzebpay commented 6 years ago

I'm having an error in deriveAddressWithHdNode, Here is my piece of test code

const mnemonicAPI = require('./wallet/mnemonic');
const addressAPI = require('./wallet/address');
const transaction = require('./wallet/transaction');

const men = mnemonicAPI.generateMnemonic();

console.log(men);

const hdnode = mnemonicAPI.mnemonicToHdNode(men);
console.log(hdnode);

console.log(hdnode.extendedPublicKey.toString('hex'));

const address = addressAPI.deriveAddressWithHdNode(hdnode, '0x1').then(()=>{}).catch((err)=>{console.log(err)});

Upon inspecting it looks like hdNode.getExtendedPublicKey() returns a Buffer but pbkdf2 function is throwing for some reason. Might be happening coz of this pull-request https://github.com/crypto-browserify/pbkdf2/pull/77

If I change the getExtendedPublicKey function to return a string instead of the buffer, It works. But is it okay and valid in terms of cardano?

  getExtendedPublicKey() {
    return this.extendedPublicKey.toString('hex');
  }
rahilzebpay commented 6 years ago

Changing the pbkdf2 version to 3.0.14 fix this. But the addresses generated are not accurate.

For eg: I get this.

55bCV98MgS7iQ9MMHwEBFQt6iqP71Ur3frZEJ4taChtKyyuPStWHZWb2SGDyUc2unwgyZANkFspkRhNtBSKHD7MFKrwgexbLCGzAuwTRognCGhWMsXT257HLTbjRx1YJC58AZq4qVtZgsWtAVovJzKQBksM42FufKKinUdB8iG7zqexUAK4i6L4u9yrtHjG47c9GtBQxekAk81D7MtYztPQRfUBYdwykzgfKkFUvNmcmRBDpw1mmu2jqxxW6tf9XSuJ5nZtJeijJ4ZPGgmZy7Rbgd4PwetfovwrKjdRXHXnWp2xWULKG34yuEJKVgx7jcw1WNmUXbjdCgekNQmgbovMgEN

refi93 commented 6 years ago

@rahilzebpay Thanks for notifying us!

I think you ran npm install instead of yarn install - therefore the yarn.lock file in the repo was ignored and you installed the latest version allowed by package.json of each npm package, which has brought that breaking change you are describing.

I recommend deleting the node_modules folder, installing yarn globally and running yarn install, but we will probably fix the code soon to be able to work with the latest version of pbkdf2.

rahilzebpay commented 6 years ago

Yes right! Thanks @refi93 !!

Can you please help me with the second issue regrading the address I mentioned above?

I have also tried this..

const CARDANO_CONFIG = {"CARDANOLITE_BLOCKCHAIN_EXPLORER_URL":"https://cardanoexplorer.com","CARDANOLITE_TRANSACTION_SUBMITTER_URL":"https://cardanoexplorer.com/api/transactions","CARDANOLITE_ADDRESS_RECOVERY_GAP_LENGTH":"5"};
const wallet = require('./wallet/cardano-wallet');
const mnemonic = 'cruise bike bar reopen mimic title style fence race solar million clean';

const mywallet = wallet.CardanoWallet(mnemonic, CARDANO_CONFIG);

//console.log(mywallet.getUsedAddresses());
(async()=>{ 
    console.log((await mywallet.getChangeAddress(9007199254740991, 1)));
})();

This is the response I get.

https://cardanoexplorer.com/api/addresses/summary/2eCXTvVvdp5SE4bbP9UatkeQyEncCHzYQZSWJpkp1aLqx6S2EFKJf59Dr6BdW41YTL14Qr2hG94FERF8HEbpoKLyuP44tWfkBFkhovGeMxzXvPusU7nhDQd7GoUVcVvbfKPbyeYvtLA2BgWoxV445VJqPZX9hAGy1kA168ENf483CWx264pDBog4aYi9C127gi8EeUL9YZrMM5ZRVtGySDjHHU5kKUxpjKVKQhYb6nzLsz7jQRqoXtEP53jj9jtsTHVDdzHiPPDP3oRYQGT6rbPp776qFH3uNbNYerLF28j3eR9u6xdCrJjj3XqSiNt3WiLKzRqfKdKyKdHhCs2ossNWeiMz6aNvYDgDRw51h861TwTgCFUjptmZwJs returns ReferenceError:  fetch is not defined on payload: {"method":"GET","headers":{}}
(node:12058) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js processwith a non-zero exit code.

If you see, the address generated in the URL is not valid and hence the API is failing. This generates correct addreses when I try from browser, but running with node is giving problems..

Handling invalid addresses in the explorer check is also one issue.

refi93 commented 6 years ago

The current code is meant to be run in the browser (chrome, firefox, edge), not in Node.js. Therefore the fetch function uses the browser's api. If you want to use it in node, you will have to modify the call of the fetch() function in the request.js library. If you find a way to unify the fetch and address invalidity check to be able to work both in the browser and in Node, ideally without using babel or an additional library, to avoid increasing dependencies, feel free to submit a PR.

rahilzebpay commented 6 years ago

Sure @refi93 will be glad to do so!

But the major issue I'm having is the address that is generated. Why is an invalid address generated is what I'm not getting.

eg: 2eCXTvVvdp5SE4bbP9UatkeQyEncCHzYQZSWJpkp1aLqx6S2EFKJf59Dr6BdW41YTL14Qr2hG94FERF8HEbpoKLyuP44tWfkBFkhovGeMxzXvPusU7nhDQd7GoUVcVvbfKPbyeYvtLA2BgWoxV445VJqPZX9hAGy1kA168ENf483CWx264pDBog4aYi9C127gi8EeUL9YZrMM5ZRVtGySDjHHU5kKUxpjKVKQhYb6nzLsz7jQRqoXtEP53jj9jtsTHVDdzHiPPDP3oRYQGT6rbPp776qFH3uNbNYerLF28j3eR9u6xdCrJjj3XqSiNt3WiLKzRqfKdKyKdHhCs2ossNWeiMz6aNvYDgDRw51h861TwTgCFUjptmZwJs

Even directly calling deriveAddressWithHdNode gives an invalid address.

const addressAPI = require('./wallet/address');
const transaction = require('./wallet/transaction');

const men = mnemonicAPI.generateMnemonic();

console.log(men);

const hdnode = mnemonicAPI.mnemonicToHdNode(men);
console.log(hdnode);

console.log(hdnode.extendedPublicKey.toString('hex'));

const address = addressAPI.deriveAddressWithHdNode(hdnode, '0x1').then(()=>{}).catch((err)=>{console.log(err)});
refi93 commented 6 years ago

The child index should be a number, not string. I tried running this code in nodejs:

const mnemonicAPI = require('./wallet/mnemonic');
const addressAPI = require('./wallet/address');

const mnem = mnemonicAPI.generateMnemonic();
const hdnode = mnemonicAPI.mnemonicToHdNode(mnem);
const address = addressAPI.deriveAddressWithHdNode(hdnode, 0x80000001).then((x)=>{ console.log(x.address)})

And it produces an invalid address as you say, but we currently do not support nodejs, so in that sense this is expected.

I recommend you running "yarn test" and building the tests for the browser. You can inject my code sample from above there (at the beginning of the test.js file) and try opening the test.html file in the browser. It should log to the console a valid address. If you want to make it work in nodejs, you will have to analyze step-by step, what are the differences between the code execution in nodejs and in the browser and you can make the needed modifications then. I guess, nodejs handles the Buffer objects a bit differently than the browser, but you will easily see it if you trace the outputs of the called functions.

Nodejs and browser js unification would be a nice feature and if you can do it, it would be really appreciated. Unfortunately, we are currently focusing on other features, like Trezor integration.

There is also an old "nodejs" branch which we do not support anymore but there should be a working implementation of the cardano wallet api in nodejs: https://github.com/vacuumlabs/cardano/tree/nodejs

Edit: in the code sample I updated the child index to 0x80000001 so it would produce a "good" valid address, not the address associated with the root public key, which is officially discouraged from being used to preserve the anonymity of your wallet and its balance ("Crucial point in design" in https://cardanodocs.com/technical/hd-wallets/)