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

Report proposal errors earlier and more precisely where possible #1441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daira
Copy link
Contributor

@daira daira commented Jul 2, 2024

This follow-up to #1257 is not essential, but it improves the error reporting by:

  • reporting some errors when a proposal is constructed rather than when it is used to create transactions;
  • reporting more precise errors for unsupported operations.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 32.60870% with 31 lines in your changes missing coverage. Please review.

Project coverage is 60.67%. Comparing base (f35a894) to head (46f1e7b).

Files Patch % Lines
zcash_client_backend/src/proposal.rs 31.25% 22 Missing ⚠️
zcash_client_backend/src/data_api/error.rs 25.00% 6 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs 0.00% 2 Missing ⚠️
zcash_client_backend/src/fees.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1441      +/-   ##
==========================================
- Coverage   60.73%   60.67%   -0.06%     
==========================================
  Files         140      140              
  Lines       16467    16475       +8     
==========================================
- Hits        10001     9996       -5     
- Misses       6466     6479      +13     

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

@daira daira marked this pull request as draft July 7, 2024 02:53
@daira
Copy link
Contributor Author

daira commented Jul 7, 2024

This is not up-to-date with #1257. I will rebase it when that merges.

This is now rebased onto main.

@daira daira force-pushed the refactor-errors branch from 5669bb7 to 03fc0a8 Compare July 27, 2024 23:28
@daira daira marked this pull request as ready for review July 27, 2024 23:29
@daira daira force-pushed the refactor-errors branch from 03fc0a8 to 46f1e7b Compare July 27, 2024 23:40
@daira daira changed the title Report proposal errors earlier where possible Report proposal errors earlier and more precisely where possible Jul 27, 2024
@daira daira requested review from str4d and nuttycom and removed request for str4d July 30, 2024 16:55
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.

Flushing comments.

/// This is indicative of a programming error; a transaction proposal that presumes
/// support for the specified pool was decoded or executed using an application that
/// does not provide such support.
ProposalNotSupported(ProposalError),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that having two different variants that wrap ProposalError is a good idea; it suggests that the entire state space of ProposalError is reachable by multiple paths, but the comment here suggests something different, and that only a subset of ProposalError is reachable via the path that produces ProposalNotSupported.

Comment on lines +198 to +199
ProposalError::SpendsPaymentFromUnsupportedPool(_)
| ProposalError::PaysUnsupportedPoolRecipient(_) => Error::ProposalNotSupported(e),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these variants really be a part of ProposalError?

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.

2 participants