Remove Decimal and replace strason with serde_json #250
Conversation
There was a problem hiding this comment.
Hey, thank you for tackling this issue (thought about doing this since we switched to 1.21/22). This should fix our random fuzz-test stack overflows. I noticed that it wasn't compiling half-way through and will do a full review once it does. In the meantime I'll mark the PR as WIP.
|
Yeah I forgot that. It is compiling for me, I considered it quite finished.
Just somehow Travis doesn't seem to pull in dev-dependencies. I added
serde_json to dev-dependencies (and as an optional regular dependency), so
the test run should have it. I'll look into it later.
…On Tue, Mar 26, 2019, 19:49 Sebastian Geisler ***@***.*** wrote:
***@***.**** requested changes on this pull request.
Hey, thank you for tackling this issue (thought about doing this since we
switched to 1.21/22). This should fix our random fuzz-test stack
overflows. I noticed that it wasn't compiling half-way through and will do
a full review once it does. In the meantime I'll mark the PR as WIP.
------------------------------
In src/util/decimal.rs
<#250 (comment)>
:
> impl FromStr for Decimal {
type Err = ParseDecimalError;
/// Parses a `Decimal` from the given amount string.
fn from_str(s: &str) -> Result<Self, Self::Err> {
- Json::from_str(s)?
- .num()
- .ok_or(ParseDecimalError::NotANumber)
- .and_then(Decimal::parse_decimal)
+ Decimal::parse_decimal(&serde_json::Number::from_str(s)?.to_string())
+ //serde_json::Number::from_str(s)
I guess you just forgot that in here?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#250 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA0F3C10gkYInmzEfGHBPB7pNrnxpkFMks5vannSgaJpZM4cL7pK>
.
|
15bdc37 to
fe67ce8
Compare
src/util/psbt/mod.rs
Outdated
| @@ -116,7 +116,7 @@ impl<D: Decoder> Decodable<D> for PartiallySignedTransaction { | |||
| return Err(Error::InvalidMagic.into()); | |||
| } | |||
|
|
|||
| if 0xff_u8 != Decodable::consensus_decode(d)? { | |||
| if 0xff != u8::consensus_decode(d)? { | |||
There was a problem hiding this comment.
Just wanted to mention that this somehow broke for me. It couldn't infer type, even though the 0xff_u8 is very explicitly typed. Same goes for the misc.rs change. Well, never hurts to be more specific I guess.
|
Would be nice to have this merged together with #252. |
|
Maybe I don't understand something and it's not an issue, but just thinking aloud: I don't understand how the "arbitrary_precision" feature on |
|
@dpc @apoelstra any other objection to this? |
|
Cool. Let's get Frustrating that we can't feature-gate |
|
So you prefer to have Amount come first? Because currently it builds on this PR. I could change that though. |
|
8000
Can this be rebased now that #270 is in? |
|
I also removed Decimal, is that ok? |
| @@ -805,15 +805,14 @@ mod test { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[cfg(all(feature = "serde", feature = "strason"))] | |||
| #[cfg(feature = "serde")] | |||
There was a problem hiding this comment.
Needs to be feature = "serde_json", no?
There was a problem hiding this comment.
No, you're testing the serde stuff in the main code and serde_json is a dev-dependency that is always present for testing.
(I was mistaken in the Cargo file, though, I forgot to move serde_json to dev-dependencies. We don't rely on it for anything else than tests.)
There was a problem hiding this comment.
@apoelstra do we need to coordinate that with other breaking changes? Since the Amount PR was merged this seems to be ready for review/merging.
|
No coordination, we'll put it all in the next major release. |
I'm also wondering if it makes sense to have Decimal in here at all.. Was that intended to be used for difficulty numbers or so? In rust-bitcoincore-rpc we're using
BigUintfrom the num-bigint crate for that.