search

CounterpartyXCP / counterparty-core

Counterparty Protocol Reference Implementation
http://counterparty.io
MIT License
287 stars 206 forks source link

API Redesign #1327

Closed adamkrellenstein closed 6 months ago

adamkrellenstein commented 10 months ago

So it turns out that I designed the API very badly 10 years ago. It's insanely leaky and is missing critical functionality. Ideas:

  1. Deprecate everything that exposes the DB schema.
    1. Use X-API-Warn
    2. Throw warnings when they're used
    3. Put behind a CLI flag
    4. Put behind /api/v1/ (and new API goes behind /api/ and /api/v2/)
  2. Add normal ReST endpoints for common operations — look to xchain.io for inspiration (!)
  3. Implement unpack for all message types. The API only provides this for send: https://github.com/CounterpartyXCP/counterparty-lib/blob/e65172a6700a6a45207120ca082da2673d8b240c/counterpartylib/lib/api.py#L888
  4. Blueprint or Swagger documentation generation + testing (e.g. with https://github.com/apiaryio/dredd)
  5. Load testing
jotapea commented 10 months ago

Maybe not including GETs is a way of delegating the scaling to the application layer? The SQLite DB optimized (and focused) on validation, instead of queries, I believe makes sense.

Unpack is unfinished, and quite useful. And will clean the messages code.

Then, the mirror functionality could be a “compose” (maybe best to be a create flag) in which DB validation is skipped.

Leaving the default with validation.

adamkrellenstein commented 7 months ago

Let's be sure not to log exceptions for simple bad requests like we do now. (These should all be single-line warnings.)

e 606, in compose_transaction
16|counterparty  |     return transaction.construct(
16|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/transaction.py", line 119, in construct
16|counterparty  |     return TRANSACTION_SERVICE_SINGLETON.construct(
16|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/transaction.py", line 577, in construct
16|counterparty  |     raise exceptions.TransactionError("Destination output is dust.")
16|counterparty  | counterpartylib.lib.exceptions.TransactionError: Destination output is dust.
16|counterparty  | [2024-04-14 18:04:37][INFO] Mempool parsed txs count:0 from 60978
16|counterparty  | [2024-04-14 18:04:37][INFO] Mempool parsing interrupted, there are blocks to parse
16|counterparty  | [2024-04-14 18:04:38][INFO] New block inserted 839221
16|counterparty  | [2024-04-14 18:04:43][INFO] New transaction inserted 46aebc99c602bdf5fcef78be0162d16d801da9cd7198149a30b583421a2a2c95
16|counterparty  | [2024-04-14 18:04:43][INFO] New transaction inserted d028ca5778f5aa1bf477ca4afa21067504beb12a4c02c073eb5f7b01cfeb7523
16|counterparty  | [2024-04-14 18:04:43][INFO] New transaction inserted b1aafde4c783c8d79b95ec6c01edaa1b67325883c9e4d8179a5c4add63e3625e
16|counterparty  | [2024-04-14 18:04:45][INFO] Updated order 4b09178264985c999a9b38c68f12ad7081cbc0ecf4308133d5cf4274677e2df7. New status: expired
16|counterparty  | [2024-04-14 18:04:45][INFO] Credit 1 JACKPOTPEPE to 17JqqKacU6ibZheUyDQaW6K49otYx7Aysq
16|counterparty  | [2024-04-14 18:04:45][INFO] Order 4b09178264985c999a9b3
17|counterparty  | WARNING:root:Traceback (most recent call last):
17|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/api.py", line 780, in create_method
17|counterparty  |     return compose_transaction(
17|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/api.py", line 606, in compose_transaction
17|counterparty  |     return transaction.construct(
17|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/transaction.py", line 119, in construct
17|counterparty  |     return TRANSACTION_SERVICE_SINGLETON.construct(
17|counterparty  |   File "/home/adam/counterparty-core/counterparty-lib/counterpartylib/lib/transaction.py", line 577, in construct
17|counterparty  |     raise exceptions.TransactionError("Destination output is dust.")
17|counterparty  | counterpartylib.lib.exceptions.TransactionError: Destination output is dust.
17|counterparty  | WARNING:root:Error composing send transaction via API: Insufficient BTC at address 1JjALGQhbVGx2ig6cRFu56SM7KT4xkX39q. (Need approximately 0.0144608 BTC.)
jdogresorg commented 7 months ago

I am guessing your still in the process of defining the v2 API endpoints.... as what I am seeing defined right now lacks APIs to get data from various parts of the system (broadcasts, dispensers, bets, burns, holders, etc)

I will respectfully wait for the v2 API endpoints to be fully defined by the current core devs/team before I can speak further on v1 vs v2 APIs... but I do feel it is important to share the following thoughts:

I am supportive of moving things forward and adding new features and would like to get on the new versions when they are proven stable (takes many weeks).

Developers need to be given ample time to test new v2 API endpoints and update their infastructure / code / tools... node operators can't be forced to update, they need to believe in the new version and want to update to it.

I look forward to seeing the v2 API endpoints and being able to test them.

TLDR... Please leave get_messages endpoint operational.

adamkrellenstein commented 7 months ago

Yes, APIv2 is still in development. There will definitely be endpoints for all of the standard messages, bets, broadcasts etc.

Unfortunately, because APIv1 gives users the ability effectively to make SQL queries directly against the DB, v2 will not have all of the functionality of v1. (Part of the problem with that v1 is that it is defined only implicitly.) However, v2 can be expanded over time to include all of the important stuff! Block explorers like yours may want to read from the messages table. We're going to preserve get_messages, but I think it'll just be a GET request on /messages with similar functionality (but with somewhat different query params, I assume).

All v1 calls (including get_*) will be immediately deprecated, but they will be available behind a flag and with a new version prefix (/api/v1/get_*) for a while to allow developers the chance to migrate to the new version before they are actually removed.

We'll get an beta version up and running ASAP so that you and everyone else can note all functionality that it's missing but that's important, and we're going to be liberal about adding new endpoints / features. Keeping downstream dev happy is absolutely a top priority.

ouziel-slama commented 6 months ago

So it turns out that I designed the API very badly 10 years ago. It's insanely leaky and is missing critical functionality. Ideas:

  1. Deprecate everything that exposes the DB schema.

    1. Use X-API-Warn
    2. Throw warnings when they're used
    3. Put behind a CLI flag
    4. Put behind /api/v1/ (and new API goes behind /api/ and /api/v2/)
  2. Add normal ReST endpoints for common operations — look to xchain.io for inspiration (!)
  3. Implement unpack for all message types. The API only provides this for send: https://github.com/CounterpartyXCP/counterparty-lib/blob/e65172a6700a6a45207120ca082da2673d8b240c/counterpartylib/lib/api.py#L888
  4. Blueprint or Swagger documentation generation + testing (e.g. with https://github.com/apiaryio/dredd)
  5. Load testing

Everything is done except 4. and 5. See https://github.com/CounterpartyXCP/counterparty-core/issues/1739 and more generally https://github.com/CounterpartyXCP/counterparty-core/labels/api