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

Zeromq bitcoind subscriptions #390

Closed
wants to merge 14 commits into from

Conversation

jotapea
Copy link
Contributor

@jotapea jotapea commented Jul 29, 2021

src/bitcoind.ts Outdated Show resolved Hide resolved
src/bitcoind.ts Outdated
txid: string
include_watchonly?: boolean
}): Promise<InWalletTransaction> {
return await this.client.getTransaction({ txid, include_watchonly })
Copy link
Member

Choose a reason for hiding this comment

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

Why are you sometimes using the default client and sometimes this.client? I think I'd prefer to always use this.client and pass in the default client via constructor if that works.

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.client is also the instance variable name used in the BitcoindWalletClient because it actually is a client just like in the BitcoindClient class. The only difference is that in BitcoindWalletClient it was instantiated with the wallet argument. This is the way for the BitcoindWalletClient instances to contact the same "default" (main) client while being "loaded" and ready to invoke wallet rpc calls.

src/bitcoindSubscribers.ts Outdated Show resolved Hide resolved
@@ -21,6 +27,22 @@ it("connects to bitcoind", async () => {
expect(chain).toEqual("regtest")
})

it("detect active bitcoind zeromq notifications", async () => {
const notifications = await bitcoindClient.getZmqNotifications()
expect(notifications.length).toEqual(4) // 2 per topic
Copy link
Member

@bodymindarts bodymindarts Jul 29, 2021

Choose a reason for hiding this comment

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

Where is the code that creates these notifications? This is very implicit and order dependent. Perhaps you could make this idempotent like so:

const beforeLength = await bitcoindClient.getZmqNotifications().length
// execute some tx
const notifications = await bitcoindClient.getZmqNotifications()
expect(notifications.length).toEqual(beforLength + 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling zeromq is set in bitcoind configuration, and it was already enabled. So that call is not being affected by this branch.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. This is a new test right? So can we try to make any new tests we add be idempotent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a newly added test yes, but is not actually testing anything that has a "before" and "after". It is just detecting if the bitcoind client has the zeromq notifications enabled.

for await (const [topic, msg] of sock) {
if (topic.toString() === subscribeTopic) {
const rawTx = msg.toString("hex")
const tx = await bitcoindDefaultClient.decodeRawTransaction({ hexstring: rawTx })
Copy link
Member

Choose a reason for hiding this comment

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

can we always pass in a client and not ever use the default client except on the top level when we inject it? This would remove coupling to global state and improve testability.

@jotapea
Copy link
Contributor Author

jotapea commented Jul 29, 2021

@bodymindarts The way I am approaching it is that the wallet-clients are separate from the "loading of" or "total of" loaded wallets in the default (main) client. So, to use a wallet, you actually need a new wallet-client for this. Also the classes are done in a way that their methods don't overlap (at least for now).

@jotapea
Copy link
Contributor Author

jotapea commented Jul 30, 2021

@nicolasburtey @bodymindarts I think this is ready to get merged, at least from the perspective of testing. I am not sure where else changes need to be done for production compatibility (i.e. equivalent to the opening of the ports in docker-compose).

Copy link
Member

@bodymindarts bodymindarts left a comment

Choose a reason for hiding this comment

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

The PR looks good but still you are coupling to global state which we are trying to move away from for better testability / code structure / modularisation etc...

@@ -267,7 +267,7 @@ export class SpecterWallet {
throw new Error(err)
}

const tx = await this.bitcoindClient.getTransaction({
const tx = await bitcoindDefaultClient.getTransaction({
Copy link
Member

Choose a reason for hiding this comment

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

Can we please avoid coupling to global state / objects. No singletons... use constructor injection.

for await (const [topic, msg] of sock) {
if (topic.toString() === BITCOIND_EVENTS.RAW_TX) {
const rawTx = msg.toString("hex")
const tx = await bitcoindDefaultClient.decodeRawTransaction({ hexstring: rawTx })
Copy link
Member

Choose a reason for hiding this comment

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

Can we please not reference singletons / depend on global state?

@jotapea
Copy link
Contributor Author

jotapea commented Aug 3, 2021

@bodymindarts bitcoindDefaultClient is now fully removed.

Copy link
Member

@bodymindarts bodymindarts left a comment

Choose a reason for hiding this comment

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

Please remove the TODO comments. If you are unsure then ask in PR discussion don't commit this noise to the code.
Other than that its looking fine.

@@ -61,10 +61,14 @@ export const getOnChainTransactions = async ({
export const OnChainMixin = (superclass) =>
class extends superclass {
readonly config: UserWalletConfig
// TODO? is this ok here?
Copy link
Member

Choose a reason for hiding this comment

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

Why the comment in code? Please remove


constructor(...args) {
super(...args)
this.config = args[0].config
// TODO? is this ok here?
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be committed to code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bodymindarts Comment removed, and I assume it is ok to add the bitcoind client as an instance variable to the "mixin", at least for now.

readonly logger: Logger
readonly config: SpecterWalletConfig

constructor({ logger, config }: SpecterWalletConstructorArgs) {
// TODO? Add BitcoindClient type to SpecterWalletConstructorArgs
Copy link
Member

Choose a reason for hiding this comment

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

Why the comment with a question? Either do it or don't or bring it up for discussion in PR review... not in code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I add the BitcoindClient type to the SpecterWalletConstructorArgs? @bodymindarts @samerbuna @dolcalmi

export enum BITCOIND_EVENTS {
RAW_BLOCK = "rawblock",
RAW_TX = "rawtx",
// HASH_BLOCK = "hashblock",
Copy link
Member

Choose a reason for hiding this comment

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

Why keep commented code here?

Copy link
Contributor Author

@jotapea jotapea Aug 5, 2021

Choose a reason for hiding this comment

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

For this one I am not really sure why we have to remove them. It is there for completeness of the events that bitcoind emits. And they are commented out because we don't currently use them, but we could in the future.

- estimateSmartFee added to BitcoindClient
- listTransactions added to BitcoindWalletClient
- getTransaction moved to BitcoindWalletClient
- getNewAddress now accepts optional address_type param
- sendToAddress now accepts optional fee_rate param
@samerbuna samerbuna marked this pull request as draft September 13, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants