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.

Sign up for GitHub

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

Jump to bottom

Add signers ticker automatic import process + /signers/tickers route to aggregator http server #1276

Merged
merged 19 commits into from
Oct 4, 2023
Merged

Add signers ticker automatic import process + /signers/tickers route to aggregator http server #1276

merged 19 commits into from
Oct 4, 2023

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 4, 2023 •
edited
Loading

Content

This PR add to the aggregator:

  • The SignerImporter services and traits (to define how it retrieves its data and how it persist them)
  • Run this service in an infinite loop within a thread when using the serve commands (if an source url is defined, else the thread won't be spawned)
  • Add a new route: /signers/tickers that expose a simplified view of the signer table.
  • Add a new field in the signer table: last_registered_at, since we will now import signers into that table from external source and not create them anymore only at registration.

This PR also add a TestHttpServer that allow test to define a http server, configured using warp routes as it's done in the aggregator.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

Issue(s)

Relates to #1185

@Alenar Alenar requested review from jpraynaud and dlachaume October 4, 2023 07:41
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited
Loading

Test Results

    3 files  ±  0    17 suites  ±0   6m 16s ⏱️ +11s
699 tests +18  699 ✔️ +18  0 💤 ±0  0 ❌ ±0 
757 runs  +18  757 ✔️ +18  0 💤 ±0  0 ❌ ±0 

Results for commit 5f01ab7. ± Comparison against base commit 4ba02f9.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview October 4, 2023 07:53 — with GitHub Actions Inactive
dlachaume
dlachaume reviewed Oct 4, 2023
View reviewed changes
openapi.yaml Outdated Show resolved Hide resolved
@Alenar Alenar temporarily deployed to testing-preview October 4, 2023 08:30 — with GitHub Actions Inactive
jpraynaud
jpraynaud reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

I noticed a few typos and a source of flakiness in the tests but overall looks good 👍

We are also missing the Cardano network in the route /signer/tickers

mithril-aggregator/src/tools/signer_tickers_importer.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/commands/serve_command.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/configuration.rs Show resolved Hide resolved
mithril-aggregator/src/database/provider/signer.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/provider/signer.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/provider/signer.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/entities/signer_ticker_message.rs Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Show resolved Hide resolved
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from 183d97f to ba008ff Compare October 4, 2023 08:37
@Alenar Alenar temporarily deployed to testing-preview October 4, 2023 08:47 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch 3 times, most recently from 2d6162a to 899473f Compare October 4, 2023 09:37
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from 899473f to 57c4e85 Compare October 4, 2023 09:40
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from 57c4e85 to 50c9607 Compare October 4, 2023 11:35
Alenar and others added 11 commits October 4, 2023 13:38
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from 50c9607 to 057e78b Compare October 4, 2023 11:38
@Alenar Alenar marked this pull request as ready for review October 4, 2023 11:40
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from e63320a to cce50ea Compare October 4, 2023 11:45
@Alenar Alenar requested a review from jpraynaud October 4, 2023 11:46
@Alenar Alenar temporarily deployed to testing-preview October 4, 2023 11:55 — with GitHub Actions Inactive
jpraynaud
jpraynaud approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

openapi.yaml Show resolved Hide resolved
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from cce50ea to f8f88f4 Compare October 4, 2023 12:40
Alenar added 5 commits October 4, 2023 14:44
@Alenar
Add /signers/tickers route to aggregator
c8260d9
@Alenar
Rename SignerTickerImporter to SignersImporter
bba21dc 
This is more cardano agnostic and say better what it realy does (it does
not just import the ticker but also create a signer in the db if it does
not exist).
Also make one of its test more robust by using a BtreeMap in a assert_eq
instead of a HashMap (that does not have fixed order).
@Alenar
Rename signer registered_at to last_registered_at
be456f8
@Alenar
Add cardano network to /signers/tickers response
887671a
@Alenar
Update versions
5f01ab7
@Alenar Alenar force-pushed the ensemble/1185/add-spo-tickers-in-explorer branch from f8f88f4 to 5f01ab7 Compare October 4, 2023 12:45
@Alenar Alenar temporarily deployed to testing-preview October 4, 2023 13:04 — with GitHub Actions Inactive
@Alenar Alenar merged commit 924a3d0 into main Oct 4, 2023
27 of 51 checks passed
@Alenar Alenar deleted the ensemble/1185/add-spo-tickers-in-explorer branch October 4, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlachaume dlachaume dlachaume approved these changes

@ghubertpalo ghubertpalo ghubertpalo approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Alenar @jpraynaud @ghubertpalo @dlachaume