Skip to content

New apis#2

Open
Andrew-Pohl wants to merge 9 commits into
mainfrom
new-apis
Open

New apis#2
Andrew-Pohl wants to merge 9 commits into
mainfrom
new-apis

Conversation

@Andrew-Pohl

Copy link
Copy Markdown
Member

Pull Request: Add comprehensive Thorchain data modules and endpoints

Summary

This PR significantly expands the backend API surface by incorporating nearly the entire Thornode REST API suite. New modules, models, services, views and scheduled tasks have been added to capture, store and expose vault, pool, saver, LP and other network data. Additionally, proxy endpoints were created for quick fetches of transactions, queue, rune‑pool, mimir, etc.


What’s New

  1. Vaults

    • Model, service, views, and cron task (5‑min interval).
    • Endpoints: /vaults, /vaults/asgard, /vaults/yggdrasil, /vaults/vault/<pub_key>.
  2. Pools

    • Database-backed pool info with regular updates.
    • Endpoints: /pools, /pools/pool/<asset>.
  3. Savers

    • Track saver deposits/withdrawals per asset/address.
    • Endpoints: /savers/pool/<asset>/savers,
      /savers/pool/<asset>/saver/<address>.
  4. Liquidity Providers

    • Capture LP positions for each pool.
    • Endpoints: /lps/pool/<asset>/liquidity_providers,
      /lps/pool/<asset>/liquidity_provider/<address>.
  5. Transactions (proxy)

    • Direct fetch from Thornode.
    • Endpoints: /tx/<hash>, /tx/<hash>/signers.
  6. Queue (proxy)

    • Network queue status.
    • Endpoints: /queue, /queue/outbound.
  7. RUNE Pool (proxy)

    • RUNE pool and provider API.
    • Endpoints: /rune/runepool, /rune/rune_provider/<address>.
  8. Mimir (proxy)

    • Network constants.
    • Endpoints: /mimir, /mimir/key/<key>.
  9. Configuration & Scheduling

    • Added THORNODE_URL and updated config.py.
    • Scheduler updated to run vault/pool/saver/LP jobs.
    • Tables auto‑created at app start.
  10. General

    • Swagger docs auto‑updated for all new endpoints.
    • Existing historic node views remain unaffected.
    • Added two new historic-node report endpoints:
      • POST /historic/node/generateBPReport for bond-provider earnings
      • GET /historic/node/grabBPsForNode/<node> to list bond providers seen on a node across churns.

@Andrew-Pohl
Andrew-Pohl requested a review from dannydango March 10, 2026 12:59
@liquify-validation liquify-validation deleted a comment from claude Bot Mar 11, 2026
@liquify-validation liquify-validation deleted a comment from claude Bot Mar 11, 2026
@Andrew-Pohl

Copy link
Copy Markdown
Member Author

@claude review this PR

@claude

claude Bot commented Mar 11, 2026

Copy link
Copy Markdown

Claude finished @Andrew-Pohl's task in 2m 25s —— View job


PR Review: New APIs

Todo List:

  • Gather context and understand request
  • Read changed files and analyze code
  • Review for bugs, security issues, and best practices
  • Post comprehensive review feedback

Summary

This is a solid expansion of the backend API surface. The overall structure is clean and consistent across modules (model → task → view). Below are the findings, grouped by severity.


🔴 Bugs / Correctness Issues

1. Falsy check incorrectly rejects start=0 or end=0

API/historic_nodes/views.py:63 and :201:

if not node or not start or not end:  # line 63
if not node or not start or not end or not bp:  # line 201

If start or end is 0 (an integer), Python treats it as falsy and returns a 400. The correct check should be:

if node is None or start is None or end is None:

In practice churn heights won't be 0, but this is still a latent bug.
Fix this →

2. _sync_mimir double-parses JSON

API/mimir/task.py:26:

print(f'[mimir] Synced {len(response.json())} parameters')

response.json() is called a second time after already parsing and storing the data. This makes an extra HTTP deserialization pass. Cache the result:

data = response.json()
row.data = json.dumps(data)
...
print(f'[mimir] Synced {len(data)} parameters')

Fix this →

3. Wrong HTTP status for "no data found"

