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

Add protobuf representation for transaction proposals #891

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Aug 8, 2023

In order to make complete information about proposed transactions readily available to wallets, it's necessary to provide a mechanism to pass a TransactionProposal across the FFI. This PR adds a Protobuf representation for proposed transactions and implements conversions back and forth between the protobuf representation and the native type.

Closes #1093.

@nuttycom nuttycom requested review from str4d and daira August 8, 2023 18:30
@nuttycom nuttycom marked this pull request as draft August 11, 2023 13:52
@nuttycom nuttycom marked this pull request as ready for review August 11, 2023 15:10
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

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

Comparison is base (2d6a02e) 70.14% compared to head (c5f48f5) 69.79%.
Report is 3 commits behind head on main.

Files Patch % Lines
zcash_client_backend/src/proto.rs 65.83% 55 Missing ⚠️
zcash_client_backend/src/zip321.rs 12.82% 34 Missing ⚠️
...ent_backend/src/data_api/wallet/input_selection.rs 53.19% 22 Missing ⚠️
zcash_client_backend/src/proto/proposal.rs 0.00% 13 Missing ⚠️
zcash_client_sqlite/src/wallet.rs 64.51% 11 Missing ⚠️
zcash_client_backend/src/data_api.rs 0.00% 4 Missing ⚠️
zcash_client_sqlite/src/lib.rs 50.00% 2 Missing ⚠️
zcash_client_backend/src/data_api/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   70.14%   69.79%   -0.35%     
==========================================
  Files         141      142       +1     
  Lines       13674    13959     +285     
==========================================
+ Hits         9591     9743     +152     
- Misses       4083     4216     +133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nuttycom nuttycom force-pushed the proposal-ffi branch 3 times, most recently from 65f4204 to 4c501da Compare August 16, 2023 17:39
@pacu
Copy link
Contributor

pacu commented Aug 18, 2023

Folks, do you have an expected deadline for accepting comments on this?
Just prioritizing here 😅

@nuttycom
Copy link
Contributor Author

nuttycom commented Aug 18, 2023 via email

Copy link
Contributor

@pacu pacu left a comment

Choose a reason for hiding this comment

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

Left non-blocking comments on PR.

  1. possible feature request: create proposals from ZIP-321 string-serialized requests
  2. Evaluate ways to make it hard (or better impossible) for Proposal's payment request property to be out of sync of a Proposal instance.
  3. Consider adding "privacy policy" to the transaction proposal similar as the ones documented in z_sendmany and z_shieldcoinbase on Zcashd.

zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved
zcash_client_backend/CHANGELOG.md Outdated Show resolved Hide resolved
zcash_client_backend/proto/proposal.proto Show resolved Hide resolved
zcash_client_backend/proto/proposal.proto Outdated Show resolved Hide resolved
zcash_client_backend/src/data_api/wallet.rs Outdated Show resolved Hide resolved
zcash_client_backend/src/proto.rs Show resolved Hide resolved
@nuttycom nuttycom marked this pull request as draft September 7, 2023 17:50
@nuttycom nuttycom force-pushed the proposal-ffi branch 3 times, most recently from a2d416a to ad08dec Compare September 12, 2023 23:13
@daira daira added this to the SDK 2.0 Post-Release Cleanup milestone Sep 13, 2023
@nuttycom nuttycom marked this pull request as ready for review September 13, 2023 20:10
str4d
str4d previously approved these changes Nov 14, 2023
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 7aab6fd with a few nits.

daira
daira previously approved these changes Nov 15, 2023
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 typo fix suggestions and a non-blocking comment.

Co-authored-by: Daira Emma Hopwood <daira@jacaranda.org>
Co-authored-by: str4d <thestr4d@gmail.com>
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.

Re-utACK c5f48f5

@nuttycom nuttycom merged commit 236cd56 into zcash:main Nov 15, 2023
14 of 16 checks passed
@nuttycom nuttycom deleted the proposal-ffi branch November 15, 2023 22:47
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.

Post-hoc ACK

str4d added a commit that referenced this pull request Nov 21, 2023
A comment change to the source file was made in #891
before merging, but the generated file was not updated to match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add protobuf representation for transaction proposals
6 participants