Ensemble/895/mithril client snapshot commands #951

ghubertpalo · 2023-05-31T10:09:37Z

Content

This PR includes snapshot commands to the Mithril client.

Pre-submit checklist

Branch
- Tests are provided
- Crates versions are updated
- 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 documentation website
- Add dev blog post (yes but not in this ticket)

Comments

This PR also includes some refactoring in the common crate.

Issue(s)

Closes #895

mithril-client/src/services/snapshot.rs

mithril-client/src/aggregator_client/http_client.rs

mithril-client/src/commands/snapshot/list.rs

mithril-client/src/commands/snapshot/show.rs

mithril-client/src/aggregator_client/http_client.rs

mithril-client/src/commands/snapshot/list.rs

mithril-client/src/commands/snapshot/show.rs

mithril-client/src/services/snapshot.rs

github-actions · 2023-05-31T10:17:15Z

Test Results

    3 files ±  0   30 suites ±0 5m 28s ⏱️ -19s
574 tests - 22 574 ✔️ - 22 0 💤 ±0 0 ❌ ±0
606 runs - 66 606 ✔️ - 66 0 💤 ±0 0 ❌ ±0

Results for commit fbece38. ± Comparison against base commit b2124a3.

This pull request removes 33 and adds 11 tests. Note that renamed tests count towards both.

aggregator::tests ‑ discard_current_api_version_one_version
aggregator::tests ‑ discard_current_api_version_two_version
aggregator::tests ‑ discard_current_api_version_zero_version
aggregator::tests ‑ get_certificate_details_ko_404
aggregator::tests ‑ get_certificate_details_ko_500
aggregator::tests ‑ get_certificate_details_ko_unreachable
aggregator::tests ‑ get_certificate_details_ok
aggregator::tests ‑ get_download_snapshot_ko_500
aggregator::tests ‑ get_download_snapshot_ko_unreachable
aggregator::tests ‑ get_download_snapshot_ok
…

aggregator_client::certificate_client::tests ‑ test_show_ko
aggregator_client::certificate_client::tests ‑ test_show_ok_none
aggregator_client::certificate_client::tests ‑ test_show_ok_some
services::snapshot::tests ‑ test_download_snapshot_dir_already_exists
services::snapshot::tests ‑ test_download_snapshot_invalid_digest
services::snapshot::tests ‑ test_download_snapshot_ok
services::snapshot::tests ‑ test_list_snapshots
services::snapshot::tests ‑ test_list_snapshots_err
services::snapshot::tests ‑ test_show_snapshot
services::snapshot::tests ‑ test_show_snapshot_err
…

♻️ This comment has been updated with latest results.

mithril-client/src/services/snapshot.rs

jpraynaud

LGTM 👍

I left few comments below

jpraynaud · 2023-06-01T08:10:36Z

mithril-client/src/commands/snapshot/list.rs

