Skip to content

Add concrete error types for add_spend and add_output#366

Merged
nuttycom merged 8 commits intozcash:mainfrom
zingolabs:add_concrete_errors_for_spend_and_output_construction
Dec 8, 2022
Merged

Add concrete error types for add_spend and add_output#366
nuttycom merged 8 commits intozcash:mainfrom
zingolabs:add_concrete_errors_for_spend_and_output_construction

Conversation

@AloeareV
Copy link
Contributor

@AloeareV AloeareV commented Dec 1, 2022

No description provided.

@AloeareV
Copy link
Contributor Author

AloeareV commented Dec 1, 2022

orchard::builder::Error is now BuildError to disambiguate from SpendError, if breaking changes are an issue that could be type-aliased to Error.

Copy link

@zancas zancas left a comment

Choose a reason for hiding this comment

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

It seems to me that the changelog is out of sync with the actual mod paths, and that needs to be fixed.

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Base: 84.13% // Head: 84.15% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (9807e32) compared to base (d05b6ce).
Patch coverage: 35.71% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
+ Coverage   84.13%   84.15%   +0.02%     
==========================================
  Files          32       32              
  Lines        2672     2670       -2     
==========================================
- Hits         2248     2247       -1     
+ Misses        424      423       -1     
Impacted Files Coverage Δ
src/builder.rs 68.77% <35.71%> (+0.45%) ⬆️
src/note.rs 88.57% <0.00%> (-0.17%) ⬇️
src/circuit.rs 92.51% <0.00%> (-0.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Fixes required for the changelog, otherwise this LGTM.

This is an API-breaking change, so it will require a minor-version release.

CHANGELOG.md Outdated
Comment on lines +29 to +37
- `orchard::builder`:
- `SpendError`
- `OutputsDisabled`

### Changed
- Migrated to `zcash_note_encryption 0.2`.
- `orchard::builder::Builder::{add_spend, add_output}` now use
concrete error types instead of `&'static str`s.
- `orchard::builder::Error` is now `BuildError` to differentiate from
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be in the Unreleased section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

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

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 modulo suggestions from me and @str4d.

AloeareV and others added 3 commits December 8, 2022 16:35
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: str4d <thestr4d@gmail.com>
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

@nuttycom nuttycom merged commit 06cea3f into zcash:main Dec 8, 2022
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.

6 participants