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

feat: generic api failover client + bitcoin client proof-of-concept with it #549

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Sep 19, 2024

Description

Adds a generic fallback/retry type to be used with API clients. Provides the first integration with the BitcoinCoreClient and the BlockObserver to ensure that everything plays nicely together and to show its usage.

The goal here is to ensure that we have some measure of resiliency in our external API calls without needing to duplicate that kind of code everywhere (i.e. Bitcoin, Stacks, Emily & Blocklist clients).

I was a little bit back-and-forth on whether or not to put the API clients on the context, but in the end decided to so that they can be created at application startup and the fallback client's state (i.e. current "live" endpoint) shared with the rest of the app. The context now has two methods for newing up: init() is more implicit but requires more trait bounds, where new() can be used in tests to provide concrete impls where needed (i.e. the TestHarness in block_observer.rs).

The fallback client's logic is pretty naive as it is right now, but we can expand upon this later to add exponential backoffs per endpoint, simpler load-balancing across endpoints (round-robin or lowest-latency, for example), etc. if we want.

Closes:

Works towards:

Changes

  • Adds the ApiFallbackClient type which can be used to failover to a new endpoint while also supporting retries (default is 3 retries or the number of clients, whichever is higher). Probably not the best name tbh since it's not necessarily specific to API's, but that's what we use it for right now 🤷
  • Reworked the sbtc library errors a little so that we get rid of the dyn Error and problems with Send.
  • Refactored load_latest_deposit_requests() in the BlockObserver which otherwise had problems with type signatures (between signer and sbtc crates) and wouldn't use the failover functionality otherwise.
  • Added the bitcoin client to the context and implemented this in the BlockObserver.
  • Some other small stuff I saw.

Testing Information

Nothing specific, all existing tests pass and there are new tests testing the ApiFallbackClients failover and retry functionality.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cylewitruk
Copy link
Member Author

Looks like this PR is hitting the http 403 issue too... 🤔

}
}

impl BitcoinInteract for BitcoinCoreClient {
Copy link
Member Author

@cylewitruk cylewitruk Sep 19, 2024

Choose a reason for hiding this comment

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

Yeah I coulda implemented BitcoinInteract for ApiFallbackClient<BitcoinCoreClient> and handled the exec calls internally, then kept ApiFallbackClient out of the Context.. but I didn't... maybe that's the better way to go but I'm conflicted, there's "syntactical" downsides to that too 🤔

Copy link
Member Author

@cylewitruk cylewitruk Sep 19, 2024

Choose a reason for hiding this comment

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

I spoke with Matteo and he seemed to agree with the thoughts that my lazy-angel wanted to ignore (i.e. the "yeah i coulda" above), so I'll make that change.

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.

1 participant