HTTP message service handles signed entities #1366

ghubertpalo · 2023-11-22T15:40:15Z

Content

This PR includes a patch that use the HTTP message service to deliver directly HTTP messages for signed entities to the web server. By doing so, it avoids expensive key decode / encode.

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

None

Issue(s)

Relates to #1327

github-actions · 2023-11-22T15:45:33Z

Test Results

    3 files ±0   30 suites ±0 7m 33s ⏱️ -10s
745 tests +8 745 ✔️ +8 0 💤 ±0 0 ❌ ±0
853 runs +8 853 ✔️ +8 0 💤 ±0 0 ❌ ±0

Results for commit adc59bd. ± Comparison against base commit b36415b.

This pull request removes 5 and adds 13 tests. Note that renamed tests count towards both.

mithril-aggregator ‑ message_adapters::to_certificate_list_message::tests::adapt_ok
mithril-aggregator ‑ message_adapters::to_certificate_message::tests::adapt_ok
mithril-aggregator ‑ services::http_message::tests::get_certificate
mithril-aggregator ‑ services::http_message::tests::get_last_certificates
mithril-aggregator ‑ services::http_message::tests::get_no_certificate

mithril-aggregator ‑ services::message::tests::get_certificate
mithril-aggregator ‑ services::message::tests::get_last_certificates
mithril-aggregator ‑ services::message::tests::get_mithril_no_stake_distribution
mithril-aggregator ‑ services::message::tests::get_mithril_stake_distribution
mithril-aggregator ‑ services::message::tests::get_mithril_stake_distribution_list_message
mithril-aggregator ‑ services::message::tests::get_no_certificate
mithril-aggregator ‑ services::message::tests::get_no_snapshot
mithril-aggregator ‑ services::message::tests::get_snapshot
mithril-aggregator ‑ services::message::tests::get_snapshot_list_message
mithril-signer ‑ aggregator_client::tests::test_epoch_settings_timeout
…

♻️ This comment has been updated with latest results.

mithril-aggregator/src/database/provider/signed_entity.rs

mithril-aggregator/src/services/http_message.rs

jpraynaud

Do we still need to keep the previous message adapters that are not used anymore?

mithril-aggregator/src/database/provider/signed_entity.rs

mithril-aggregator/src/services/http_message.rs

jpraynaud · 2023-11-29T11:16:00Z

mithril-aggregator/src/services/http_message.rs

+    async fn get_snapshot() {
+        let beacon = fake_data::beacon();
+        let entity = SignedEntity {
+            signed_entity_id: "msd1".to_string(),


It's a bit confusing to use Mithril stake distribution code in snapshot test.

Sorry, I don't see the issue here.

Suggested change

signed_entity_id: "msd1".to_string(),

signed_entity_id: "snapshot1".to_string(),

jpraynaud · 2023-11-29T11:19:45Z

mithril-aggregator/src/services/http_message.rs

+            .await?;
+
+        entities.into_iter().map(|i| i.try_into()).collect()
+    }
 }

 #[cfg(test)]


Given all the boilerplate in these tests, it would have been a good idea to create dummy functions for SignedEntity<T> types IMHO.

dlachaume

I have no comments other than those raised by @jpraynaud. Otherwise, LGTM

ghubertpalo · 2023-11-29T14:29:17Z

Do we still need to keep the previous message adapters that are not used anymore?

They are still used mainly in tests in MessageService but not only.

mithril-aggregator/src/services/message.rs

jpraynaud

LGTM 👍

ghubertpalo temporarily deployed to testing-preview November 22, 2023 15:46 — with GitHub Actions Inactive

ghubertpalo temporarily deployed to testing-preview November 24, 2023 09:40 — with GitHub Actions Inactive

dlachaume reviewed Nov 24, 2023

View reviewed changes

mithril-aggregator/src/database/provider/signed_entity.rs Outdated Show resolved Hide resolved

dlachaume reviewed Nov 24, 2023

View reviewed changes

mithril-aggregator/src/database/provider/signed_entity.rs Show resolved Hide resolved

dlachaume reviewed Nov 24, 2023

View reviewed changes

mithril-aggregator/src/services/http_message.rs Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 27, 2023

