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

refactor(proto)!: remove proto timestamp wrapper types #5833

Merged
merged 3 commits into from Oct 23, 2023

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Oct 5, 2023

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.

Breaking Changes

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

Messages between new and old nodes are likely to break.

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Test Results (CI)

1 232 tests   1 232 ✔️  13m 19s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit e199048.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 5, 2023
@brianp brianp changed the title reafctor(proto)!: remove proto wrapper types refactor(proto)!: remove proto wrapper types Oct 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Test Results (Integration tests)

  2 files  11 suites   12m 56s ⏱️
33 tests 32 ✔️ 0 💤 1
34 runs  33 ✔️ 0 💤 1

For more details on these failures, see this check.

Results for commit e199048.

♻️ This comment has been updated with latest results.

@brianp brianp added the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Oct 5, 2023
@brianp
Copy link
Contributor Author

brianp commented Oct 5, 2023

I marked this controversial. Obviously, there are some tests to fix up, but having done the work I'm not sure I like the PR. It feels like we've taken a strongly typed paradigm such as having a new block tip, or not, and allowed the chance for defaults like a 0 filled vec take it's place, creating a vaguely strongly typed paradigm.

It's not an all or nothing though in the case of timestamps a forced u64 can often make sense. What we're losing is having null types in the database and instead, as a default will always land us in 1970.

In the case of one function we expected a timestamp relatively close to the current date, and skipped it if None. If we default to 0 we would end up querying everything in the table. As the default would be 1970 and we would query everything since 1970. So there's always the chance a default date can land us somewhere we don't want.

Alas, these are just thoughts. I'll continue on, and see if anyone else holds an opinion.

Edit: I forgot to add a recommendation. I think getting rid of the protobuf wrappers is still a good idea. But it may be beneficial if we manually map the proto message from its value default or otherwise, into an Option again. This would leave almost all the code exactly the same other than some minor adjustments in the serializers/deserializers.

@brianp brianp requested review from sdbondi and SWvheerden and removed request for sdbondi October 5, 2023 21:03
@brianp brianp force-pushed the refactor-proto-types branch 2 times, most recently from f2ea0d9 to 5ec7e51 Compare October 9, 2023 12:57
@brianp
Copy link
Contributor Author

brianp commented Oct 9, 2023

Update on my previous comment: More or less what I had written in the recommendation was what some others had in mind. This PR has been updated to reflect that. So the changes are only in the proto types, and serialization/deserialization where needed. Tari types shouldn't need to change here.

@brianp brianp marked this pull request as ready for review October 9, 2023 18:32
@brianp brianp removed the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Oct 9, 2023
@brianp brianp changed the title refactor(proto)!: remove proto wrapper types refactor(proto)!: remove proto timestamp wrapper types Oct 10, 2023
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

Looks good, just the one we need accept and cannot error on. I think its find that we just accept a 0 zero

base_layer/core/src/base_node/proto/chain_metadata.rs Outdated Show resolved Hide resolved
SWvheerden pushed a commit that referenced this pull request Oct 12, 2023
Description
---
This PR will remove the google proto bytes wrapper in favour of using a
standard vec.

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

Related to #5833

How Has This Been Tested?
---
CI

What process can a PR reviewer use to test or verify this change?
---
Previously used test values included `None`, `Some(vec![])` and
`Some(FixedHash::zero())`. This PR removes the option wrappers and as a
result the previous `Some(vec![])` values will be `vec![]` and
considered the equivalent to `None`.

<!-- 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.
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 23, 2023
@SWvheerden SWvheerden merged commit 43b994e into tari-project:development Oct 23, 2023
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChainMetadata uses non standard uint64
3 participants