Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Chore: clean TODOs in repository (second round) #2220

Merged
merged 8 commits into from
Jan 14, 2025
Merged

Conversation

jpraynaud
Copy link
Member

@jpraynaud jpraynaud commented Jan 13, 2025

Content

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

TODO (41 remaining 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
│ ├─ 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.
├─ mithril-aggregator
│ ├─ src
│ │ ├─ dependency_injection
│ │ │ ├─ builder.rs
│ │ │ │ ├─ line 419: todo : add capacity to create a connection pool to the `ConnectionBuilder` (CREATE ISSUE)
│ │ │ │ └─ 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)
│ │ ├─ tools
│ │ │ └─ digest_helpers.rs
│ │ │ └─ line 35: @todo : maybe we should add a length check (TO EXPLAIN)
│ │ └─ 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
│ └─ memory_cache.rs
│ └─ line 65: todo : should we raise an error if an empty string is given for previous_certificate_hash ? (or any other kind of validation) (CREATE ISSUE)
├─ 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
│ │ ├─ codec.rs
│ │ │ └─ line 33: todo : create the KES key directly from a buffer instead of deserialising from disk (TO EXPLAIN)
│ ├─ 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 162: 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-explorer
│ └─ index.js
│ └─ line 80: todo : uncomment when the fallback is removed (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)
├─ mithril-stm
│ ├─ merkle_tree.rs
│ │ ├─ line 199: todo : We should not panic if the size of the slice is invalid (I believe `bytes[offset + i * 8..offset + (i + 1) * 8]` will panic if bytes is not large enough. (TO EXPLAIN)
│ │ ├─ line 277: todo : Do we need to concat msg to whole commitment (nr_leaves and root) or just the root? (TO EXPLAIN)
│ │ ├─ line 293: todo : Update doc. (CREATE ISSUE)
│ │ ├─ line 294: todo : Simplify the algorithm. (CREATE ISSUE)
│ │ ├─ line 295: todo : Maybe we want more granular errors, rather than only `BatchPathInvalid` (CREATE ISSUE)
│ │ └─ line 499: todo : Update doc. (CREATE ISSUE)
│ └─ stm.rs
│ ├─ line 140: todo : this is the criteria to consider parameters valid: (CREATE ISSUE)
│ ├─ line 960: todo : We need to agree on a criteria to dedup (by default we use a BTreeMap that guarantees keys order) (TO EXPLAIN)
│ ├─ line 961: todo : not good, because it only removes index if there is a conflict (see benches) (TO EXPLAIN)
│ └─ line 1520: todo : fn test_invalid_proof_individual_sig (CREATE ISSUE)
└─ mithril-test-lab
└─ fake_client.rs
└─ line 121: todo : redraw this progress bar but in a MultiProgressBar, drawing it as is create (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

@jpraynaud jpraynaud self-assigned this Jan 13, 2025
Copy link

github-actions bot commented Jan 13, 2025

Test Results

    4 files  ±0     52 suites  ±0   10m 18s ⏱️ -12s
1 494 tests +1  1 494 ✅ +1  0 💤 ±0  0 ❌ ±0 
1 750 runs  +1  1 750 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 63b9cd2. ± Comparison against base commit 368e5a9.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

* mithril-persistence from `0.2.42` to `0.2.43`
* mithril-aggregator from `0.6.11` to `0.6.12`
* mithril-common from `0.4.102` to `0.4.103`
* mithril-stm from `0.3.35` to `0.3.36`
* mithril-end-to-end from `0.4.62` to `0.4.63`
@jpraynaud jpraynaud temporarily deployed to testing-sanchonet January 14, 2025 10:13 — with GitHub Actions Inactive
@jpraynaud jpraynaud merged commit 584e2c6 into main Jan 14, 2025
43 checks passed
@jpraynaud jpraynaud deleted the jpraynaud/fix-todos-2 branch January 14, 2025 10:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants