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: add getzmqnotifications RPC #1587

Closed
wants to merge 0 commits into from

Conversation

torkelrogstad
Copy link
Contributor

This PR is #1555 take 2. I force-pushed to the wrong branch when responding to review, which closed the PR automatically. GitHub then complained that there were no new commits on my branch, so I could not reopen the PR. I'm therefore opening this instead.

@onyb note that I responded to your review comments.

@onyb
Copy link
Contributor

onyb commented Jun 29, 2020

@torkelrogstad This needs a rebase, and fixing of failing unit-tests.

@torkelrogstad torkelrogstad force-pushed the master branch 3 times, most recently from 6082bd9 to 18e9886 Compare June 30, 2020 11:32
@torkelrogstad
Copy link
Contributor Author

@onyb Rebased

rpcclient/examples/bitcoincorehttp/main.go Outdated Show resolved Hide resolved
@@ -398,6 +399,63 @@ type GetRawMempoolVerboseResult struct {
Depends []string `json:"depends"`
}

// GetZmqNotificationResult models the data returned from the getzmqnotifications command.
type GetZmqNotificationResult []struct {
Type string // Type of notification
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use an enumeration for the Type. According to the Bitcoin Core docs, the possible notification types are very limited.

  • pubhashtx
  • pubhashblock
  • pubrawblock
  • pubrawtx

What do you think?

@@ -668,6 +668,15 @@ func NewInvalidateBlockCmd(blockHash string) *InvalidateBlockCmd {
}
}

// GetZmqNotificationsCmd defines the getzmqnotifications JSON-RPC command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to my comment on your other PR:

Should we put zmq stuff in separate files (ex: zmq*.go), or go with chain*.go files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to ZMQ prefixed files. Kept the test cases where they are though, to avoid repeating all the boilerplate

btcjson/chainsvrresults.go Outdated Show resolved Hide resolved
@jcvernaleo
Copy link
Member

Looks like this needs a rebase to get the correct github actions and to deal with some conflicts.

@torkelrogstad
Copy link
Contributor Author

Git remotes are hard...

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.

4 participants