Improve parsing of `Denomination` string by tcharding · Pull Request #731 · rust-bitcoin/rust-bitcoin

tcharding · 2021-12-01T02:25:50Z

There is no reason to force users to use a particular format or case for Denomination strings. Users may wish to write any of the following and all seem reasonable

100 sats
100 sat
100 SAT

The same goes for various other Denominations.

Patch 1 enables usage of "sats", "sat", "bit", "bits"
Patch 2 enables usage of various lower/uper case formatting

Fixes: #729

dr-orlovsky · 2021-12-01T08:24:52Z

From one side, this breaks Display/FromStr round-trip. From the other, it was already broken and this will be a required companion for the proposal by @Kixunil on new formatting for Amount. So concept ACK

Kixunil · 2021-12-01T09:46:10Z

Concept ACK from me as well. Although I wouldn't say it's required.

Kixunil

I'm "requesting changes" but if anyone disagrees, please explain.

Kixunil · 2021-12-01T10:06:14Z

src/util/amount.rs

I think ifs with eq_ignore_ascii_case would be better - more concise and obvious what it does. It would also allow ridiculous strings like 10 sAtS but I don't see harm in it.

Maybe we could forbid Msat (any sats starting with capital M) as suspicious - possibly meaning "mega sats"

Done. In case you are wondering what all the force pushing is about. I removed the suffix 's' but didn't test locally with 1.29 so it was no good :(

The code is pretty ugly.

Kixunil · 2021-12-02T09:44:20Z

src/util/amount.rs

Maybe we could have PossiblyConfusingDenomination variant or something similar.

I'm still unsure if we should allow MSAT, MSat and similar.

Can do, will re-spin later today.

Kixunil · 2021-12-02T09:47:29Z

src/util/amount.rs

How about making s mut, adding:

if s.ends_with('s') || s.ends_with('S') { s = &s[..(s.len() - 1)]; }

And matching only singular from here?

Ha ha, I wanted to do this as well but remember we cannot index strings in Rust because of utf8. I didn't want to do an allocation so no String methods either. Then I tried strip_suffix, not noticing it was Rust 1.45. Hence I reverted to the ugly if clauses again.

we cannot index strings in Rust because of utf8

We can, we just have to do it by bytes and given that s and S are ASCII, it's exactly 1 byte, so indexing is fine (and if we're lucky the compiler will even optimize-out bounds check).

Good points!

Please re-review, its looking hot now :)

Kixunil

I believe instead of having match at the end it'd be more robust to check s.starts_with('M') and reject it at the beginning. If you want to avoid returning PossiblyConfusingDenomination for things like Mfoo, make a helper function that doesn't check M and call it also from error handling with &s[1..]

Kixunil · 2021-12-03T07:46:20Z

src/util/amount.rs

Note, s will be stripped here.

Kixunil · 2021-12-03T07:46:55Z

src/util/amount.rs

Should we allow mSat? I don't see any harm in it.

Kixunil · 2021-12-03T07:47:40Z

src/util/amount.rs

Actually, Mbtc could mean "mega btc"

ha, I missed another one. Thanks for paying close attention to this one @Kixunil!

tcharding · 2021-12-10T00:16:17Z

I've had another got at this one, note the addition of a match statement at the start of the function now.

Kixunil · 2021-12-10T09:52:30Z

Could you please explain why the change you made is better than my suggestion? Am I missing something?

RCasatta · 2021-12-10T10:07:16Z

There is no reason to force users to use a particular format or case for Denomination strings.

I think the problem between using "m" as milli and "M" as mega is a reason strong enough to keep casing.

Kixunil · 2021-12-10T10:40:09Z

@RCasatta thoughts on why just banning M is not sufficient?

RCasatta · 2021-12-10T10:53:40Z

@RCasatta thoughts on why just banning M is not sufficient?

I guess it would be fine if mega isn't allowed and is the only ambiguity, I also checked in https://github.com/lightning/bolts/blob/master/11-payment-encoding.md and it's not a valid multiplier

tcharding · 2021-12-13T20:54:13Z

Could you please explain why the change you made is better than my suggestion? Am I missing something?

I missed your suggestion originally, sorry (I'm on holidays ATM just hacking with half my brain :) New and improved version force pushed, incl. helper function as suggested.

tcharding · 2021-12-13T20:57:55Z

I created an issue thanks to your findings in BOLT 11 @RCasatta: #741

Kixunil

Nearly perfect. I just realized the error message would be confusing for the users and I really, really want to have the message clear. (Clear error messages are like 80% of debugging success.) Sorry about one more round.

src/util/amount.rs

Kixunil · 2021-12-14T05:59:02Z

API breaking because of the new variant.

There is no reason to force users to use one particular form when providing a denomination string. We can be liberal in what we accept with no loss of clarity. Allow `Denomination` strings to use a variety of forms, in particular lower case and uppercase. Note, we explicitly disallow various forms of `Msat` because it is ambiguous whether this means milli or mega sats. Co-developed-by: Martin Habovštiak <martin.habovstiak@gmail.com>

tcharding · 2021-12-22T02:53:29Z

@Kixunil, I added a Co-developed-by: tag to the second commit of this PR, you've been a driving force of this work.

Kixunil

ACK f690b8e

Thanks!

apoelstra

ACK f690b8e

lol @ the "this denomination is uncommon" megabitcoin error

tcharding · 2021-12-23T01:38:30Z

lol @ the "this denomination is uncommon" megabitcoin error

How many Mega-bitcoin do you have?

apoelstra · 2021-12-23T14:58:38Z

I think this is good to merge BTW -- though I think we should wait for 24h after my ACK, as I've gotten complaints before when I show up and ack/merge tons of PRs without warning after weeks of silence.

RCasatta · 2021-12-27T09:17:54Z

More than 24 hours passed... Merged

Kixunil requested changes Dec 1, 2021

View reviewed changes

tcharding force-pushed the denomination-string branch 3 times, most recently from ce8fc98 to 3f9daff Compare December 2, 2021 05:12

Kixunil reviewed Dec 2, 2021

View reviewed changes

Kixunil added the waiting for author This can only progress if the author responds to a request. label Dec 2, 2021

tcharding force-pushed the denomination-string branch 2 times, most recently from 7835d98 to 8625501 Compare December 3, 2021 04:02

Kixunil requested changes Dec 3, 2021

View reviewed changes

Add missing white space character

628168e

tcharding force-pushed the denomination-string branch from 8625501 to 6ee3fff Compare December 10, 2021 00:15

tcharding requested a review from Kixunil December 10, 2021 00:16

tcharding force-pushed the denomination-string branch 2 times, most recently from 11d2abc to 42351f0 Compare December 10, 2021 02:14

tcharding force-pushed the denomination-string branch from 42351f0 to 10010fd Compare December 13, 2021 20:53

Kixunil added enhancement and removed waiting for author This can only progress if the author responds to a request. labels Dec 13, 2021

Kixunil reviewed Dec 13, 2021

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

Kixunil added waiting for author This can only progress if the author responds to a request. API break This PR requires a version bump for the next release labels Dec 13, 2021

tcharding force-pushed the denomination-string branch from 10010fd to f690b8e Compare December 22, 2021 02:52

tcharding requested a review from Kixunil December 22, 2021 02:54

Kixunil approved these changes Dec 22, 2021

View reviewed changes

apoelstra approved these changes Dec 23, 2021

View reviewed changes

RCasatta merged commit 9e1f256 into rust-bitcoin:master Dec 27, 2021

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

tcharding deleted the denomination-string branch March 15, 2022 04:00

Conversation

tcharding commented Dec 1, 2021

Uh oh!

dr-orlovsky commented Dec 1, 2021

Uh oh!

Kixunil commented Dec 1, 2021

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Dec 10, 2021

Uh oh!

Kixunil commented Dec 10, 2021

Uh oh!

RCasatta commented Dec 10, 2021

Uh oh!

Kixunil commented Dec 10, 2021

Uh oh!

RCasatta commented Dec 10, 2021

Uh oh!

tcharding commented Dec 13, 2021

Uh oh!

tcharding commented Dec 13, 2021

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil commented Dec 14, 2021

Uh oh!

tcharding commented Dec 22, 2021

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Dec 23, 2021

Uh oh!

RCasatta commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tcharding commented Dec 23, 2021 •

edited

Loading