Skip to content
This repository was archived by the owner on Apr 25, 2024. It is now read-only.

Configurable client#365

Closed
bucko13 wants to merge 4 commits intounchained-capital:masterfrom
bucko13:configurable-client
Closed

Configurable client#365
bucko13 wants to merge 4 commits intounchained-capital:masterfrom
bucko13:configurable-client

Conversation

@bucko13
Copy link
Contributor

@bucko13 bucko13 commented Jan 15, 2024 •
edited
Loading

Description

With a recent release adding mempool.space support (#347), a few issues came to light, which this PR seeks to address.

Problems

Rate Limiting in mempool.space

The biggest one noticed in #364 was that recent rate limiting implemented on mempool.space's main production endpoint caused significant performance degradation. Throttling was already an issue with blockstream for which we added a delay to avoid hitting the rate limit, but this may not be enough.

Hard to change client

Unfortunately the way to change the client had to come from unchained-bitcoin, which is really inconvenient and probably not the right long term place for this.

Clumsy API

Every api call had to pass in the network and client info even though once the wallet is setup, this doesn't change. Would be nice to just get this from the store and be done.

Mempool not a perfect drop-in replacement

One of the api calls (getFeeEstimates) was not quite a drop-in b/w mempool and blockstream. So this might've been broken as well

Hard to make changes

If there are any changes in the future (immediate example: mempool is going to have a websocket option for using as a wallet backend), it's not easy to update the client without backwards incompatible changes.

Solution

  • This PR first moves all the client relevant code into their own module (c90211d).
  • Then it introduces a new BlockchainClient class (d3201c2), which once initialized with the connection details shares an api no matter what client you're connecting with
  • This class supports bitcoind, mempool, or blockstream. This moves the connection info from unchained-bitcoin. Longer term, I think this should exist in its own package to be maintained separately. An external package shouldn't dictate what the public client endpoint is, but it makes sense to have a unified (and well tested) package for communicating with the blockchain. The consumer (in this case the caravan coordinator) can decide which it wants to use when initialized and just go from there.
  • the UI is still very tightly coupled with redux, so this PR adds some code f591a0e to the store and actions to set the client and get the client (based on other client settings in the store) for components to use as needed.
  • TypeScript and tests. TS isn't as useful as it could be without many TS consumers but it's a start.
  • The class also has built-in, configurable support for throttling if necessary to avoid rate limiting in a way that should be easy to turn on and off
  • the update in clientActions shows how easy it should be to support multiple clients. It's one line now to change the default public b/w blockstream and mempool and eventually we should easily be able to support either and leave it up to the user.

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR fix an open issue?

@bucko13 bucko13 requested a review from waldenraines January 15, 2024 04:08
@bucko13 bucko13 force-pushed the configurable-client branch 2 times, most recently from f591a0e to 1d077e7 Compare January 15, 2024 15:58
@bucko13 bucko13 force-pushed the configurable-client branch from 1d077e7 to 5b5d127 Compare January 29, 2024 15:48
bucko13 added a commit to caravan-bitcoin/caravan that referenced this pull request Feb 12, 2024
@bucko13
add caravan/clients package
76e3d19 
Based on code from PR to legacy repo: unchained-capital/caravan#365
@carlaiau
Copy link

carlaiau commented Feb 20, 2024

Is there an ETA of this getting merged into main?

@carlaiau
Copy link

carlaiau commented Feb 22, 2024 •
edited
Loading

Blockstream API broadcast accepts raw tx not JSON object. I am experiencing a CORS policy failure on preflight as well as the broadcast itself. Copying the HEX and posting it via CURL works successfully.

public async broadcastTransaction(rawTx: string): Promise<any> {
    try {
      if (this.type === ClientType.PRIVATE) {
        return bitcoindSendRawTransaction({
          hex: rawTx,
          ...this.bitcoindParams,
        });
      }
      return await this.Post(`/tx`, { tx: rawTx });
    } catch (error) {
      throw new Error(`Failed to broadcast transaction: ${error.message}`);
    }
  }

@bucko13
Copy link
Contributor Author

bucko13 commented Feb 27, 2024

Added in the caravan monorepo now live as of d1d169

@bucko13 bucko13 closed this Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@waldenraines waldenraines Awaiting requested review from waldenraines

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error Loading Wallet from JSON Configuration Files in Caravan Multisig

2 participants

@bucko13 @carlaiau