API/historic_nodes/views.py:77, 89, 111, 215, 227: Returns 500 for scenarios that are clearly "not found":

return jsonify({'message': f'No data between churns {start}-{end}'}), 500

A missing record is a 404, not a server error. Returning 500 makes client error handling harder and confuses monitoring/alerting.
Fix this →

4. Misleading 503 when cache is populated but filter matches nothing

API/vaults/views.py:79-80 — the /vaults/pending endpoint returns 503 "Cache not yet populated" when there are simply no non-active vaults. This is semantically wrong: the cache is populated, there just happen to be no retiring vaults. Same issue in API/pools/views.py:49-50 when a ?status= filter matches nothing.
Fix this →


🟡 Security / Reliability Issues

5. Unauthenticated /sync endpoints

All four modules expose POST /sync endpoints with no authentication:

  • API/vaults/views.py:11
  • API/pools/views.py:11
  • API/mimir/views.py:11
  • API/queue/views.py:11

Any external caller can trigger repeated data refreshes, creating unnecessary load on THORNode/Midgard. These should require authentication or at minimum be rate-limited.

6. Hardcoded THORNODE_URL in nodes/task.py

API/nodes/task.py:21 and :132 hardcode https://thornode.ninerealms.com/... directly, bypassing the config.Config.THORNODE_URL config key added in this PR:

response_API = requests.get('https://thornode.ninerealms.com/thorchain/nodes')  # line 21
response_API = requests.get('https://thornode.ninerealms.com/thorchain/nodes')  # line 132

Should use config.Config.THORNODE_URL for consistency and deployability.
Fix this →

7. MIDGARD_URL in config is hardcoded (env var commented out)

config/config.py:14:

MIDGARD_URL = 'https://midgard.ninerealms.com' #os.getenv('MIDGARD_URL')

THORNODE_URL on line 15 is similarly hardcoded. Both should read from environment variables to allow deployment against different environments (staging, local devnet, etc.).
Fix this →

8. Unhandled exceptions in sync view endpoints

API/vaults/views.py:21: _sync_vaults() is called without try/except. If the sync raises an unhandled exception (e.g., DB is down, network timeout), Flask will return a raw 500 traceback. The scheduled task wrappers handle this correctly — the same pattern should apply to the manual sync endpoints.


🟡 Code Quality Issues

9. Unused import: fetch_and_store_prices

API/__init__.py:21 imports fetch_and_store_prices but only fetch_and_store_price is ever scheduled. The bulk-backfill function is never called.
Fix this →

10. updated_at missing from VaultCache.to_dict()

API/vaults/models.py:17-26: The to_dict() method doesn't expose updated_at, so API consumers have no way to assess data freshness. By contrast, QueueCache.summary() correctly includes it. Consider adding it to vault responses.
Fix this →

11. Scheduler starts before blueprints are registered

API/__init__.py:57-64: The scheduler is started and jobs are added inside with app.app_context():, but blueprints are registered after the with block (lines 66-73). If a scheduled job fires during startup before blueprints are registered (unlikely but possible under load), it could fail. More importantly, the scheduler runs even before the app is fully wired. Consider moving scheduler.start() after return app.

12. PR description lists endpoints not in changed files

The PR description mentions /tx/<hash>, /tx/<hash>/signers, /rune/runepool, /rune/rune_provider/<address>, /savers/..., /lps/... — none of these appear in the changed files. This suggests either the PR description is aspirational/forward-looking, or some work was excluded from this branch. Worth clarifying so reviewers know what's actually merged.


🟢 Positive Notes

  • Consistent module structure across all new packages (model → task → view) makes the code easy to follow and extend.
  • The separation between _sync_*() (core logic) and sync_*_scheduled() (APScheduler entry-point with app context) is clean.
  • db.session.rollback() is correctly called in all scheduled task exception handlers.
  • The grabBPsForNode endpoint efficiently deduplicates bond providers across churns using a dict.
  • Use of <path:asset> in pools/views.py:72 correctly handles asset identifiers containing dots and slashes (e.g., ETH.USDC-0xA0...).
  • The historicPerformers N+1 query fix (batch query via .in_()) is a good improvement over the previous approach.

|

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