HD/Wallet clean #200
HD/Wallet clean #200
Conversation
Unknowingly this also revealed a subtle bug in the previous implementation which allowed the creation of transactions even when no UTXOs existed.
The definition of a dust amount is pretty clear, and I feel it is less readable when represented as isDust(amount) or !isDust(amount), by comparison to amount <= dustThreshold or amount > dustThreshold. Also means I don't have to stray my eyes to understand the implemention by looking up isDust does.
There was a problem hiding this comment.
I removed network here as it is implied by the closure it resides in. If a user was to specify otherwise, it would produce a HDWallet node with a different network type to the master wallet. This seems erroneous and just plain confusing.
|
Integration tests are failing, seems to be a problem with helloblock. @sidazhang @locksley? For what its worth, the error messages the helloblock node package gives are impossible to debug. |
src/wallet.js
Outdated
There was a problem hiding this comment.
- why not be consistent and use assert here as well?
- the original
checkDustchecks fornullandundefinedas well. Is there a particular reason why you dropped it?
There was a problem hiding this comment.
- Agreed, I'll add on a commit.
- Any invalid
values will be caught byTransaction.toBuffer. Even though its not explicit. The same mechanism is being used fortoaddress validation (aka,address.toScriptPubKey() is undefined). If we want to be stricter about parameter validation, probably better to push it down the tree (maybe inTransaction.addOutput).
There was a problem hiding this comment.
Also, as mentioned on IRC, undefined compared to any number is false, so it would always have thrown anyway.
|
Sure they are not truly async and I don't mind having only synchronous methods. But I do think the callback interface is nicer for error handling and more familiar to node devs. At some point I'd like to see us migrating to callback interfaces from throwing errors. |
|
Kicked off the build again. All passed this time. @sidazhang @locksley FYI, the failing build: https://travis-ci.org/bitcoinjs/bitcoinjs-lib/builds/26368607 |
|
+1 when you're ready |
|
If the coverage message is annoying, just let me know and I'll try to turn it off. |
|
+1 Haha :)
|
Taken from browserify-buffer. Also adds a few more tests to assert this is working correctly from both read and write perspectives. The assertion in for writePushDataInt in the 32 bit case was unnecessary as that is handled by buffer.writeUInt32LE anyway.
|
Rebased last 3 commits and fixed the wallet error messages as per the discussion. @kyledrake it's my fault, it posts whenever I rebase a commit. Perhaps I'll need to iterate with @weilu and others before I post it here, so we can clean up smaller issues and then only commit on top of the PRs. |
|
No worries, I'm totally cool with you doing that. |
5 participants
This pull request cleans up
HDWalletandWalletjust a little bit.It first fixes a bug in
createTxwhich allowed the creation of transactions even when no UTXOs existed. Tests have been amended to ensure this does not regress.I've then removed all the
Walletasync interfaces (which weren't actually async anyway), with the proposed alternative for users to use something like https://github.com/caolan/async#nextTick.hashLittleEndianis a small convenience option for people using blockchain.info's inconsistent endianness in their API; and therefore has no business being in our interface, and has thus been removed. This further reduces parameter complexity for theWalletinterface.I have also enforced consistent use of
networkthroughout the project by avoiding stringly typed parameters wherever possible, and instead just using the network objects as they were intended.Also, in accordance with the standards we chose a while back, constructors should always use operator new where possible.
Finally I added some exception messages and tests for
HDWallet.fromBase58.