+                Arc::new(CertificateClient::new(http_client)),
+                Arc::new(MithrilCertificateVerifier::new(slog_scope::logger())),
+                Arc::new(CardanoImmutableDigester::new(
+                    Path::new(""),


Maybe add a comment or to do here to explain with the path is empty?

jpraynaud · 2023-06-01T08:11:32Z

mithril-client/src/commands/snapshot/show.rs

+                Arc::new(CertificateClient::new(http_client)),
+                Arc::new(MithrilCertificateVerifier::new(slog_scope::logger())),
+                Arc::new(CardanoImmutableDigester::new(
+                    Path::new(""),


Same comment as above

jpraynaud · 2023-06-01T08:13:15Z

mithril-client/src/commands/snapshot/show.rs

+
+/// Convert Snapshot to SnapshotFieldItems routine
+fn convert_to_field_items(snapshot: &Snapshot) -> Vec<SnapshotFieldItem> {
+    let mut field_items = vec![


We could add a SnapshotFieldItem with the Cardano network and use the snapshot.beacon.network to retrieve the value

jpraynaud · 2023-06-01T08:16:09Z

mithril-client/src/entities.rs

@@ -21,10 +21,6 @@ pub struct Config {
 /// for the purpose of tabular display
 #[derive(Table, Debug, Clone, PartialEq, Eq, PartialOrd, Serialize)]
 pub struct SnapshotListItem {
-    /// Cardano network


We could have kept this information of the Cardano network and use the snapshot.beacon.network to retrieve the value

mithril-client/src/main.rs

jpraynaud · 2023-06-01T08:21:20Z

mithril-client/src/main.rs

 /// CLI command list
 #[derive(Subcommand, Debug, Clone)]
-enum Commands {
+enum SnapshotCommands {


This could be declared in the snapshot command sub-module? WDYT?

jpraynaud · 2023-06-01T08:23:41Z

mithril-client/src/main.rs

    /// List available snapshots
    #[clap(arg_required_else_help = false)]
-    List(ListCommand),
+    List(SnapshotListCommand),

    /// Show detailed informations about a snapshot
    #[clap(arg_required_else_help = false)]


Do we need this instead?

Suggested change

#[clap(arg_required_else_help = false)]

#[clap(arg_required_else_help = true)]

jpraynaud · 2023-06-01T08:24:35Z

mithril-client/src/runtime.rs

Do we still need this runtime module?

jpraynaud · 2023-06-01T08:26:36Z

mithril-common/src/crypto_helper/codec.rs

-    let from_vec = Vec::from_hex(from).map_err(|e| format!("can't parse from hex: {e}"))?;
-    serde_json::from_slice(from_vec.as_slice()).map_err(|e| format!("can't deserialize: {e}"))
+    let from_vec = Vec::from_hex(from).map_err(|e| {
+        format!("Key decode hex: can not turn hexadecimal '{from}' into bytes, error: {e}")


The from could be very lengthy and hard to read error message. Maybe using the std::any::type_name::<T>() as below could give a good hint of where the problem occurs?

jpraynaud

We miss an update of this doc as well: https://mithril.network/doc/manual/getting-started/run-mithril-devnet

mithril-client/src/aggregator_client/certificate_client.rs

mithril-client/src/commands/snapshot/show.rs

github-advanced-security bot found potential problems May 31, 2023

View reviewed changes

mithril-client/src/services/snapshot.rs Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 31, 2023

View reviewed changes

mithril-client/src/services/snapshot.rs Fixed Show fixed Hide fixed

mithril-client/src/services/snapshot.rs Fixed Show fixed Hide fixed

ghubertpalo requested review from Alenar and jpraynaud June 1, 2023 08:01

jpraynaud approved these changes Jun 1, 2023

View reviewed changes

jpraynaud reviewed Jun 1, 2023

View reviewed changes

Alenar reviewed Jun 1, 2023

View reviewed changes

mithril-client/src/aggregator_client/certificate_client.rs Show resolved Hide resolved

ghubertpalo added 8 commits June 1, 2023 12:18

add snapshot service and client

0f78b62

add artifact command and certificate client

d289914

add download command

aa69269

fix digester path

8bc28ca

remove unpack files when check fails

69f8d89

add prerequisite checks

f3cf506

update documentation

d5a1023

add download meter

a1be3cb

ghubertpalo force-pushed the ensemble/895/mithril_client_snapshot_commands branch from 2428f22 to 671c082 Compare June 1, 2023 10:22

github-advanced-security bot found potential problems Jun 1, 2023

View reviewed changes

mithril-client/src/commands/snapshot/show.rs Fixed Show fixed Hide fixed

mithril-client/src/commands/snapshot/show.rs Fixed Show fixed Hide fixed

ghubertpalo force-pushed the ensemble/895/mithril_client_snapshot_commands branch 2 times, most recently from 14060f8 to 1cb7bdb Compare June 1, 2023 12:32

Alenar approved these changes Jun 1, 2023

View reviewed changes

ghubertpalo temporarily deployed to testing-preview June 1, 2023 12:39 — with GitHub Actions Inactive

update versions

fbece38

ghubertpalo force-pushed the ensemble/895/mithril_client_snapshot_commands branch from 1cb7bdb to fbece38 Compare June 1, 2023 13:00

ghubertpalo temporarily deployed to testing-preview June 1, 2023 13:07 — with GitHub Actions Inactive

ghubertpalo merged commit 27d5783 into main Jun 1, 2023
24 checks passed

ghubertpalo deleted the ensemble/895/mithril_client_snapshot_commands branch June 1, 2023 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensemble/895/mithril client snapshot commands #951

Ensemble/895/mithril client snapshot commands #951

ghubertpalo commented May 31, 2023 •

edited by jpraynaud

github-actions bot commented May 31, 2023 •

edited

jpraynaud left a comment

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023

jpraynaud Jun 1, 2023 •

edited

jpraynaud Jun 1, 2023

jpraynaud left a comment

	#[clap(arg_required_else_help = false)]
	#[clap(arg_required_else_help = true)]

Ensemble/895/mithril client snapshot commands #951

Ensemble/895/mithril client snapshot commands #951

Conversation

ghubertpalo commented May 31, 2023 • edited by jpraynaud

Content

Pre-submit checklist

Comments

Issue(s)

github-actions bot commented May 31, 2023 • edited

Test Results

jpraynaud left a comment

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023 • edited

Choose a reason for hiding this comment

jpraynaud Jun 1, 2023

Choose a reason for hiding this comment

jpraynaud left a comment

Choose a reason for hiding this comment

ghubertpalo commented May 31, 2023 •

edited by jpraynaud

github-actions bot commented May 31, 2023 •

edited

jpraynaud Jun 1, 2023 •

edited