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

Overloaded encode error #329

wants to merge 7 commits into
base: master


Copy link

commented Sep 9, 2019

We're using the encode::Error in places where we probably shouldn't. I know it's a silly breaking change, so I'd be ok with having it stale here for a while until we're going to release a breaking version.

It's hard to get all encode errors out of the psbt module because of a circular dependency otherwise. (There is an encode::Error::Psbt variant.)

@stevenroose stevenroose force-pushed the stevenroose:overloaded-encode-error branch from 1ff45de to 66141c6 Sep 9, 2019

stevenroose added 7 commits Sep 9, 2019
Remove encode::Error::ByteOrder
Functions from the byteorder crate only return downstream io errors on
io calls.

This comment has been minimized.

Copy link

commented Sep 9, 2019

Codecov Report

Merging #329 into master will increase coverage by 0.45%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #329      +/-   ##
+ Coverage   81.98%   82.43%   +0.45%     
  Files          38       38              
  Lines        6998     7327     +329     
+ Hits         5737     6040     +303     
- Misses       1261     1287      +26
Impacted Files Coverage Δ
src/util/ 97.64% <ø> (+0.5%) ⬆️
src/util/psbt/ 86.06% <0%> (ø) ⬆️
src/blockdata/ 94.28% <100%> (+0.79%) ⬆️
src/blockdata/ 79.67% <100%> (-1.85%) ⬇️
src/util/ 100% <100%> (ø) ⬆️
src/consensus/ 82.17% <37.5%> (+5.4%) ⬆️
src/util/ 82.82% <59.09%> (+1.61%) ⬆️
... and 2 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 de9ccde...66141c6. Read the comment docs.


This comment has been minimized.

Copy link

commented Sep 9, 2019

I think #284 is going to result in a bunch of "silly API changes" so one more is no big deal.


This comment has been minimized.

Copy link
Collaborator Author

commented Sep 13, 2019

@apoelstra is that an ACK? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
3 participants
You can’t perform that action at this time.