|
Title: x Post by: abrkn on January 23, 2014, 07:54:30 AM x
Title: Re: Need BIP0032 code reviewed (node.js) Post by: empoweoqwj on January 23, 2014, 08:01:35 AM I re-implemented the BIP0032 wallet in vbuterin/bitcoinjs-lib and would like someone to review my work: Source code: https://github.com/justcoin/bitcoinjs-lib/blob/master/src/hdwallet.js Tests: https://github.com/justcoin/bitcoinjs-lib/blob/master/test/hdwallet.js Is it customary to tip for code review? If someone spends their time properly reviewing your code then I would certainly think a tip would be in order :) Title: Re: Need BIP0032 code reviewed (node.js) Post by: empoweoqwj on January 26, 2014, 10:45:29 AM Will pay a 0.1 BTC bounty How many lines of js are we talking? Title: Re: Need BIP0032 code reviewed (node.js) Post by: pocesar on January 30, 2014, 10:02:43 AM You are missing tests for transaction.js, script.js and wallet.js and not taking in consideration third party modules (like jsbn and crypto-js).
You got some defined functions that arent being used (therefore tested if working), for example in index.js, rotl, rotr and endian. You need to make tests cover exceptions being thrown (haven't found any in any file). since you are using qunit, assert.throws should do it. On the derive of HDWallet, you are checking for this.priv, but the pub is not being covered (in case priv is false). None of the base64 functions from convert.js are being covered. This is what I got so far. Btw, you should enable issues in your repo (since when you fork on github, it's not enabled by default) Title: Re: Need BIP0032 code reviewed (node.js) Post by: empoweoqwj on January 30, 2014, 11:37:45 AM You are missing tests for transaction.js, script.js and wallet.js and not taking in consideration third party modules (like jsbn and crypto-js). You got some defined functions that arent being used (therefore tested if working), for example in index.js, rotl, rotr and endian. You need to make tests cover exceptions being thrown (haven't found any in any file). since you are using qunit, assert.throws should do it. On the derive of HDWallet, you are checking for this.priv, but the pub is not being covered (in case priv is false). None of the base64 functions from convert.js are being covered. This is what I got so far. Btw, you should enable issues in your repo (since when you fork on github, it's not enabled by default) Ouch - I think you should pay pocesar immediately :) |