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.

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

rpcclient: Export symbols needed for custom commands #1457

Merged
merged 4 commits into from
Sep 2, 2021

Conversation

JeremyRand
Copy link
Contributor

Fixes #1083

I'm not particularly familiar with the btcsuite codebase yet, so hopefully I didn't break anything with this patch. (I assume I'll notice quickly if Travis complains.)

JeremyRand added a commit to namecoin/ncdns that referenced this pull request Oct 27, 2019
2b1eed8 Switch to new rpcclient based on latest upstream Conformal (JeremyRand)

Pull request description:

  Namecoin's fork of Conformal's RPC client library is ancient and unmaintained; switching to upstream eliminates a major source of potential bugs (and known bugs, e.g. the ConsensusJ and Electrum compatibility bugs that we had to patch).

  Fixes #9

  TODO:

  - [ ] Wait for Conformal to merge btcsuite/btcd#1457
  - [ ] Wait for Conformal to merge btcsuite/btcd#1460
  - [x] Push `name_show` support for `ncjson` and `ncrpcclient`
  - [x] Implement cookie authentication
  - [x] Test cookie authentication

ACKs for commit 2b1eed:

Tree-SHA512: 157780613661af240d83a78d66386c66fcfeed0700088d263a20389c45b200db1b651ea985f345de2ac2f250bdafcc4cd5901579e9f5a97ddc13e77e7a7bcf39
@octobocto
Copy link

What is the motivation for this change though? As far as I can tell, it isn't used anywhere

@torkelrogstad
Copy link
Contributor

A use case is implementing a RPC for an unreleased version of Core/btcd, or simply to add a RPC locally which isn't available in rpcclient yet. Concept ACK, would be nice with some documentation around how to add a custom comand.

@JeremyRand
Copy link
Contributor Author

would be nice with some documentation around how to add a custom comand.

@torkelrogstad Namecoin is adding custom commands in these repos:

Should be pretty easy to figure out how to do it based on the code in those two repos. If desired, I could add links to those two repos to the readme or something.

@torkelrogstad
Copy link
Contributor

I think linking to code in a different repo is not really something we should do here. A simple demonstration of how to add a RPC would be more than enough, should be 20 lines of code or so?

@torkelrogstad
Copy link
Contributor

Also, this PR needs a rebase

@JeremyRand
Copy link
Contributor Author

JeremyRand commented Dec 17, 2019

I think linking to code in a different repo is not really something we should do here. A simple demonstration of how to add a RPC would be more than enough, should be 20 lines of code or so?

@torkelrogstad No objection from me. I probably won't be able to get to it until early January though. (Ditto for the rebase.)

@JeremyRand
Copy link
Contributor Author

Rebased. @torkelrogstad Should the sample code for using a custom command go in the rpcclient/examples folder, or is there a better place for it? Should that sample code use a fictional RPC method, a real altcoin RPC method (I'd probably do Namecoin's name_show), or is there some other approach for demonstrating it?

@torkelrogstad
Copy link
Contributor

I think rpcclient/examples makes sense, and also doing a real example (that people could run if they have a local Namecoin node) would be cool

@jakesylvestre
Copy link
Collaborator

@jcvernaleo (as per #1530)

  • low priority
  • enhancement

Probably needs some work as per @torkelrogstad

@JeremyRand
Copy link
Contributor Author

Rebased and added example. @torkelrogstad How does it look now? (I'm not 100% sure about the CI failure, but at first glance it looks unrelated to this PR.)

Copy link
Contributor

@torkelrogstad torkelrogstad left a comment

Choose a reason for hiding this comment

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

Couple of comments on the wording in the README, other than that this looks good to me. Also looks like there's a CI failure

Comment on lines +25 to +28
```bash
$ cd $GOPATH/src/github.com/btcsuite/btcd/rpcclient/examples/customcommand
$ go run *.go
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This example assumes that the user is using Go modules, which is no longer the default with newer Go versions. I'd suggest replacing 9-13 with:

The first step is to clone the btcd repo:

$ git clone github.com/btcsuite/btcd
$ cd btcd

And then replace 23-28 with:

Finally, run the example:

$ go run rpcclient/examples/customcommand/main.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Readme is copied nearly verbatim from the bitcoincorehttp example's Readme; for consistency I prefer not to make the requested changes as part of this PR (but I'd be fine with those changes as a separate PR that covers both examples' Readmes).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the changes suggested here, but I'm happy to take care of it in a separate PR.

@JeremyRand
Copy link
Contributor Author

Triggering a CI rebuild to see if the failure was a fluke...

@JeremyRand JeremyRand closed this May 21, 2020
@JeremyRand JeremyRand reopened this May 21, 2020
@JeremyRand
Copy link
Contributor Author

Triggering a CI rebuild to see if the failure was a fluke...

Yes, CI appears to have succeeded this time.

@coveralls
Copy link

coveralls commented Dec 21, 2020

Pull Request Test Coverage Report for Build 993591029

  • 0 of 322 (0.0%) changed or added relevant lines in 9 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.008%) to 52.892%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcclient/rawrequest.go 0 2 0.0%
rpcclient/extensions.go 0 18 0.0%
rpcclient/notify.go 0 18 0.0%
rpcclient/net.go 0 20 0.0%
rpcclient/infrastructure.go 0 23 0.0%
rpcclient/mining.go 0 24 0.0%
rpcclient/rawtransactions.go 0 27 0.0%
rpcclient/chain.go 0 56 0.0%
rpcclient/wallet.go 0 134 0.0%
Files with Coverage Reduction New Missed Lines %
btcec/signature.go 3 92.76%
Totals Coverage Status
Change from base Build 896856960: -0.008%
Covered Lines: 21015
Relevant Lines: 39732

💛 - Coveralls

@JeremyRand
Copy link
Contributor Author

Rebased to fix merge conflicts. Would be great if this can be reviewed and/or merged.

Copy link
Contributor

@torkelrogstad torkelrogstad left a comment

Choose a reason for hiding this comment

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

rpcclient/examples/customcommand/main.go needs to be formatted with gofmt

Comment on lines 938 to 1007
func (c *Client) sendCmdAndWait(cmd interface{}) (interface{}, error) {
func (c *Client) SendCmdAndWait(cmd interface{}) (interface{}, error) {
// Marshal the command to JSON-RPC, send it to the connected server, and
// wait for a response on the returned channel.
return receiveFuture(c.sendCmd(cmd))
return ReceiveFuture(c.SendCmd(cmd))
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of deleting this method? It is only used a single place. It can be replaced by inling the contents here. That way there's one less public symbol exported, which is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, making that one public was unintentional; seems I was a tad sloppy while rebasing. Reverted it to private; thanks for catching that.

rpcclient/examples/customcommand/main.go Show resolved Hide resolved
@JeremyRand JeremyRand force-pushed the public-sendcmd branch 4 times, most recently from 56a3124 to 48a1ec7 Compare February 11, 2021 10:14
@JeremyRand
Copy link
Contributor Author

rpcclient/examples/customcommand/main.go needs to be formatted with gofmt

@torkelrogstad Thanks for catching that; fixed.

@JeremyRand JeremyRand force-pushed the public-sendcmd branch 2 times, most recently from 960ae63 to 96fed42 Compare June 17, 2021 02:48
This facilitates using custom commands with rpcclient.

See btcsuite#1083
This facilitates using custom commands with rpcclient.

See btcsuite#1083
@JeremyRand
Copy link
Contributor Author

Rebased again. Would be awesome if we can get this reviewed and merged.

@jcvernaleo jcvernaleo added this to the 0.22.1 milestone Jun 17, 2021
@jcvernaleo
Copy link
Member

Thanks @JeremyRand I don't have time to review this morning but I added it to the milestone for the next release so it is at the top of the list.

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.

One minor thing from me then looks good.

@@ -0,0 +1,32 @@
Namecoin Core Custom Command Example
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving the other parts of the readme as is and then update with a future PR, but I think we should fix the name at least so it isn't Namecoin

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this in a separate PR. I think just using "Custom Command Example" is appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

@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.

This PR needs some very minor cosmetic changes, but let's do that in a different PR.

rpcclient/examples/customcommand/main.go Show resolved Hide resolved
@@ -0,0 +1,32 @@
Namecoin Core Custom Command Example
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this in a separate PR. I think just using "Custom Command Example" is appropriate.

Comment on lines +25 to +28
```bash
$ cd $GOPATH/src/github.com/btcsuite/btcd/rpcclient/examples/customcommand
$ go run *.go
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the changes suggested here, but I'm happy to take care of it in a separate PR.

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.

I think this is good to go.
OK

@JeremyRand
Copy link
Contributor Author

Any chance we can get this merged? AFAICT there are no remaining blockers (unless I missed something?).

@onyb onyb merged commit 3e2d846 into btcsuite:master Sep 2, 2021
@onyb
Copy link
Collaborator

onyb commented Sep 2, 2021

Squashed and merged. Thanks @JeremyRand for the PR. 🙌

@jcvernaleo
Copy link
Member

@JeremyRand my apologies. I see that I approved this a while ago but forgot to actually merge it. Sorry about that. Thanks for the reminder.

@JeremyRand JeremyRand deleted the public-sendcmd branch October 2, 2021 17:27
kcalvinalvin pushed a commit to kcalvinalvin/btcd that referenced this pull request Nov 17, 2021
* rpcclient: Export sendCmd and response

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Export receiveFuture

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Add customcommand example

* rpcclient: remove "Namecoin" from customcommand readme heading
Elbandi pushed a commit to Elbandi/btcd that referenced this pull request Apr 12, 2022
* rpcclient: Export sendCmd and response

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Export receiveFuture

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Add customcommand example

* rpcclient: remove "Namecoin" from customcommand readme heading
Elbandi pushed a commit to Elbandi/btcd that referenced this pull request Apr 12, 2022
* rpcclient: Export sendCmd and response

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Export receiveFuture

This facilitates using custom commands with rpcclient.

See btcsuite#1083

* rpcclient: Add customcommand example

* rpcclient: remove "Namecoin" from customcommand readme heading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpcclient: Missing custom command support
7 participants