search

ruimarinho / bitcoin-core

A modern Bitcoin Core REST and RPC client.
477 stars 186 forks source link

Add new bitcoin@0.17.0 RPC methods to sign raw transactions #80

Closed lautarodragan closed 5 years ago

lautarodragan commented 5 years ago

Part of https://github.com/ruimarinho/bitcoin-core/issues/77

Not sure this is right. I just copy-pasted the current signRawTransaction, changing the version and name, and updated the version for the deprecated signRawTransaction.

Tested the "happy path" case — didn't test that the version filters are working, only that I could correctly call this method. That much works well.

lautarodragan commented 5 years ago

Thank you for your review @pedrobranco!

I went ahead and made the requested changes on signRawTransactionWithWallet.

I also added signRawTransactionWithKey as requested, but I'm not sure whether the obfuscate field is right. (Basically copied what is in place for the deprecated signRawTransaction).

The docs for signRawTransactionWithKey state:

Sign inputs for raw transaction (serialized, hex-encoded). The second argument is an array of base58-encoded private keys that will be the only keys used to sign the transaction. The third optional argument (may be null) is an array of previous transaction outputs that this transaction depends on but may not yet be in the block chain.

pedrobranco commented 5 years ago

Thank you for your review @pedrobranco!

Anytime.

I also added signRawTransactionWithKey as requested, but I'm not sure whether the obfuscate field is right. (Basically copied what is in place for the deprecated signRawTransaction).

The old one was the third argument, in this new method is the second argument. I've already pushed a comment here.

BTW, there is another PR for adding these methods #84. I think we can merge your PR first (despite this PR being a subset of the #84 ).

And many thanks for the pull request 👍

lautarodragan commented 5 years ago

@pedrobranco all done I think! Awaiting your input regarding the upper bound in the version range.

pedrobranco commented 5 years ago

@pedrobranco all done I think! Awaiting your input regarding the upper bound in the version range.

Done.

lautarodragan commented 5 years ago

@pedrobranco added the multi wallet feature and removed the version cap. I need to squash again, but other than that, is it looking good?

pedrobranco commented 5 years ago

@pedrobranco added the multi wallet feature and removed the version cap. I need to squash again, but other than that, is it looking good?

Just add a test for the new multi-wallet method just to be sure that it works fine. To help I've bumped the docker image for the multi-wallet daemon to 0.17.

pedrobranco commented 5 years ago

@lautarodragan Any news on the missing test? We would like to add this to the new release.

lautarodragan commented 5 years ago

@pedrobranco sorry for the delay! just pushed a commit with mutli wallet tests.

jeamick commented 5 years ago

Hey @pedrobranco & @lautarodragan,

Any news for this PR ? do we just need to change the commit message ? Thanks

Kind regards JM

@pedrobranco @lautarodragan

lautarodragan commented 5 years ago

@jeamick @pedrobranco I'll squash the commits but I'd rather wait for feedback first.

I'd like to suggest enabling squash-merging for this repository. It allows a clean master without losing details / history in PRs, and simplifies communication between maintainers and contributors.

jeamick commented 5 years ago

I did the change and compiled it into my repository and it works for signRawTransactionWithKey. here is the link : https://github.com/jeamick/bitcoin-core

jeamick commented 5 years ago

Any update on this PR ? @lautarodragan @pedrobranco

lautarodragan commented 5 years ago

@jeamick I think we can consider this repo pretty much abandoned. I decided to fork it and publish here.

Haven't had a chance to try it yet, if you do please let me know.

I'll be making several changes to the project soon. I intend to keep it compatible with new bitcoin-core releases and add TypeScript type definitions (which I already have for some calls in a different project), as well as fixing some limitations we faced at Po.et.

ruimarinho commented 5 years ago

This repo is definitely not abandoned @lautarodragan, but we have been focused on other projects at the moment. This library remains tried and tested. Looking forward to bring back my attention to it.

Which limitations are you trying to solve? Are those public?

lautarodragan commented 5 years ago

@ruimarinho that's great news! I think the biggest issue is how we're lagging behind Bitcoin Core releases.

Adding TypeScript definitions and making it compatible with modern ES6 modules would be nice too (although you are setting exports.default = Client so maybe it was an issue on my side).

Another issue I found, albeit minor, was trying to use the methods "outside" the object. I don't know how to explain this properly, but for example doing const { createRawTransaction } = bitcoinCore; createRawTransation(...) failed, if my memory serves me right. Had the same issue trying to pass the functions to Ramda's pipe. I think .bind(bitcoinCore) solved it, but it'd be nice not to need that binding, but it's not very idiomatic.

jeamick commented 5 years ago

@jeamick I think we can consider this repo pretty much abandoned. I decided to fork it and publish here.

Haven't had a chance to try it yet, if you do please let me know.

I'll be making several changes to the project soon. I intend to keep it compatible with new bitcoin-core releases and add TypeScript type definitions (which I already have for some calls in a different project), as well as fixing some limitations we faced at Po.et.

I already did the changes on my repo and it is working : https://github.com/jeamick/bitcoin-core

lautarodragan commented 5 years ago

Awesome! Thanks @jeamick. Please let me know if you give the published version a try.

jeamick commented 5 years ago

Awesome! Thanks @jeamick. Please let me know if you give the published version a try.

Which published version ?

I am actually using it for a personal project involving RPC request to bitcoin-core node and everything is working as expected

lautarodragan commented 5 years ago

Awesome! Thanks @jeamick. Please let me know if you give the published version a try.

Which published version ?

I am actually using it for a personal project involving RPC request to bitcoin-core node and everything is working as expected

This one: https://www.npmjs.com/package/bitcoins/v/1.0.1

pedrobranco commented 5 years ago

I will submit the fix for the tests and merge this. Also, I've squashed all commits.