Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

btcjson,rpcclient: add support for PSBT commands to rpcclient #1596

Merged
merged 1 commit into from
Sep 14, 2020
Merged

btcjson,rpcclient: add support for PSBT commands to rpcclient #1596

merged 1 commit into from
Sep 14, 2020

Conversation

federicobond
Copy link
Contributor

No description provided.

@federicobond federicobond force-pushed the psbt branch 2 times, most recently from 46b0594 to 1ee0001 Compare June 18, 2020 20:18
@federicobond federicobond mentioned this pull request Jun 28, 2020
Closed
@Rjected
Copy link
Contributor

Rjected commented Jul 17, 2020

Hello, you'll need to fix the CI issues in order for this to get reviewed and merged. Thanks for the contribution!

@federicobond
Copy link
Contributor Author

@Rjected thanks for the heads up! Yeah, its weird though, I could not find out what exactly is it complaining about, the file appears formatted correctly.

@Rjected
Copy link
Contributor

Rjected commented Aug 24, 2020

Hey, sorry for getting back so late. Here are the changes you'd need to do to fix the formatting issues: Rjected@23478ac . There are also now some conflicts, make sure those are fixed as well and then we can start reviewing!

@federicobond
Copy link
Contributor Author

Ah, that makes sense. Thanks for the heads up.

@federicobond
Copy link
Contributor Author

This is now ready for review.

onyb
onyb suggested changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@onyb onyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the PR!

Most of my remarks are about types. Please also feel free to pull in some descriptions from Bitcoin Core docs wherever relevant. It doesn't need to be an exact copy, but just enough to guide the users and show it beautifully on pkg.go.dev.

rpcclient/wallet.go Show resolved Hide resolved
rpcclient/wallet.go Outdated Show resolved Hide resolved
rpcclient/wallet.go Outdated Show resolved Hide resolved
rpcclient/wallet.go Outdated Show resolved Hide resolved
btcjson/walletsvrresults.go Show resolved Hide resolved
btcjson/btcwalletextcmds.go Outdated Show resolved Hide resolved
btcjson/btcwalletextcmds.go Outdated Show resolved Hide resolved
btcjson/btcwalletextcmds.go Outdated Show resolved Hide resolved
btcjson/btcwalletextcmds.go Outdated Show resolved Hide resolved
btcjson/btcwalletextcmds.go Outdated Show resolved Hide resolved
@onyb
Copy link
Contributor

onyb commented Aug 27, 2020

I'm wondering if btcwalletextcmds.go is the right file for defining the types. This is what the header of the file says:

btcd/btcjson/btcwalletextcmds.go

Lines 5 to 6 in d2b1a06

// NOTE: This file is intended to house the RPC commands that are supported by
// a wallet server with btcwallet extensions.

Compare that to walletsvrcmds.go, where we have this:

btcd/btcjson/walletsvrcmds.go

Lines 5 to 6 in d2b1a06

// NOTE: This file is intended to house the RPC commands that are supported by
// a wallet server.

Using walletsvrcmds.go would be more appropriate.

@federicobond federicobond force-pushed the psbt branch 2 times, most recently from 83e5cef to bec3737 Compare August 27, 2020 17:25
@federicobond
Copy link
Contributor Author

I moved the PSBT methods to walletsvrcmds.go as suggested.

@federicobond federicobond force-pushed the psbt branch 3 times, most recently from 507b6fc to 08000bc Compare August 28, 2020 00:25
@jakesylvestre jakesylvestre mentioned this pull request Aug 28, 2020
Open
onyb
onyb reviewed Aug 28, 2020
View reviewed changes
btcjson/chainsvrcmds.go Outdated Show resolved Hide resolved
onyb
onyb suggested changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@onyb onyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more minor review comments.

btcjson/walletsvrcmds.go Outdated Show resolved Hide resolved
btcjson/walletsvrcmds.go Outdated
Comment on lines 766 to 773
var (
SighashAll SighashType = "ALL"
SighashNone SighashType = "NONE"
SighashSingle SighashType = "SINGLE"
SighashAllAnyoneCanPay SighashType = "ALL|ANYONECANPAY"
SighashNoneAnyoneCanPay SighashType = "NONE|ANYONECANPAY"
SighashSingleAnyoneCanPay SighashType = "SINGLE|ANYONECANPAY"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have comments here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just noticed that rpcclient already defines a SigHashType type. I think it will be more clear to use that and then convert to *string for the btcjson constructors (to avoid a circular import). This is consistent with the behaviour of SignRawTransaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SigHashType should've really been in btcjson package. But there's nothing we can do about it now, without introducing a breaking change. Let's keep it for later.

btcjson/walletsvrcmds.go Outdated Show resolved Hide resolved
@federicobond federicobond force-pushed the psbt branch 2 times, most recently from 6f1c59e to 87e49cd Compare September 1, 2020 00:32
@federicobond
Copy link
Contributor Author

Fixed the conflicts, should be ready to merge.

onyb
onyb approved these changes Sep 5, 2020
View reviewed changes
Copy link
Contributor

@onyb onyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good on my side!

rpcclient/wallet.go Outdated Show resolved Hide resolved
@onyb onyb added this to the 0.22.0 milestone Sep 7, 2020
jcvernaleo
jcvernaleo approved these changes Sep 8, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@jakesylvestre jakesylvestre mentioned this pull request Sep 8, 2020
Merged
jakesylvestre
jakesylvestre reviewed Sep 8, 2020
View reviewed changes
rpcclient/wallet.go
) (*btcjson.WalletProcessPsbtResult, error) {
return c.WalletProcessPsbtAsync(psbt, sign, sighashType, bip32Derivs).Receive()
}

// TODO(davec): Implement
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might make sense to remove these in favor of issues, but not germane to this review

@jcvernaleo jcvernaleo merged commit 6f49f1f into btcsuite:master Sep 14, 2020
@onyb onyb mentioned this pull request Apr 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rjected Rjected Rjected left review comments

@jakesylvestre jakesylvestre jakesylvestre approved these changes

@onyb onyb onyb approved these changes

@jcvernaleo jcvernaleo jcvernaleo approved these changes

Assignees
No one assigned
Labels
enhancement rpc
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.
5 participants
@federicobond @Rjected @onyb @jcvernaleo @jakesylvestre