Chore: clean TODOs in repository #2206

jpraynaud · 2025-01-09T16:39:30Z

Content

This PR includes a cleanup of some TODOs comments in the repository that could either be removed or fixed.

TODO (before cleanup)

└─ mithril
   ├─ internal
   │  ├─ mithril-doc
   │  │  ├─ lib.rs
   │  │  │  └─ line 3: TODO : Some Configuration could not be generated properly because there is a lack of information. (TO EXPLAIN)
   │  │  └─ test_doc_macro.rs
   │  │     └─ line 44: TODO May be part of StructDoc. (FIXED)
   │  └─ mithril-persistence
   │     ├─ repository
   │     │  └─ cardano_transaction_repository.rs
   │     │     └─ line 158: TODO : remove this collect to return the iterator directly (CREATE ISSUE)
   │     └─ hydrator.rs
   │        └─ line 23: TODO : Maybe there is a better way of doing this. (FIXED)
   ├─ mithril-aggregator
   │  ├─ src
   │  │  ├─ dependency_injection
   │  │  │  ├─ builder.rs
   │  │  │  │  └─ line 1313: TODO : Make this part of a warmup phase of the aggregator?  (CREATE ISSUE)
   │  │  │  └─ containers.rs
   │  │  │     └─ line 53: TODO : remove this field and only use the `Configuration` in the dependencies builder (CREATE ISSUE)
   │  │  ├─ file_uploaders
   │  │  │  └─ local_snapshot_uploader.rs
   │  │  │     └─ line 13: TODO : This specific local uploader will be removed. (FIXED)
   │  │  ├─ http_server
   │  │  │  ├─ cardano_database.rs
   │  │  │  │  ├─ line 54: TODO : the directory opened here should be narrowed to the final directory where the artifacts are stored (CREATE ISSUE)
   │  │  │  │  ├─ line 118: TODO : this function should probable be unit tested once the file naming convention is defined (CREATE ISSUE)
   │  │  │  │  └─ line 136: TODO : enhance this check with a regular expression once the file naming convention is defined (CREATE ISSUE)
   │  │  │  └─ snapshot.rs
   │  │  │     ├─ line 71: TODO : This legacy route should be removed when this code is released with a new distribution (FIXED)
   │  │  │     └─ line 85: TODO : This legacy route should be removed when this code is released with a new distribution (FIXED)
   │  │  └─ configuration.rs
   │  │     └─ line 337: TODO : This function should be completed when the configuration of the uploaders for the Cardano database is done. (CREATE ISSUE)
   │  └─ tests
   │     └─ aggregator_observer.rs
   │        └─ line 141: TODO : This case will be implemented once the signable and artifact builders are available. (TO REFERENCE IN ISSUE #2151)
   ├─ mithril-client-cli
   │  └─ mod.rs
   │     └─ line 39: TODO : This should not be done this way. (CREATE ISSUE)
   ├─ mithril-common
   │  ├─ chain_observer
   │  │  ├─ cli_observer.rs
   │  │  │  └─ line 531: TODO : This function implements a fallback mechanism to compute the stake distribution: new/optimized computation when available, legacy computation otherwise (CREATE ISSUE)
   │  │  └─ model.rs
   │  │     └─ line 147: TODO : Remove this chunking of the bytes fields once the cardano-cli 1.36.0+ is released (CREATE ISSUE)
   │  ├─ crypto_helper
   │  │  └─ key_certification.rs
   │  │     ├─ line 46: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │     ├─ line 249: TODO : Parameter should be removed once the signer certification is fully deployed (FIXED)
   │  │     ├─ line 250: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  │     └─ line 251: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  ├─ entities
   │  │  ├─ signed_entity_config.rs
   │  │  │  └─ line 162: TODO : See if we can remove this adjustment by including a "partial" block range in (CREATE ISSUE)
   │  │  ├─ signer.rs
   │  │  │  ├─ line 17: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  ├─ line 24: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  ├─ line 29: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  ├─ line 34: TODO : This kes period should not be used as is and should probably be within an allowed range of kes period for the epoch (TO EXPLAIN)
   │  │  │  ├─ line 139: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  ├─ line 146: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  ├─ line 151: TODO : Option should be removed once the signer certification is fully deployed (FIXED)
   │  │  │  └─ line 156: TODO : This kes period should not be used as is and should probably be within an allowed range of kes period for the epoch (TO EXPLAIN)
   │  │  └─ single_signatures.rs
   │  │     └─ line 68: TODO : this method is slow due to the fixture creation, we should either make (FIXED)
   │  ├─ messages
   │  │  ├─ message_parts
   │  │  │  ├─ signed_entity_type_message.rs
   │  │  │  │  ├─ line 13: TODO : Remove this enum when all nodes are updated, and use the [CardanoDbBeacon] directly as before. (CREATE ISSUE)
   │  │  │  │  └─ line 33: TODO : Remove this enum when all nodes are updated, and use the [SignedEntityType] directly as before. (CREATE ISSUE)
   │  │  │  └─ signer.rs
   │  │  │     ├─ line 19: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │  │     ├─ line 27: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │  │     ├─ line 34: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │  │     ├─ line 40: TODO : This KES period should not be used as is and should probably be
   │  │  │     ├─ line 164: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │  │     ├─ line 172: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │  │     ├─ line 179: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │  │     └─ line 185: TODO : This KES period should not be used as is and should probably be (TO EXPLAIN)
   │  │  └─ register_signer.rs
   │  │     ├─ line 16: TODO : Should be removed once the signer certification is fully deployed (FIXED)
   │  │     ├─ line 24: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │     ├─ line 31: TODO : Option should be removed once the signer certification is fully (FIXED)
   │  │     └─ line 37: TODO : This KES period should not be used as is and should probably be (TO EXPLAIN)
   │  └─ test_utils
   │     └─ apispec.rs
   │        └─ line 315: TODO : For now, it verifies only one parameter, (CREATE ISSUE)
   ├─ mithril-relay
   │  ├─ src
   │  │  ├─ aggregator.rs
   │  │  │  ├─ line 40: TODO : retrieve current version (CREATE ISSUE)
   │  │  │  └─ line 68: TODO : retrieve current version (CREATE ISSUE)
   │  │  └─ passive.rs
   │  │     ├─ line 10: TODO : should be private (FIXED)
   │  │     └─ line 28: TODO : should be removed (FIXED)
   │  └─ tests
   │     └─ register_signer_signature.rs
   │        └─ line 16: TODO : this test is not optimal and should be refactored for better performances, (CREATE ISSUE)
   ├─ mithril-signer
   │  └─ configuration.rs
   │     └─ line 55: TODO : Field should be removed once the signer certification is fully deployed (FIXED)
   └─ mithril-test-lab
      ├─ mithril-devnet
      │  ├─ configuration
      │  │  └─ configuration.yaml
      │  │     └─ line 19: TODO : These parameters cannot yet be used in the config file, only on the CLI: (FIXED)
      │  └─ mkfiles
      │     └─ mkfiles-docker.sh
      │        └─ line 232: TODO : Should be removed once the signer certification is fully deployed (FIXED)
      └─ mithril-end-to-end
         ├─ mithril
         │  └─ infrastructure.rs
         │     └─ line 262: TODO : Should be removed once the signer certification is fully deployed (FIXED)
         └─ end_to_end_spec.rs
            └─ line 211: TODO : uncomment when the client can verify Cardano database snapshots (FIXED)

TODO (after cleanup)

└─ mithril
   ├─ internal
   │  ├─ mithril-doc
   │  │  └─ lib.rs
   │  │     └─ line 3: TODO : Some Configuration could not be generated properly because there is a lack of information. (TO EXPLAIN)
   │  └─ mithril-persistence
   │     └─ cardano_transaction_repository.rs
   │        └─ line 158: TODO : remove this collect to return the iterator directly (CREATE ISSUE)
   ├─ mithril-aggregator
   │  ├─ src
   │  │  ├─ dependency_injection
   │  │  │  ├─ builder.rs
   │  │  │  │  └─ line 1313: TODO : Make this part of a warmup phase of the aggregator?  (CREATE ISSUE)
   │  │  │  └─ containers.rs
   │  │  │     └─ line 53: TODO : remove this field and only use the `Configuration` in the dependencies builder (CREATE ISSUE)
   │  │  ├─ http_server
   │  │  │  └─ cardano_database.rs
   │  │  │     ├─ line 54: TODO : the directory opened here should be narrowed to the final directory where the artifacts are stored (CREATE ISSUE)
   │  │  │     ├─ line 118: TODO : this function should probable be unit tested once the file naming convention is defined (CREATE ISSUE)
   │  │  │     └─ line 136: TODO : enhance this check with a regular expression once the file naming convention is defined (CREATE ISSUE)
   │  │  └─ configuration.rs
   │  │     └─ line 337: TODO : This function should be completed when the configuration of the uploaders for the Cardano database is done. (CREATE ISSUE)
   │  └─ tests
   │     └─ aggregator_observer.rs
   │        └─ line 141: TODO : This case will be implemented once the signable and artifact builders are available. (TO REFERENCE IN ISSUE #2151)
   ├─ mithril-client-cli
   │  └─ mod.rs
   │     └─ line 39: TODO : This should not be done this way. (CREATE ISSUE)
   ├─ mithril-common
   │  ├─ chain_observer
   │  │  ├─ cli_observer.rs
   │  │  │  └─ line 531: TODO : This function implements a fallback mechanism to compute the stake distribution: new/optimized computation when available, legacy computation otherwise (CREATE ISSUE)
   │  │  └─ model.rs
   │  │     └─ line 147: TODO : Remove this chunking of the bytes fields once the cardano-cli 1.36.0+ is released (CREATE ISSUE)
   │  ├─ entities
   │  │  ├─ signed_entity_config.rs
   │  │  │  └─ line 162: TODO : See if we can remove this adjustment by including a "partial" block range in (CREATE ISSUE)
   │  │  └─ signer.rs
   │  │     └─ line 37: TODO : This kes period should not be used as is and should probably be within an allowed range of kes period for the epoch (TO EXPLAIN)
   │  │     └─ line 156: TODO : This kes period should not be used as is and should probably be within an allowed range of kes period for the epoch (TO EXPLAIN)
   │  ├─ messages
   │  │  ├─ message_parts
   │  │  │  ├─ signed_entity_type_message.rs
   │  │  │  │  ├─ line 13: TODO : Remove this enum when all nodes are updated, and use the [CardanoDbBeacon] directly as before. (CREATE ISSUE)
   │  │  │  │  └─ line 33: TODO : Remove this enum when all nodes are updated, and use the [SignedEntityType] directly as before. (CREATE ISSUE)
   │  │  │  └─ signer.rs
   │  │  │     ├─ line 41: TODO : This KES period should not be used as is and should probably be (TO EXPLAIN)
   │  │  │     └─ line 187: TODO : This KES period should not be used as is and should probably be (TO EXPLAIN)
   │  │  └─ register_signer.rs
   │  │     └─ line 38: TODO : This KES period should not be used as is and should probably be (TO EXPLAIN)
   │  └─ test_utils
   │     └─ apispec.rs
   │        └─ line 315: TODO : For now, it verifies only one parameter, (CREATE ISSUE)
   ├─ mithril-relay
   │  ├─ src
   │  │  └─ aggregator.rs
   │  │     ├─ line 40: TODO : retrieve current version (CREATE ISSUE)
   │  │     └─ line 68: TODO : retrieve current version (CREATE ISSUE)
   │  └─ tests
   │     └─ register_signer_signature.rs
   │        └─ line 16: TODO : this test is not optimal and should be refactored for better performances, (CREATE ISSUE)

Pre-submit checklist

Branch
- 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

…loader.rs'

…es/snapshot.rs' Released with distribution 2321.1 which is not run by SPOs anymore.

github-actions · 2025-01-09T16:54:32Z

Test Results

4 files ±0 52 suites ±0 10m 25s ⏱️ +2s
1 493 tests ±0 1 493 ✅ ±0 0 💤 ±0 0 ❌ ±0
1 749 runs ±0 1 749 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 55b2e82. ± Comparison against base commit 493c182.

♻️ This comment has been updated with latest results.

Fixed by #2153.

…figuration.yaml'

….rs'

deployed' - 'mithril-common/src/crypto_helper/cardano/key_certification.rs' - 'mithril-common/src/entities/signer.rs' - 'mithril-common/src/messages/message_parts/signer.rs' - 'mithril-common/src/messages/register_signer.rs' - 'mithril-signer/src/configuration.rs' - 'mithril-test-lab/mithril-devnet/mkfiles/mkfiles-docker.sh' - 'mithril-test-lab/mithril-end-to-end/src/mithril/infrastructure.rs'

sfauvel

LGTM

jpraynaud added 3 commits January 9, 2025 15:29

fix: TODO in 'internal/mithril-doc/test_doc_macro.rs'

e0b51c8

fix: TODO in 'mithril-aggregator/src/file_uploaders/local_snapshot_up…

ac53cd5

…loader.rs'

fix: TODO in 'mithril-aggregator/src/http_server/routes/artifact_rout…

8a6c81f

…es/snapshot.rs' Released with distribution 2321.1 which is not run by SPOs anymore.

jpraynaud self-assigned this Jan 9, 2025

jpraynaud temporarily deployed to testing-preview January 9, 2025 16:56 — with GitHub Actions Inactive

jpraynaud temporarily deployed to testing-sanchonet January 9, 2025 16:56 — with GitHub Actions Inactive

jpraynaud added 5 commits January 9, 2025 18:00

fix: TODO in 'mithril-common/src/entities/single_signatures.rs'

e3926b9

Fixed by #2153.

fix: TODO in 'mithril-relay/src/relay/passive.rs'

8a233da

fix: TODO in 'mithril-test-lab/mithril-devnet/configuration/byron/con…

68688e4

…figuration.yaml'

fix: TODO in 'mithril-test-lab/mithril-end-to-end/src/end_to_end_spec…

423e225

….rs'

jpraynaud force-pushed the jpraynaud/fix-todos branch from 03b1f4b to 55b2e82 Compare January 9, 2025 17:00

jpraynaud marked this pull request as ready for review January 9, 2025 17:00

jpraynaud requested review from Alenar, sfauvel and dlachaume January 9, 2025 17:01

Alenar approved these changes Jan 9, 2025

View reviewed changes

jpraynaud temporarily deployed to testing-preview January 9, 2025 17:19 — with GitHub Actions Inactive

jpraynaud temporarily deployed to testing-sanchonet January 9, 2025 17:19 — with GitHub Actions Inactive

dlachaume approved these changes Jan 10, 2025

View reviewed changes

sfauvel approved these changes Jan 10, 2025

View reviewed changes

jpraynaud merged commit 62578b1 into main Jan 10, 2025
51 checks passed

jpraynaud deleted the jpraynaud/fix-todos branch January 10, 2025 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: clean TODOs in repository #2206

Chore: clean TODOs in repository #2206

jpraynaud commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

sfauvel left a comment

Chore: clean TODOs in repository #2206

Chore: clean TODOs in repository #2206

Conversation

jpraynaud commented Jan 9, 2025 • edited Loading

Content

TODO (before cleanup)

TODO (after cleanup)

Pre-submit checklist

github-actions bot commented Jan 9, 2025 • edited Loading

Test Results

sfauvel left a comment

Choose a reason for hiding this comment

jpraynaud commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading