Skip to content

util::amount: Add a FromStr-based serde deserializer#509

Closed
stevenroose wants to merge 1 commit intorust-bitcoin:masterfrom
stevenroose:amount-serde-parse
Closed

util::amount: Add a FromStr-based serde deserializer#509
stevenroose wants to merge 1 commit intorust-bitcoin:masterfrom
stevenroose:amount-serde-parse

Conversation

@stevenroose
Copy link
Copy Markdown
Collaborator

Related to #501.

apoelstra
apoelstra previously approved these changes Nov 5, 2020
@stevenroose stevenroose changed the title Add a FromStr-based serde deserializer util::amount: Add a FromStr-based serde deserializer Dec 21, 2020
@apoelstra
Copy link
Copy Markdown
Member

Doesn't compile with 1.29.0

cargo +1.29.0  build  '--no-default-features' '--features=rand use-serde base64' # cargo 1.29.0 (524a578d7 2018-08-05)
error[E0658]: access to extern crates through prelude is experimental (see issue #44660)
   --> src/util/amount.rs:938:27
    |
938 |         impl<'de, ValueT> serde::de::Visitor<'de> for Visitor<ValueT>
    |                           ^^^^^

error[E0658]: access to extern crates through prelude is experimental (see issue #44660)
   --> src/util/amount.rs:949:29
    |
949 |             fn visit_str<E: serde::de::Error>(self, v: &str) -> Result<Self::Value, E> {
    |                             ^^^^^

error[E0658]: access to extern crates through prelude is experimental (see issue #44660)
   --> src/util/amount.rs:953:38
    |
953 |             fn visit_borrowed_str<E: serde::de::Error>(self, v: &'de str) -> Result<Self::Value, E> {
    |                                      ^^^^^

error[E0658]: access to extern crates through prelude is experimental (see issue #44660)
   --> src/util/amount.rs:957:32
    |
957 |             fn visit_string<E: serde::de::Error>(self, v: String) -> Result<Self::Value, E> {
    |                                ^^^^^

etc

Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK, but with fixes for 1.29 compilation errors

@dr-orlovsky dr-orlovsky added this to the 0.26.1 milestone Jan 30, 2021
@dr-orlovsky dr-orlovsky added the minor API Change This PR should get a minor version bump label Apr 28, 2021
@stevenroose
Copy link
Copy Markdown
Collaborator Author

Fixed those MSRV errors and rebased.

sgeisler
sgeisler previously approved these changes Apr 29, 2021
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

utACK dc54199c46169329500a27e43f8ac769ebfd172f

I think we should revisit the FromStr impl at some point and make it work properly (e.g. without spaces, like c-lightning would require it) at least if we still want to include and not deprecate it (I'm not sure if really that useful tbh, see my comment in #593).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding this bound is a breaking change, no?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This trait is a "private" one and should not be used outside of the crate. However, this is not enforced (yet), so if somebody is using it against the recommendation, this may break their code.

@dr-orlovsky dr-orlovsky modified the milestones: 0.26.1, 0.27.0 Jun 8, 2021
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Needs rebase and replacement of all std:: to core::

@stevenroose stevenroose force-pushed the amount-serde-parse branch 2 times, most recently from d00b252 to 61ce753 Compare November 13, 2021 21:14
@stevenroose
Copy link
Copy Markdown
Collaborator Author

Fixed and rebased.

Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Nov 29, 2021

For what its worth, I had a play around with this and it looks good to me. I played some more because I was trying to understand why this PR only does one way (deserialization) and not both, I was unable to work this out, am I missing something @stevenroose?.

EDIT: Is it so it works with no_std?

Also I was playing with a solution to #729. I pushed a draft PR (#730) that does the same as this PR but uses the same pattern as as_btc and as_sat to add a module as_string (EDIT: But this doesn't work with no_std.)

@sanket1729
Copy link
Copy Markdown
Member

sanket1729 commented Jan 7, 2022

Needs rebase.

Hi @stevenroose , we are gearing up for a major release soon. Otherwise, this might have to wait for 0.29

@sanket1729 sanket1729 modified the milestones: 0.28.0, 0.29.0 Jan 13, 2022
@tcharding
Copy link
Copy Markdown
Member

Hey mate, this PR is currently marked as part of the 0.29 milestone, would you like to get this functionality in before the 0.29 release?

@tcharding
Copy link
Copy Markdown
Member

No response from @stevenroose so I'm going to remove the milestone from this one.

@tcharding tcharding removed this from the 0.29.0 milestone Jul 18, 2022
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jan 27, 2024

This looks stale and I'd rather add mod as_str. We could have also With<Strategy> instead to handle different denominations but that seems too much.

Also is this actually needed in bitcoincore-rpc or anywhere else?

@Kixunil Kixunil added the C-units PRs modifying the units crate label Jan 27, 2024
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Feb 19, 2024

Closing as stale. Reopen when ready.

@Kixunil Kixunil closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-units PRs modifying the units crate minor API Change This PR should get a minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants