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 use of Duration for timestamps with an actual timestamp, e.g. chrono::DateTime #4729

Open
0x009922 opened this issue Jun 13, 2024 · 8 comments · May be fixed by #5015
Open

Replace use of Duration for timestamps with an actual timestamp, e.g. chrono::DateTime #4729

0x009922 opened this issue Jun 13, 2024 · 8 comments · May be fixed by #5015
Assignees
Labels
Enhancement New feature or request good first issue Good for newcomers

Comments

@0x009922
Copy link
Contributor

In some places in iroha_data_model, structures use Duration for data which doesn't mean a span of time, but actually means a timestamp. Examples:

  • start:
    pub struct Schedule {
    /// The first execution time
    pub start: Duration,
    /// If some, the period between cyclic executions
    pub period: Option<Duration>,
    }
  • since:
    pub struct TimeInterval {
    /// The start of a time interval
    pub since: Duration,
    /// The length of a time interval
    pub length: Duration,
    }

These fields are timestamps, not time spans.

I propose to use a structure that is designed specifically for timestamps, e.g. chrono::DateTime.

@0x009922 0x009922 added Enhancement New feature or request good first issue Good for newcomers labels Jun 13, 2024
@hollermay
Copy link

Can I work on this issue?
I can help around this

@nxsaken
Copy link
Contributor

nxsaken commented Jun 19, 2024

Hi @hollermay, feel free to open a PR.

@mversic
Copy link
Contributor

mversic commented Jun 19, 2024

start should become start_ms and since should become since_ms. However, getters should return something like chrono::DateTime

@hollermay should I assign you?

@hollermay
Copy link

start should become start_ms and since should become since_ms. However, getters should return something like chrono::DateTime

@hollermay should I assign you?

Sure I can give a shot

@mversic
Copy link
Contributor

mversic commented Jun 19, 2024

also do the same for SignedTransactionV1::creation_time_ms and SignedBlockV1::timestamp_ms

@hollermay

This comment was marked as resolved.

@dima74
Copy link
Contributor

dima74 commented Aug 23, 2024

u64 is used for timestamps after #4841:

pub struct Schedule {
/// The first execution time
pub start_ms: u64,
/// If some, the period between cyclic executions
pub period_ms: Option<u64>,
}

@mversic should this issue be closed?

@mversic
Copy link
Contributor

mversic commented Aug 23, 2024

No, it's still valid

@dima74 dima74 linked a pull request Aug 27, 2024 that will close this issue
3 tasks
@dima74 dima74 self-assigned this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants