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

Gracefully handle when given an Orchard-only UA #1009

Merged
merged 15 commits into from
Oct 13, 2023
Merged

Gracefully handle when given an Orchard-only UA #1009

merged 15 commits into from
Oct 13, 2023

Conversation

tw0po1nt
Copy link
Contributor

@tw0po1nt tw0po1nt commented Oct 7, 2023 •
edited
Loading

Closes #1008

@tw0po1nt tw0po1nt force-pushed the fix_txn_builder_panic branch from ac7af8b to a9d18ec Compare October 7, 2023 20:37
nuttycom
nuttycom reviewed Oct 8, 2023
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

This is looking really good! There are a couple of documentation changes/additions that will be needed, and a couple of small stylistic suggestions.

zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/wallet.rs Show resolved Hide resolved
zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/error.rs Outdated Show resolved Hide resolved
tw0po1nt
tw0po1nt commented Oct 9, 2023
View reviewed changes
Copy link
Contributor Author

@tw0po1nt tw0po1nt left a comment

Choose a reason for hiding this comment

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

Self-reviewed latest changes

nuttycom
nuttycom previously approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK modulo the changelog formatting and rustfmt failures.

@tw0po1nt @daira
Update CHANGELOG.md
4bbe658 
Co-authored-by: Daira Emma Hopwood <[email protected]>
@tw0po1nt @daira
Update wallet.rs
13a2d5d 
Co-authored-by: Daira Emma Hopwood <[email protected]>
daira
daira requested changes Oct 9, 2023
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.

This comment is blocking: https://github.com/zcash/librustzcash/pull/1009/files#r1350429559

@codecov
Copy link

codecov bot commented Oct 9, 2023 •
edited
Loading

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Files Coverage Δ
zcash_client_backend/src/data_api/error.rs 8.62% <0.00%> (-0.16%) ⬇️
zcash_client_sqlite/src/error.rs 0.00% <0.00%> (ø)
zcash_client_sqlite/src/wallet.rs 82.48% <0.00%> (-0.37%) ⬇️
zcash_client_backend/src/data_api.rs 37.50% <0.00%> (-0.92%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 84.12% <0.00%> (-3.12%) ⬇️

📢 Thoughts on this report? Let us know!.

Matthew Watt and others added 4 commits October 10, 2023 06:35
@tw0po1nt tw0po1nt marked this pull request as ready for review October 10, 2023 12:47
@tw0po1nt
Copy link
Contributor Author

@daira @nuttycom Fixed the merge conflicts, this should be ready for re-review

nuttycom
nuttycom approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK a788cc4

@nuttycom nuttycom requested a review from daira October 12, 2023 20:25
daira
daira approved these changes Oct 12, 2023
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

@nuttycom nuttycom merged commit 29c676f into zcash:main Oct 13, 2023
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

@nuttycom nuttycom nuttycom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transaction builder should fail gracefully when given an Orchard-only UA
3 participants
@tw0po1nt @nuttycom @daira