Skip to content

Unified container parsing enforces typecode order. #474

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
Merged
nuttycom merged 6 commits into from
Jan 4, 2022
Merged

Unified container parsing enforces typecode order. #474

nuttycom merged 6 commits into from
Jan 4, 2022

Conversation

@nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Jan 4, 2022

No description provided.

@nuttycom nuttycom force-pushed the enforce_typecode_order branch from d5dbda3 to f17ab99 Compare January 4, 2022 01:44
@nuttycom nuttycom requested a review from str4d January 4, 2022 01:59
@nuttycom nuttycom force-pushed the enforce_typecode_order branch from f17ab99 to 89cd7ec Compare January 4, 2022 01:59
@nuttycom nuttycom added this to the Core Sprint 2021-52 milestone Jan 4, 2022
@codecov
Copy link

codecov bot commented Jan 4, 2022 •
edited
Loading

Codecov Report

Merging #474 (8d34e62) into master (5622b06) will increase coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   50.30%   50.35%   +0.04%     
==========================================
  Files          88       88              
  Lines        7941     7963      +22     
==========================================
+ Hits         3995     4010      +15     
- Misses       3946     3953       +7
Impacted Files Coverage Δ
components/zcash_address/src/test_vectors.rs 100.00% <ø> (ø)
components/zcash_address/src/kind/unified.rs 39.37% <45.45%> (+3.16%) ⬆️
...mponents/zcash_address/src/kind/unified/address.rs 88.23% <100.00%> (-0.66%) ⬇️
components/zcash_address/src/kind/unified/fvk.rs 89.47% <100.00%> (-2.20%) ⬇️
components/zcash_address/src/kind/unified/ivk.rs 90.47% <100.00%> (-2.03%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 27.39% <0.00%> (-1.37%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5622b06...8d34e62. Read the comment docs.

@nuttycom nuttycom force-pushed the enforce_typecode_order branch from 89cd7ec to 6bd8225 Compare January 4, 2022 02:08
@nuttycom
Copy link
Contributor Author

nuttycom commented Jan 4, 2022

UA test vectors need to be updated for the tests here to pass.

@nuttycom nuttycom force-pushed the enforce_typecode_order branch from 6bd8225 to 33880c0 Compare January 4, 2022 05:16
@daira daira self-requested a review January 4, 2022 11:26
daira
daira reviewed Jan 4, 2022
View reviewed changes
daira
daira reviewed Jan 4, 2022
View reviewed changes
daira
daira reviewed Jan 4, 2022
View reviewed changes
components/zcash_address/src/kind/unified/address.rs Outdated
Copy link
Contributor

@daira daira Jan 4, 2022

Choose a reason for hiding this comment

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

Should this be cloning typecodes before mutating it? It doesn't seem common for arb_* functions to mutate their arguments, and I'm not sure whether there are any special requirements to make shrinking work.

Copy link
Contributor Author

@nuttycom nuttycom Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since this takes ownership, there's no need to clone it, right? The shrinking question is interesting, but I think the answer to that is that this should really take a set of typecodes, not a vec since the uniqueness property needs to hold anyway.

daira
daira reviewed Jan 4, 2022
View reviewed changes
daira
daira reviewed Jan 4, 2022
View reviewed changes
daira
daira approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with suggestions.

daira
daira reviewed Jan 4, 2022
View reviewed changes
@nuttycom nuttycom force-pushed the enforce_typecode_order branch from 55b7fb4 to 5682ffe Compare January 4, 2022 14:53
@nuttycom
Copy link
Contributor Author

nuttycom commented Jan 4, 2022

One style issue that I note this PR raises: we now have two canonical orderings for typecodes (preference order and encoding order) and as such it feels like the Ord instance that prioritizes preference order should instead just be a function that can be used with sort_by.

@nuttycom nuttycom force-pushed the enforce_typecode_order branch from a23e3f8 to a9c9fbf Compare January 4, 2022 16:06
@nuttycom nuttycom mentioned this pull request Jan 4, 2022
Merged
nuttycom and others added 5 commits January 4, 2022 11:48
@nuttycom @daira
Makes unified container parsing enforce typecode order.
4c4c0b1 
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
@nuttycom @daira
Apply suggestions from code review
e413f12 
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
@nuttycom
Remove Ord instances for sealed items.
2fa73ed 
There are two canonical orderings for sealed items: preference
order and encoding order. Removing the `Ord` instances means
that a user can't accidentally choose the wrong ordering;
these orderings are replaced by explicit `preference_order`
and `encoding_order` comparison functions.
@nuttycom
Update test vectors to respect item ordering.
6065c07
@nuttycom
Pin pprof version
e3c67ff
@nuttycom nuttycom force-pushed the enforce_typecode_order branch from c31ff8e to e3c67ff Compare January 4, 2022 18:49
str4d
str4d approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK. I verified that the updated test vectors here match the ones generated by zcash/zcash-test-vectors@61894e7.

@nuttycom nuttycom merged commit 7801ddd into zcash:master Jan 4, 2022
@nuttycom nuttycom deleted the enforce_typecode_order branch January 4, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daira daira daira approved these changes

@str4d str4d str4d approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Core Sprint 2021-52

Development

Successfully merging this pull request may close these issues.

3 participants

@nuttycom @daira @str4d