View reviewed changes

mithril-aggregator/src/services/http_message.rs Fixed Show fixed Hide fixed

mithril-aggregator/src/services/http_message.rs Fixed Show fixed Hide fixed

ghubertpalo force-pushed the greg/1327/signed_entity branch from 1d02d35 to 7a83852 Compare November 27, 2023 15:43

ghubertpalo temporarily deployed to testing-preview November 27, 2023 16:38 — with GitHub Actions Inactive

ghubertpalo temporarily deployed to testing-preview November 28, 2023 17:15 — with GitHub Actions Inactive

ghubertpalo requested review from dlachaume and jpraynaud November 29, 2023 08:08

ghubertpalo added 10 commits November 29, 2023 11:00

add snapshot signed entity

01362b4

http message service provides snapshot entity

08ab93d

http message service provides msd entity

c89250d

test msd ok

1d7361a

test msd ok norecord

2b77d58

test msd ko

eb0cc2d

test for msd message service

b2b6806

add msd list item message

952c938

add snapshot list message

a6074e5

update version

e804163

ghubertpalo force-pushed the greg/1327/signed_entity branch from 7bbe415 to e804163 Compare November 29, 2023 10:08

ghubertpalo temporarily deployed to testing-preview November 29, 2023 10:25 — with GitHub Actions Inactive

ghubertpalo self-assigned this Nov 29, 2023

revert lint in cargo.toml

6bc3cab

ghubertpalo temporarily deployed to testing-preview November 29, 2023 11:15 — with GitHub Actions Inactive

jpraynaud reviewed Nov 29, 2023

View reviewed changes

dlachaume reviewed Nov 29, 2023

View reviewed changes

ghubertpalo added 2 commits November 29, 2023 15:31

remove unused message adapters

becd150

remove snapshot tmp structure

8f0303a

ghubertpalo temporarily deployed to testing-preview November 29, 2023 14:48 — with GitHub Actions Inactive

renamed message service

89d6d22

ghubertpalo temporarily deployed to testing-preview November 29, 2023 15:22 — with GitHub Actions Inactive

rename service methods

94b4e5b

github-advanced-security bot found potential problems Nov 29, 2023

View reviewed changes

mithril-aggregator/src/services/message.rs Fixed Show fixed Hide fixed

mithril-aggregator/src/services/message.rs Fixed Show fixed Hide fixed

ghubertpalo requested a review from jpraynaud November 30, 2023 07:27

fix documentation

adc59bd

jpraynaud approved these changes Nov 30, 2023

View reviewed changes

ghubertpalo temporarily deployed to testing-preview November 30, 2023 09:38 — with GitHub Actions Inactive

ghubertpalo merged commit 0e90d2c into main Nov 30, 2023
30 of 31 checks passed

ghubertpalo deleted the greg/1327/signed_entity branch November 30, 2023 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP message service handles signed entities #1366

HTTP message service handles signed entities #1366

ghubertpalo commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

jpraynaud left a comment

jpraynaud Nov 29, 2023

ghubertpalo Nov 29, 2023

dlachaume Nov 29, 2023

jpraynaud Nov 29, 2023

dlachaume left a comment

ghubertpalo commented Nov 29, 2023

jpraynaud left a comment

	signed_entity_id: "msd1".to_string(),
	signed_entity_id: "snapshot1".to_string(),

HTTP message service handles signed entities #1366

HTTP message service handles signed entities #1366

Conversation

ghubertpalo commented Nov 22, 2023 • edited Loading

Content

Pre-submit checklist

Comments

Issue(s)

github-actions bot commented Nov 22, 2023 • edited Loading

Test Results

jpraynaud left a comment

Choose a reason for hiding this comment

jpraynaud Nov 29, 2023

Choose a reason for hiding this comment

ghubertpalo Nov 29, 2023

Choose a reason for hiding this comment

dlachaume Nov 29, 2023

Choose a reason for hiding this comment

jpraynaud Nov 29, 2023

Choose a reason for hiding this comment

dlachaume left a comment

Choose a reason for hiding this comment

ghubertpalo commented Nov 29, 2023

jpraynaud left a comment

Choose a reason for hiding this comment

ghubertpalo commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading