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

Refactoring Spotbit #55

Closed
wants to merge 9 commits into from
Closed

Conversation

nochiel
Copy link
Contributor

@nochiel nochiel commented Mar 6, 2022

To be done in separate PRs.

  • Tests
    • Tests for APIs.
    • Tests for the database to makes sure data is stored correctly.
  • Migration
    • Update README with breaking changes.
    • Migration script. (Do we need it?)
    • Migrate web app to new API.
  • Improve installation process?

@nochiel nochiel requested a review from ChristopherA as a code owner March 6, 2022 00:12
@nochiel nochiel changed the title Refactoring Spotbit WIP: Refactoring Spotbit Mar 8, 2022
@nochiel nochiel force-pushed the cleanup branch 2 times, most recently from ac75483 to 58b5df0 Compare March 28, 2022 19:14
Add `/exchanges` endpoint that returns exchange metadata. (Fixes BlockchainCommons#54)
Cleanup variable and function naming style (https://google.github.io/styleguide/pyguide.html#316-naming) to fix BlockchainCommons#51.
Freeze dependencies.
Cleanup imports.
Cleanup logging.
Improve error handling.
Refactor configuration settings.
Load settings using dotenv.
Use `flask run` to run the server.
Begin refactor to simplify data collection.
Use concurrent requests to servers.
Remove unused functions.
Remove database storage.
@nochiel nochiel changed the title WIP: Refactoring Spotbit Refactoring Spotbit Apr 16, 2022
nochiel added 2 commits April 16, 2022 12:02
Use threads for HTTP requests.
@nochiel
Copy link
Contributor Author

nochiel commented Apr 16, 2022

@icculp Please could you review this PR?

@ChristopherA ChristopherA mentioned this pull request Apr 17, 2022
Use better rate limiting because Bitmex throttles aggressively.This
needs to be tested so that when using many threads we can successfully
get all the data the user wants.
Fix type error in get_candles.
Fix checks for pairs supported by exchanges.
@nochiel
Copy link
Contributor Author

nochiel commented May 3, 2022

This is closed by #58 which was based on this branch.

@nochiel nochiel closed this May 3, 2022
@k9ert
Copy link

k9ert commented Jun 14, 2022

What was the rationale for removing the database? Now, spotbit does not have any kind of caching-functionality, does it?

@nochiel
Copy link
Contributor Author

nochiel commented Jun 14, 2022

What was the rationale for removing the database? Now, spotbit does not have any kind of caching-functionality, does it?

  • The database was essentially just copying/downloading exchange databases. Given that price data is really only relevant in a situation where transacting/accounting is necessary, it's reasonable to assume that the user will configure Spotbit to use exchanges they prefer and that are available at any given time.
  • The caching was not adding a significant performance benefit given how fast most exchange APIs already are. (The main speed bottleneck when using Spotbit over Tor seems to be Tor itself, not exchanges.)
  • The database related code paths were additional unnecessary complexity. (Though, It is good that caching was attempted initially to see if it was worthwhile.)

@ChristopherA
Copy link
Contributor

I know that I'd like to see a database as an option, even if it isn't used for caching, as historical information may not always be available in the future. But this is a separate use case than the current price, and tying them together (both a cache and a historical database) in a single system caused problems. A new architecture is needed that separates them.

@ChristopherA
Copy link
Contributor

Is there a new issue on this topic? Is should be be linked here, as this issue is closed.

@k9ert
Copy link

k9ert commented Jul 1, 2022

I can't follow the rationale to be honest. Price data could be usefull in so much more scenarios than just transacting/accounting and having historical data around might also be very useful. E.g. for specter-desktop there is warden which we currently transform into a specter-extension. This one definitely likes to have historical price-data available.

Also one of the Summer-of-bitcoin-projects is to make spotbit a specter-plugin and as the DB is removed, the project effectively forked this project.

@nochiel
Copy link
Contributor Author

nochiel commented Jul 1, 2022

I can't follow the rationale to be honest. Price data could be usefull in so much more scenarios than just transacting/accounting and having historical data around might also be very useful. E.g. for specter-desktop there is warden which we currently transform into a specter-extension. This one definitely likes to have historical price-data available.

There are more efficient ways, than querying rate limited exchange APIs, of getting all historical price data, if one wants an offline record for a given period without requiring data to be sourced from specific exchanges. Eg. One could download a CSV for all of Bitcoin's recorded price history from:

  • blockchain.info
  • Coindesk
  • Yahoo Finance
  • Google BigQuery

That could of course be added as a feature to SpotBit but it's already trivial for an app to do that for itself, no?

@k9ert
Copy link

k9ert commented Jul 5, 2022

That could of course be added as a feature to SpotBit but it's already trivial for an app to do that for itself, no?

This is imho exactly the stuff which an app don't want to deal with. In your project description, you write:

It can either be used as a repository of historical data that allows for more frequent API requests, or as a simple wrapper around exchange APIs that premits the user to collect information over Tor.

This is imho the point: As an app i don't want to care how that data is obtained. I just want to use it and it's happy that some other app (spotbit) is taking care of the nitty-gritty details of that. Making it just available without caching it somehow doesn't add much value and it's also not very efficient for the operator, operating spotbit for a number of users (e.g. a raspiblitz owner being the uncle jim of his family OR a publicly available spotbit instance operated by someone who wants to give back to the community). This might even result in blacklisting the instance because it creates too many useless requests.

@nochiel
Copy link
Contributor Author

nochiel commented Jul 5, 2022

@k9ert I think I'm beginning to understand the feature and use case you have in mind. Please have a look at #66. Do feel free to add any feature requests that you feel could improve that issue or make it better suited to your needs?

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.

SpotBit should provide human-friendly names for exchanges.
3 participants