Skip to content

refactor: unify and extend CardanoNetwork implementation #2643

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.

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

Merged
merged 10 commits into from
Jul 17, 2025

Conversation

dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Jul 16, 2025

Content

This PR includes a refactoring and extends the CardanoNetwork enum in mithril-common to allow usage from clients, particularly for the mithril-client CLI.

Main changes include:

  • Make CardanoNetwork publicly accessible.
  • Add a From<MagicId> implementation to support direct conversion from network magic IDs.
  • Merge the DevNet variant into the TestNet variant.
  • Update the tools utxo-hd snapshot-converter command in the mithril-client CLI to use CardanoNetwork and remove duplicated logic.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • All check jobs of the CI have succeeded
    • 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)
    • Add ADR blog post or Dev ADR entry (if relevant)
    • No new TODOs introduced

Issue(s)

Closes #2590

@dlachaume dlachaume self-assigned this Jul 16, 2025
Copy link

github-actions bot commented Jul 16, 2025

Test Results

    4 files  ±0    154 suites  ±0   23m 41s ⏱️ -54s
2 118 tests ±0  2 118 ✅ ±0  0 💤 ±0  0 ❌ ±0 
6 466 runs  ±0  6 466 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2d90b53. ± Comparison against base commit f19a476.

This pull request removes 3 and adds 3 tests. Note that renamed tests count towards both.
mithril-cardano-node-chain ‑ chain_observer::cli_observer::tests::test_cli_devnet_runner
mithril-client ‑ utils::bootstrap_files::test::create_bootstrap_node_files_creates_protocol_magic_id_and_clean_files_for_testnet
mithril-client ‑ utils::bootstrap_files::test::create_bootstrap_node_files_does_not_create_protocol_magic_id_file_and_create_clean_file_for_devnet
mithril-client ‑ utils::bootstrap_files::test::create_bootstrap_node_files_creates_protocol_magic_id_file_and_create_clean_file_for_devnet
mithril-client ‑ utils::bootstrap_files::test::create_bootstrap_node_files_does_not_create_protocol_magic_id_file_and_create_clean_file_for_private_network
mithril-common ‑ entities::cardano_network::tests::cardano_network_from_magic_id_roundtrip

♻️ This comment has been updated with latest results.

@dlachaume dlachaume requested a review from Copilot July 16, 2025 17:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CardanoNetwork enum by merging the former DevNet variant into TestNet, making the enum and its magic ID constants public, and introducing a From<MagicId> conversion. It also updates all client code, scripts, and tests to use the new magic_id() method instead of the old code() API.

  • Unified DevNet into TestNet and exposed CardanoNetwork and its magic ID constants publicly.
  • Added From<MagicId> for direct network construction and renamed code() to magic_id().
  • Updated CLI tools, end-to-end tests, scripts, and internal clients to use the new API and removed duplicated network logic.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
mithril-common/src/entities/cardano_network.rs Refactored enum variants, made magic ID constants public, added From
mithril-common/src/entities/mod.rs Exposed CardanoNetwork and magic ID constants in the public API
mithril-client/src/utils/bootstrap_files.rs Switched to magic_id() and adjusted bootstrap file creation tests
mithril-client-cli/src/commands/tools/snapshot_converter.rs Introduced CardanoNetworkCliArg and updated detection logic
internal/cardano-node//pallas_.rs Replaced network.code() calls with network.magic_id()
Comments suppressed due to low confidence (4)

mithril-common/src/entities/cardano_network.rs:39

  • The from_code function no longer recognizes the standard "testnet" network code, which will break parsing of the main public testnet magic ID. Consider re-adding the mapping "testnet" => Ok(CardanoNetwork::TestNet(TESTNET_MAGIC_ID)).
        match network_code.to_lowercase().as_str() {

mithril-client/src/utils/bootstrap_files.rs:105

  • [nitpick] The test name states that the protocol magic ID file should not be created, yet the test actually asserts its creation. Consider renaming the test to accurately reflect that the file is expected to exist.
    fn create_bootstrap_node_files_does_not_create_protocol_magic_id_file_and_create_clean_file_for_private_network()

internal/cardano-node/mithril-cardano-node-chain/src/chain_observer/cli_observer.rs:170

  • There is no test verifying that a TestNet with the devnet magic ID (now collapsed into TestNet) yields the correct CLI flags. Add a test for CardanoNetwork::TestNet(DEVNET_MAGIC_ID) to ensure the devnet scenario is covered.
            CardanoNetwork::TestNet(magic) => {

mithril-common/src/entities/cardano_network.rs:83

  • The Display implementation no longer prints "testnet" for the former TESTNET_MAGIC_ID, causing that case to fall through to "private". Add a case for the original testnet magic ID to correctly format it as "testnet".
            CardanoNetwork::TestNet(magic_id) => match magic_id {

@dlachaume dlachaume marked this pull request as ready for review July 16, 2025 17:02
@dlachaume dlachaume temporarily deployed to testing-preview July 16, 2025 17:03 — with GitHub Actions Inactive
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 👍

@dlachaume dlachaume temporarily deployed to testing-preview July 17, 2025 08:34 — with GitHub Actions Inactive
…RSION` version

* mithril-cardano-node-chain from `0.1.3` to `0.1.4`
* mithril-dmq from `0.1.3` to `0.1.4`
* mithril-aggregator from `0.7.73` to `0.7.74`
* mithril-client-cli from `0.12.21` to `0.12.22`
* mithril-client from `0.12.21` to `0.12.22`
* mithril-common from `0.6.8` to `0.6.9`
* mithril-signer from `0.2.259` to `0.2.260`
* mithril-end-to-end from `0.4.96` to `0.4.97`
* mithril-test-lab/mithril-devnet/VERSION from `0.4.13` to `0.4.14`
@dlachaume dlachaume temporarily deployed to testing-preview July 17, 2025 09:54 — with GitHub Actions Inactive
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

Copy link
Collaborator

@turmelclem turmelclem left a comment

Choose a reason for hiding this comment

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

LGTM ✨

@dlachaume dlachaume merged commit d8ac86c into main Jul 17, 2025
56 of 59 checks passed
@dlachaume dlachaume deleted the dlachaume/2590/refactor-cardano-network-entity branch July 17, 2025 13:49
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.

Refactor CardanoNetwork entity
4 participants