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.

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

Design mithril-common split & re-organization in repository #2175

Open
3 tasks done
jpraynaud opened this issue Dec 16, 2024 · 3 comments
Open
3 tasks done

Design mithril-common split & re-organization in repository #2175

jpraynaud opened this issue Dec 16, 2024 · 3 comments
Assignees
Labels
refactoring 🛠️ Code refactoring and enhancements

Comments

@jpraynaud
Copy link
Member

jpraynaud commented Dec 16, 2024

Why

In order to reduce build times, binary sizes and number of features, and also to provide more structured libraries, we want to split the mithril-common crate in sub-crates.

What

Propose a split for the mithril-common crate and a plan of deployment.

How

  • Create a new mapping for mithril-common into sub-crates (mention responsibility of the new sub-crate, propose names and locations in the repository -internal, root, ...-)
  • Identify the new sub-crates that will be need to be published on crates.io
  • Prepare a roll-out plan which details the order of migrations and sub-crates that need to be migrated together
@jpraynaud jpraynaud added the refactoring 🛠️ Code refactoring and enhancements label Dec 16, 2024
@Alenar
Copy link
Collaborator

Alenar commented Dec 20, 2024

Current state overview

Tip

The graphs have been generated using cargo module.

Note

For all generated graph, their code have been edited to remove a mithril-common node that was connected to every items, polluting the resulting graph (this dependency exist because of the StdResult alias that's broadly used).

Top modules inter-dependencies

mithril-common-depth1

Graph code

Command

Note: Since the cfg_xxx features gate macros where hindering cargo module analisys capabilities, the cfg_fs! macro arround the digesters, block scanner, etc ... was temporary removed in the lib.rs before running the command

cargo modules dependencies -p mithril-common --lib --features all --no-fns --no-traits --no-owns --no-types --no-externs --max-depth 1

Generated graphviz

digraph {

    graph [
        label="mithril_common",
        labelloc=t,

        pad=0.4,

        // Consider rendering the graph using a different layout algorithm, such as:
        // [dot, neato, twopi, circo, fdp, sfdp]
        layout=neato,
        overlap=false,
        splines="line",
        rankdir=LR,

        fontname="Helvetica", 
        fontsize="36",
    ];

    node [
        fontname="monospace",
        fontsize="10",
        shape="record",
        style="filled",
    ];

    edge [
        fontname="monospace",
        fontsize="10",
    ];

    "mithril_common::api_version" [label="pub mod|api_version", fillcolor="#81c169"]; // "mod" node
    "mithril_common::cardano_block_scanner" [label="pub mod|cardano_block_scanner", fillcolor="#81c169"]; // "mod" node
    "mithril_common::cardano_transactions_preloader" [label="pub mod|cardano_transactions_preloader", fillcolor="#81c169"]; // "mod" node
    "mithril_common::certificate_chain" [label="pub mod|certificate_chain", fillcolor="#81c169"]; // "mod" node
    "mithril_common::chain_observer" [label="pub mod|chain_observer", fillcolor="#81c169"]; // "mod" node
    "mithril_common::chain_reader" [label="pub mod|chain_reader", fillcolor="#81c169"]; // "mod" node
    "mithril_common::crypto_helper" [label="pub mod|crypto_helper", fillcolor="#81c169"]; // "mod" node
    "mithril_common::digesters" [label="pub mod|digesters", fillcolor="#81c169"]; // "mod" node
    "mithril_common::entities" [label="pub mod|entities", fillcolor="#81c169"]; // "mod" node
    "mithril_common::era" [label="pub mod|era", fillcolor="#81c169"]; // "mod" node
    "mithril_common::logging" [label="pub mod|logging", fillcolor="#81c169"]; // "mod" node
    "mithril_common::messages" [label="pub mod|messages", fillcolor="#81c169"]; // "mod" node
    "mithril_common::protocol" [label="pub mod|protocol", fillcolor="#81c169"]; // "mod" node
    "mithril_common::resource_pool" [label="pub mod|resource_pool", fillcolor="#81c169"]; // "mod" node
    "mithril_common::signable_builder" [label="pub mod|signable_builder", fillcolor="#81c169"]; // "mod" node
    "mithril_common::signed_entity_type_lock" [label="pub mod|signed_entity_type_lock", fillcolor="#81c169"]; // "mod" node
    "mithril_common::ticker_service" [label="pub(crate) mod|ticker_service", fillcolor="#f8c04c"]; // "mod" node

    "mithril_common::api_version" -> "mithril_common::era" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_block_scanner" -> "mithril_common::chain_reader" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_block_scanner" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_block_scanner" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_transactions_preloader" -> "mithril_common::chain_observer" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_transactions_preloader" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_transactions_preloader" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::cardano_transactions_preloader" -> "mithril_common::signed_entity_type_lock" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::certificate_chain" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::certificate_chain" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::certificate_chain" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::certificate_chain" -> "mithril_common::protocol" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::chain_observer" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::chain_observer" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::chain_reader" -> "mithril_common::cardano_block_scanner" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::chain_reader" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::chain_reader" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::crypto_helper" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::crypto_helper" -> "mithril_common::resource_pool" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::digesters" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::digesters" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::digesters" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::entities" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::entities" -> "mithril_common::messages" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::entities" -> "mithril_common::protocol" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::entities" -> "mithril_common::signable_builder" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::era" -> "mithril_common::chain_observer" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::era" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::era" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::messages" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::messages" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::messages" -> "mithril_common::era" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::protocol" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::protocol" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::signable_builder" -> "mithril_common::crypto_helper" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::signable_builder" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::signable_builder" -> "mithril_common::era" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::signable_builder" -> "mithril_common::logging" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::ticker_service" -> "mithril_common::chain_observer" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::ticker_service" -> "mithril_common::digesters" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge
    "mithril_common::ticker_service" -> "mithril_common::entities" [label="uses", color="#7f7f7f", style="dashed"] [constraint=false]; // "uses" edge

}

Modules analysis

signed_entity_type_lock

Dependency of: cardano_transactions_preloader

Depends on Reason Remark
/ / /

Nothing depends on it and it don't depends on anything either, it could be easily extracted.

certificate_chain

Dependency of: Nothing

Depends on Reason Remark
crypto_helper for the multiples keys it needs /
logging To get its logger name /
entities Mostly for the Certificate and protocol parameters /
protocol To convert an input to a message when creating a genesis certificate used in a test only method

Api version

Dependency of: Nothing

Depends on Reason Remark
era Need the current era to fetch the appropriate openapi file /

Logging

Dependency of: signable_builder, certificate_chain, cardano_block_scanner, chain_reader, digesters, cardano_transactions_preloader

Depends on Reason Remark
/ / /

Resource pool

Dependency of: crypto_helper

Depends on Reason Remark
/ / /

The crypto helper only need them to implement the reset trait to the MKMap type.

Era

Dependency of: api_version, messages, signable_builder

Depends on Reason Remark
entities needs the Epoch type for the era marker /
crypto_helper for era related cryptographic primitives and hex encode/decode /
chain_observer to read the marker in the chain /

This module consist of two parts: one part to check the current era and one to read them from different providers. The read part is the one that needs the dependency to crypto_helpers and chain_observer and could be partly closer to those dependencies.

Chain observer

Dependency of: era, cardano_transactions_preloader, ticker_service

Depends on Reason Remark
crypto_helper mostly for the KESPeriod and the OpCert /
entities For multiple types (epoch, stake distribution, blocknumbers, ...) /

Signable builder

Dependency of: entities

Depends on Reason Remark
crypto_helper for Merkle trees computation /
entities For beacon types and signed entities /
era Temporary to add new required values for Pythagoras in message /
logging To get its logger name /

Ticker service

Dependency of: Nothing

Depends on Reason Remark
entities for the Epoch and TimePoint /
chain_observer to fetch the current epoch and timepoint with the ChainObserver trait /
digester to fetch the current immutable with the ImmutableFileObserver trait /

This ImmutableFileObserver trait is only used here and could be moved directly to the ticker service to reverse the dependency. Maybe something similar could be done with the ChainObserver

Protocol

Dependency of: certificate_chain, entities

Depends on Reason Remark
entities For PartyId (a string alias), SingleSignatures, and SignerWithStake /
crypto_helper for signing related cryptographic types /

Cardano block scanner

Dependency of: chain_reader

Depends on Reason Remark
chain_reader for ChainBlockNextAction and ChainBlockReader /
entities for BlockNumber, ChainPoint, SlotNumber, TransactionHash, and CardanoTransaction /
logging To get its logger name /

Warning

The dependency to chain_reader is circular

Chain reader

Dependency of: cardano_block_scanner

Depends on Reason Remark
cardano_block_scanner for the RawCardanoPoint and ScannedBlock /
entities for CardanoNetwork /
logging to get its logger name /

Messages

Dependency of: entities

Depends on Reason Remark
entities for some simple types that we don't want to bother to duplicate (ie: beacons) /
era to include the SupportedEra in the AggregatorStatusMessage /
crypto_helper to convert signer messages to entities and to test CardanoTransactionsProofsMessage /

Crypto helper

Dependency of: certificate_chain, chain_observer, era, messages, protocol, signable_builder

Depends on Reason Remark
entities for conversions to crypto types, aliases for Hex decode/encode, and some tests / tests tools /
resource_pool to implement the Reset trait on MKMap /

Entities

Dependency of: certificate_chain, chain_observer, era, messages, protocol, signable_builder

Depends on Reason Remark
messages to impl a conversion to CardanoTransactionsSetProofMessagePart /
crypto_helper to embed multiple 'protocol keys', merkle trees types for cardano transactions, and for some tests /
protocol to implement the ToMessage trait to the ProtocolMessage /
signable_builder to implement the Artifiact and Beacons trait /

Warning

All of those dependencies are circular

The protocol and signable_builder dependencies could be reversed by implementing the trait in their module (if that make sense).

@Alenar
Copy link
Collaborator

Alenar commented Jan 8, 2025

Split proposals

Criteria

At minimum a split must:

  • Avoid any circular dependencies: dependencies must be in a "tree" structure.
  • Remove all features (except num-integer-backend and rug-backend): to avoid unnecessary recompilation when, for example, building the aggregator then the client as they don't use the same features set.
  • Make the resulting crate dependency tree as flat as possible
  • Allowing nodes to

Additional, but conflicting, criteria:

  • Minimize modules structure changes: to limit the number of changes needed downstream
  • Better cohesion and coupling: by grouping things by domain

Preliminary work

Removal of the random feature

I found that the random feature does not prevent compilation of the wasm client anymore, as such it could be removed. To do so I removed the feature entirely from mithril-client and could run the mithril-client-wasm tests without issues.

If with further testing we found that there's still an incompatibility or preventively since this would be a runtime error, we can should add the following dependency to mithril-client-wasm:

getrandom = { version = "0.2", features = ["js"] }

(as detailed in getrandom documentation)

Removal of most circular dependencies

Most of the circular dependencies can be easily removed by establishing a tree dependency structure.

The following proposal will based on the following tree:

flowchart BT
    messages --> entities
    protocol --> entities
    signable_builder --> entities
Loading

entities <-> messages

Only one offender: impl TryFrom<CardanoTransactionsSetProof> for CardanoTransactionsSetProofMessagePart { (in /entities/cardano_transactions_set_proof.rs).

Solution: reverse the dependency by moving this trait impl to /messages/message_parts/cardano_transactions_set_proof.rs

entities <-> protocol

Only one offender: impl ToMessage for ProtocolMessage { (in /entities/protocol_message.rs).

Solution: reverse the dependency by moving this trait impl to /protocol/mod.rs.

entities <-> signable_builder

Multiples implementation of the signable_builder Artifact and Beacon traits.

Solution: reverse the dependency by doing them below those traits declaration in the signable_builder module.

Not easily removable

entities <-> crypto_helper

Those two modules are part of the oldest of the whole project and as such share much more dependencies than the previous cases.

I think we can safely say that tree order should be crypto_helper <-- entities since multiple 'protocol keys' are used in the entities. If we took this assumption as a basis, here are the offenders:

  • crypto_helper/conversion.rs: this whole file is about conversions from types coming from mithril-stm to their entities equivalent, it could be moved to the entities module, with a more specific name, to solve the issue.
  • crypto_helper/tests_setup.rs: multiple usages of entities, we could move the methods declared here to the test_utils module.
  • crypto_helper/merkle_map.rs tests: usages of the BlockRange and BlockNumber entities, a lead would be to remove them from those tests and use locally defined types instead (the behaviours tested here should be independent of those types).
  • crypto_helper/codec.rs: usages of HexEncodedKey and HexEncodedKeySlice, we could reverse the dependency by declaring them here instead of in entities/type_alias.rs.
  • crypto_helper/types/wrappers.rs: the ProtocolMkProof protocol key alias use a BlockRange, to solve this circularity we would have to move this alias to entities/type_alias.rs but doing so we should move all other aliases.
chain_reader <-> cardano_block_scanner

The chain_reader::ChainBlockNextAction use cardano_block_scanner::RawCardanoPoint and cardano_block_scanner::ScannedBlock. Those types were originally introduced to simplify the cardano_block_scanner api but later we decided to also use them in its underlying chain_reader.

A simple fix would be to move them to the chain_reader modules.

Another fix could be to merge those two modules together instead as they are intricately related, here's a proposal for the resulting module structure (all names are tentative):

flowchart RL
    entity --> chain_reader
    interface --> chain_reader
    chain_block_reader --> chain_reader
    block_streamer --> chain_reader
    chain_block_next_action --> entity
    raw_cardano_point --> entity
    scanned_block --> entity
    pallas --> chain_block_reader
    fake --> chain_block_reader
    chain_reader_block_streamer --> block_streamer
    dumb --> block_streamer
Loading

Proposal - Domain based split

Note

All names are tentative

Note

Due to mermaid graph limitations some names that are the same in more than more crate (test and signable_builder) are suffixed to avoid conficts.

In this proposal code is grouped by domain and

flowchart RL
    subgraph mithril-entities
    direction BT
    entities --> crypto_helper
    messages --> entities
    StdResult
    test-ent
    fake_data -.-> test-ent
    fake_keys -.-> test-ent
    end

    subgraph mithril-shared-signing-nodes-services
    direction BT
    resource_pool
    signed_entity_type_lock
    end
    mithril-shared-signing-nodes-services --> mithril-entities
    
    subgraph mithril-protocol
    direction BT
    certificate-chain
    signer
    multi-signer --> signer
    single-signer --> signer
    signable_builder
    test-prot
    certificate_chain_builder -.-> test-prot
    fixture_builder -.-> test-prot
    end
    mithril-protocol --> mithril-entities
    
    subgraph mithril-cardano-db
    direction BT
    digester
    signable_builder_cdb --> digester
    test-cdb
    dummy_cardano_db -.-> test-cdb
    end
    mithril-cardano-db --> mithril-entities & mithril-protocol
    
    subgraph mithril-cardano-chain-interop
    direction BT
    chain_observer
    cardano_block_scanner --> chain_reader
    ticker_service --> chain_observer
    cardano_era_adapter --> chain_observer
    cardano_transactions_preloader --> chain_observer
    end
    mithril-cardano-chain-interop --> mithril-entities & mithril-era
    
    mithril-era --> mithril-entities
    mithril-logging
    mithril-web-test-tools -.-> mithril-era
    
    classDef test stroke-dasharray: 5 5
    class test-ent,test-prot,test-cdb,fake_data,fake_keys,certificate_chain_builder,fixture_builder,dummy_cardano_db,mithril-web-test-tools test;
Loading

Details

  • We may want to merge the mithril-entities crate into mithril-protocol as those two crates will be used by all nodes and the client library.
  • In mithril-protocol the signer module correspond to the current protocol module.
  • The mithril-cardano-db crate would encompass everything that need to access a cardano database filesystem (ie: to compute digests).
  • The mithril-cardano-chain-interop crate would encompass everything that need communicate with a node.
  • Some existing modules will be dispatched in two crates:
    • signable_builder: dispatched between mithril-protocol and mithril-cardano-db crates. Almost everything would be in mithril-protocol, mithril-cardano-db
      would have the CardanoDatabaseSignableBuilder and the CardanoImmutableFilesFullSignableBuilder signable builders. This would allow mithril-client-lib to only include file system based signable builder if its fs feature is enabled .
    • era: dispatched between mithril-era and mithril-cardano-chain-interop crates. Almost everything would be in mithril-era, mithril-cardano-chain-interop
      would have the adapter that read era in the chain. This allow client crate to avoid a direct dependency to pallas.
  • The mithril-logging crate would be used by almost every new crates, those links have been skipped here to keep the graph simple
  • The mithril-shared-signing-nodes-services include services that have little to no dependencies and are used only by 'signing' nodes, aka only by the aggregator and the signers. This allow the mithril-client-lib to not include those services as it doesn't use them.
  • The mithril-web-test-tools include apispec and test_http_server.

Caveats

  • The fixture builder is used in some fake_data methods (ie: signers, certificates, ...), as such we would have to either replace its usage with manual, but probably cryptography incorrect, values, or move those methods to the mithril-protocol crate.
  • The mithril-web-test-tools have to depends on mithril-era since the apispec have a method that use the SupportedEra enum to identify which openapi file to use: this is quite cumbersome could this be avoided or moved elsewhere ? Note that this method is unused right now.
  • The mithril-shared-signing-nodes-services doesn't fit as well as the other proposed crates as I could only describe by who are using it than what it really encompass.
  • The era::adapter::AdapterBuilder would needs to be refactored to allow to define builders outside of the module, like we do for signable builders. This is
    needed to remove the direct dependency to pallas.

Resulting nodes dependencies

Note

To simplify the graph mithril-era, mithril-logging and mithril-web-test-tools are omitted.

flowchart RL
    mithril-entities
    mithril-protocol --> mithril-entities
    mithril-cardano-chain-interop --> mithril-entities
    mithril-cardano-db --> mithril-entities & mithril-protocol
    mithril-shared-signing-nodes-services --> mithril-entities
    
    mithril-client-lib --> mithril-entities & mithril-protocol & mithril-cardano-db
    mithril-aggregator --> mithril-entities & mithril-protocol & mithril-cardano-db & mithril-cardano-chain-interop & mithril-shared-signing-nodes-services
    mithril-signer --> mithril-entities & mithril-protocol & mithril-cardano-db & mithril-cardano-chain-interop & mithril-shared-signing-nodes-services
Loading

Crates that would be published to crates.io

The following crates would needs to be published as they would be dependencies (direct or indirect) of mithril-client-lib:

  • mithril-entities
  • mithril-protocol
  • mithril-cardano-db
  • mithril-era: dependency of mithril-web-test-tools
  • mithril-logging
  • mithril-web-test-tools

Alternative Proposal

Note

All names are tentative

Note

Due to mermaid graph limitations some names that are the same in more than more crate (test and signable_builder) are suffixed to avoid conficts.

This is a strealined version of the previous proposal with less crates by merging together the mithril-entities, mithril-protocol, mithril-era and mithril-logging.

This design can be seen as an intermediate step toward the first proposal.

flowchart BT
    subgraph mithril-protocol
    direction BT
    StdResult
    entities --> crypto_helper
    messages --> entities
    certificate-chain --> entities & crypto_helper
    signer --> entities & crypto_helper
    multi-signer --> signer
    single-signer --> signer
    signable_builder --> entities & crypto_helper
    era
    logging
    test
    fake_data -.-> test
    fake_keys -.-> test
    certificate_chain_builder -.-> test
    fixture_builder -.-> test
    end
    
    subgraph mithril-shared-signing-nodes-services
    direction BT
    resource_pool
    signed_entity_type_lock
    end
    mithril-shared-signing-nodes-services --> mithril-protocol
    
    subgraph mithril-cardano-db
    direction BT
    digester
    signable_builder_cdb --> digester
    test-cdb
    dummy_cardano_db -.-> test-cdb
    end
    mithril-cardano-db --> mithril-protocol
    
    subgraph mithril-cardano-chain-interop
    direction BT
    chain_observer
    cardano_block_scanner --> chain_reader
    ticker_service --> chain_observer
    cardano_era_adapter --> chain_observer
    cardano_transactions_preloader --> chain_observer
    end
    mithril-cardano-chain-interop --> mithril-protocol
    
    mithril-web-test-tools -.-> mithril-protocol
    
    classDef test stroke-dasharray: 5 5
    class test,test-cdb,fake_data,fake_keys,certificate_chain_builder,fixture_builder,dummy_cardano_db,mithril-web-test-tools test;
Loading

Details

See details of the previous proposal as most of them also applies here.

Caveats

This design avoid the first two caveats of the previous proposal since the era and fixture codes remains in the protocol crates, but do not avoid the other two.

Resulting nodes dependencies

Note

To simplify the graph mithril-web-test-tools is omitted.

flowchart RL
    mithril-protocol
    mithril-cardano-chain-interop --> mithril-protocol
    mithril-cardano-db --> mithril-protocol
    mithril-shared-signing-nodes-services --> mithril-protocol
    
    mithril-client-lib --> mithril-protocol & mithril-cardano-db
    mithril-aggregator --> mithril-protocol & mithril-cardano-db & mithril-cardano-chain-interop & mithril-shared-signing-nodes-services
    mithril-signer --> mithril-protocol & mithril-cardano-db & mithril-cardano-chain-interop & mithril-shared-signing-nodes-services
Loading

Crates that would be published to crates.io

The following crates would needs to be published as they would be dependencies (direct or indirect) of mithril-client-lib:

  • mithril-protocol
  • mithril-cardano-db
  • mithril-web-test-tools

Roll-out plan

The first two points are shared between the two proposals

  1. Removal of the random feature
  2. Removal of most circular dependencies
  3. Extraction of the mithril-cardano-chain-interopt crate as the code included isn't used by mithril-client-lib
  4. Extraction of the mithril-shared-signing-nodes-services crate as the code included isn't used by mithril-client-lib
  5. Extraction of the mithril-cardano-db crate as there's mainly one module to extract plus some independent structs ⚠️ will be published to crates.io
  6. Extraction of the mithril-web-test-tools crate ⚠️ will be published to crates.io
  7. Renaming of mithril-common to mithril-protocol and internal reorganisation (renaming of the protocol module to signer) ⚠️ will be published to crates.io, is there's something to handle renaming ?

If going with the first proposal:
8. Extraction of the mithril-era crate ⚠️ will be published to crates.io
9. Extraction of the mithril-logging crate ⚠️ will be published to crates.io
10. Extraction of the mithril-entities crate ⚠️ will be published to crates.io

@Alenar
Copy link
Collaborator

Alenar commented Jan 10, 2025

Complementary proposal: extra domain splitting

Note

This is based on the "Alternative Proposal" in the previous post.

The entities and crypto_helper modules are defined on a technical basis, but the project have grown a lot since it's beginnings leading those two modules, especially entities, to loose focus and encompass a lot of concepts.

It would make our code more readable, for us or auditors, if redesign what would be remaining in the "protocol" crate by domain.

We could either keep the resulting structure in the "protocol" crate or extract them to dedicated crates.

Domains proposal

Note

All names are tentative

---
config:
  nodeAlignment: "left"
---

flowchart BT
    beacon["`
    **beacon**
    epoch
    block_number
    block_range
    cardano_chain_point
    cardano_db_beacon
    slot_number
    time_point
    signed_entity_config?
    `"]

    certificate["`
    **certificate**
    certificate_chain
    genesis
    entities::certificate
    entities::certificate_metadata
    entities::signed_entity_type
    `"]

    signable["`
    **signable**
    signable_builder
    entities/signed_entity
    entities/cardano_database
    entities/cardano_stake_distribution
    entities/cardano_transactions_snapshot
    entities/mithril_stake_distribution
    entities/snapshot
    `"]
    
    signer["`
      **signer/protocol**
      multi_signer
      single_signer
      signer_builder
      key_certification
      entities/signer
      entities/single_signature
      entities/protocol_message
    `"]
    
    cardano["`
      **cardano**
      codec
      cold_key
      opcert
    `"]
    
    merkle_tree["`
       **merkle_tree**
        merkle_tree
        merkle_map
    `"]
    
    signer --> beacon & cardano
    certificate --> beacon & signer
    signable --> certificate & cardano & merkle_tree & signer
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring 🛠️ Code refactoring and enhancements
Projects
None yet
Development

No branches or pull requests

2 participants