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.

Sign up for GitHub

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

Jump to bottom

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
@dima74
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:

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

This was referenced Jun 22, 2024
Closed
Closed
@hollermay

This comment was marked as resolved.

Sign in to view
@hollermay hollermay mentioned this issue Jun 27, 2024
Closed
@dima74
Copy link
Contributor

dima74 commented Aug 23, 2024

u64 is used for timestamps after #4841:
https://github.com/hyperledger/iroha/blob/a27bc2eaa7e3cfab6c35cae3b2bcace63a3fee2b/data_model/src/events/time.rs#L100-L105
@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
Open
3 tasks
@dima74 dima74 self-assigned this Aug 28, 2024
@mversic mversic added this to Iroha Oct 10, 2024
@mversic mversic moved this to Work in Progress in Iroha Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dima74 dima74

Labels
Enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: Use SystemTime for timestamps dima74/iroha
5 participants
@dima74 @nxsaken @0x009922 @mversic @hollermay