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

ChainMetadata uses non standard uint64 #5797

Closed
SWvheerden opened this issue Sep 22, 2023 · 2 comments · Fixed by #5833
Closed

ChainMetadata uses non standard uint64 #5797

SWvheerden opened this issue Sep 22, 2023 · 2 comments · Fixed by #5833
Assignees
@brianp
Labels
release-blocker Something that needs to be fixed before a release can be made

Comments

@SWvheerden
Copy link
Collaborator

Should be swapped to uint64
@SWvheerden SWvheerden added the release-blocker Something that needs to be fixed before a release can be made label Sep 22, 2023
@brianp
Copy link
Contributor

brianp commented Oct 4, 2023

@SWvheerden Can you elaborate a little here? I took a look at ChainMetadata and nothing about its u64's jumped out. Do you mean the u128 for accumulated_difficulty?

@brianp
Copy link
Contributor

brianp commented Oct 4, 2023

@SWvheerden pointed out it's the proto file using google.protobuf.UInt64Value for height_of_longest_chain and timestamp in https://github.com/tari-project/tari/blob/development/base_layer/core/src/base_node/proto/chain_metadata.proto#L10-L24

@brianp brianp self-assigned this Oct 4, 2023
@brianp brianp mentioned this issue Oct 5, 2023
Merged
4 tasks
SWvheerden pushed a commit that referenced this issue Oct 23, 2023
@brianp
refactor(proto)!: remove proto timestamp wrapper types (#5833)
43b994e 
Description
---
We were making use of 3 proto wrapper types. These types deserialize
into option types and in some cases are not what we want.

This PR will update 2 of these types, while saving the third for an
additional PR.

Motivation and Context
---
Use standard rust types, instead of library specific types.

Closes: #5797

How Has This Been Tested?
---
CI

What process can a PR reviewer use to test or verify this change?
---
Pay close attention to changes around timestamps. Could get tricky.
Notably, we were using `Some(0)` in some tests for timestamps. This was
a valid timestamp for us, because it wasn't `None`. Now we're treating
`0` as an invalid timestamp. So any test with `None` became `0` but
tests with `Some(0)` are presumably tests that _should_ have had valid
timestamps but this was the easiest minimal value that was accepted.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [ ] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [x] Other - Please specify

Messages between new and old nodes are likely to break.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brianp brianp

Labels
release-blocker Something that needs to be fixed before a release can be made
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(proto)!: remove proto timestamp wrapper types brianp/tari
2 participants
@brianp @SWvheerden