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

Replace BlockNumber alias with dedicated structure #1846

Merged
merged 19 commits into from
Jul 23, 2024

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Jul 19, 2024

Content

This PR replace the BlockNumber alias over u64 with dedicated tuple structure.

In order to share code and simplify implementation of tuple structs that wrap an integer type, some macros have been defined to easily implement recurring traits such as arithmetic operations.
Those macros are defined in mithril-common/src/entities/arithmetic_operation_wrapper.rs.

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
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Comments

About the traits exclusively implemented on BlockNumber :

  • impl TryFrom<BlockNumber> for i64: implemented to facilitate usage with the database layer as the sqlite crate use a i64 for all integers.
  • BlockRange::new(start: u64, end: u64) is provided but for test usage only to reduce verbosity.

About SlotNumber: a type was defined but removed to keep this PR focused on BlockNumber.
Reverting the commit 997cb3a can be a good starting point for the PR for slot number.

Issue(s)

Relates to #1755

Copy link

github-actions bot commented Jul 19, 2024

Test Results

    4 files  ± 0     52 suites  ±0   9m 14s ⏱️ +8s
1 157 tests +13  1 157 ✅ +13  0 💤 ±0  0 ❌ ±0 
1 323 runs  +13  1 323 ✅ +13  0 💤 ±0  0 ❌ ±0 

Results for commit 664f3e4. ± Comparison against base commit 9f8b589.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-sanchonet July 19, 2024 16:50 — with GitHub Actions Inactive
@Alenar Alenar force-pushed the ensemble/1755/block_slot_number_type branch from d711d05 to dc0e083 Compare July 19, 2024 16:57
@Alenar Alenar temporarily deployed to testing-sanchonet July 19, 2024 17:04 — 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.

Overall looks good 👍

mithril-common/src/entities/block_number.rs Outdated Show resolved Hide resolved
mithril-common/src/entities/block_range.rs Outdated Show resolved Hide resolved
mithril-signer/src/cardano_transactions_importer.rs Outdated Show resolved Hide resolved
mithril-common/src/entities/wrapper_helpers.rs Outdated Show resolved Hide resolved
@sfauvel sfauvel force-pushed the ensemble/1755/block_slot_number_type branch from 2025826 to 6dc54be Compare July 23, 2024 13:51
@sfauvel sfauvel temporarily deployed to testing-sanchonet July 23, 2024 13:58 — 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 👍

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Just a minor suggestion.

mithril-common/src/entities/signed_entity.rs Show resolved Hide resolved
Alenar added 15 commits July 23, 2024 16:44
A wrapper on `u64` with comparaison, add, sub, display, and deref built
in.

It will replace an alias over `u64` so we can have stronger type check
at compilation and more readable code.
By defining macros.

This add several impl that were missing for the `Epoch` type (mostly for
operations that use a reference).
A wrapper on `u64` with comparaison, add, sub, display, and deref built
in.

It will replace an alias over `u64` so we can have stronger type check
at compilation and more readable code.
To allow them when the wrapped type is on the left.
Only common compile, other crates still needs some work.

In order to do so some more capabilities add to be added to the
`BlockNumber`:
* Convertion from u64: to use with 'Into<BlockNumber>' generics, should
  be reserved to specific cases when it's clear that the number written
  is a Block Number and not something magic.
* PartialOrd from/to `u64`: so it can be passed to Range::contains
  method.
Aggregator & signer still doesn't compile.

In order to do so some more capabilities add to be added to the
`BlockNumber`:
* Faillible conversion to i64: since it's the target datatype for
  sqlite integer fields.
Aggregator still doesn't compile.
This commit can be reverted to be the starting point of the PR that add
the `SlotNumber`.
Alenar and others added 4 commits July 23, 2024 16:45
mithril-signer:0.2.167 => 0.2.168
mithril-persistence:0.2.18 => 0.2.19
mithril-common:0.4.34 => 0.4.35
mithril-aggregator:0.5.46 => 0.5.47
mithril-client:0.8.7 => 0.8.8
@sfauvel sfauvel force-pushed the ensemble/1755/block_slot_number_type branch from 6dc54be to 664f3e4 Compare July 23, 2024 14:51
@sfauvel sfauvel temporarily deployed to testing-sanchonet July 23, 2024 15:08 — with GitHub Actions Inactive
@sfauvel sfauvel merged commit 0e21965 into main Jul 23, 2024
43 checks passed
@sfauvel sfauvel deleted the ensemble/1755/block_slot_number_type branch July 23, 2024 15:09
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.

4 participants