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.

Sign up for GitHub

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

Jump to bottom

Address to optimized QR string #581

Merged
merged 8 commits into from May 6, 2021
Merged

Address to optimized QR string #581

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b9d5200
Access Display and Formatter with fmt:: like in other places
RCasatta Mar 22, 2021
d18554e
Address to string conversion optimized for qr codes
RCasatta Mar 22, 2021
cac3f46
improve to_qr_string doc
RCasatta Mar 23, 2021
104836a
implements alternate formatting for address
RCasatta Apr 7, 2021
85ae82f
use the char trick to avoid allocation
RCasatta Apr 7, 2021
bc406bf
Use &mut dyn fmt::Write instead of bool
Kixunil Apr 9, 2021
3158ced
document alternate formatting
RCasatta Apr 9, 2021
0a91496
rename to_qr_string into to_qr_uri returning also the schema
RCasatta Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 45 additions & 5 deletions src/util/address.rs
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@
//! let address = Address::p2pkh(&public_key, Network::Bitcoin);
//! ```

use std::fmt::{self, Display, Formatter};
use std::fmt;
use std::str::FromStr;
use std::error;

Expand Down Expand Up @@ -352,10 +352,19 @@ impl Address {
pub fn script_pubkey(&self) -> script::Script {
self.payload.script_pubkey()
}

/// Creates a string optimized to be encoded in QR codes, meaning it becomes uppercase if bech32.
/// Quoting BIP 173 "inside QR codes uppercase SHOULD be used, as those permit the use of
/// alphanumeric mode, which is 45% more compact than the normal byte mode."
/// Even inside Bitcoin URI may be more efficient to use the uppercase address since in QR codes
/// encoding modes can be mixed as needed within a QR symbol.
pub fn to_qr_string(&self) -> String {
sgeisler marked this conversation as resolved.
Show resolved Hide resolved
format!("{:#}", self)
}
}

impl Display for Address {
fn fmt(&self, fmt: &mut Formatter) -> fmt::Result {
impl fmt::Display for Address {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
match self.payload {
Payload::PubkeyHash(ref hash) => {
let mut prefixed = [0; 21];
Expand All @@ -381,17 +390,34 @@ impl Display for Address {
} => {
let hrp = match self.network {
Network::Bitcoin => "bc",
Network::Testnet | Network::Signet => "tb",
Network::Testnet | Network::Signet => "tb",
Network::Regtest => "bcrt",
};
sgeisler marked this conversation as resolved.
Show resolved Hide resolved
let mut bech32_writer = bech32::Bech32Writer::new(hrp, fmt)?;
let is_alternate = fmt.alternate();
let mut opt_up_writer = OptionallyUpperWriter(fmt, is_alternate);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe dispatching here instead of using bool is a bit faster, but probably not something to worry about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Idk if that's what you mean (I think you mean putting the if here), but also why supply a boolean derived from fmt when you supply fmt?

Copy link
Collaborator Author

@RCasatta RCasatta Apr 8, 2021 •
edited

Choose a reason for hiding this comment

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

but also why supply a boolean derived from fmt when you supply fmt?

the naive reason is that fmt is taken as something that impl fmt::Write, not as Formatter, so it's lacking alternate()

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sgeisler yes, I meant putting the if there as was written before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure how to put the if as was before since something like this won't work

let bech32_writer = if fmt.alternate() {
  let upper =  UpperWriter(fmt);
  bech32::Bech32Writer::new(hrp, &mut upper)?;
} else {
  bech32::Bech32Writer::new(hrp, fmt)?;
}

and other solutions I can think about are too verbose

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, wasn't your initial concern essentially avoiding a single function call in some cases? Always using dynamic dispatch sounds even worse (but maybe the compiler will figure it out? but the same cou). I still see that this is more elegant than having a boolen argument.

Also: we are horribly over-engineering this PR 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

I only pointed out that evaluating if multiple times is not great without realizing it can't be fixed without adding bloat. Thus dynamic dispatch is probably preferable. Manually-implemented dynamic dispatch has no benefit over compiler-implemented dynamic dispatch and the latter has less boilerplate, thus should be preferred.

Yeah, we're overengineering, I personally enjoy it from time to time. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this example #581 (comment) there is also a lifetime issue not only a dispatching issue...

I am not sure how to go ahead:

  1. "like it is now" -> use OptionallyUpperWriter with the bool
  2. "verbose" -> use UpperWriter and if fmt.is_alternate() with the last two lines present in both of the if branch
  3. "very verbose" -> use UpperWriter and OwnedWriter
  4. "inner"
let mut upper_writer = UpperWriter(fmt);
let writer = if upper_writer.0.alternate() {
    &mut upper_writer as &mut dyn fmt::Write
} else {
    &mut upper_writer.0 as &mut dyn fmt::Write
};
  1. I miss what you are suggesting, please provide some code

I personally still prefer 1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, didn't realize the lifetime issue before. There's still a useful Rust trick to fix it, made a PR against your branch. :)

Copy link
Collaborator Author

@RCasatta RCasatta Apr 9, 2021 •
edited

Choose a reason for hiding this comment

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

very nice! merged

let mut bech32_writer = bech32::Bech32Writer::new(hrp, &mut opt_up_writer)?;
bech32::WriteBase32::write_u5(&mut bech32_writer, ver)?;
bech32::ToBase32::write_base32(&prog, &mut bech32_writer)
}
}
}
}

struct OptionallyUpperWriter<W: fmt::Write>(W, bool);

impl<W: fmt::Write> fmt::Write for OptionallyUpperWriter<W> {
fn write_str(&mut self, s: &str) -> fmt::Result {
if self.1 {
for c in s.chars() {
self.0.write_char(c.to_ascii_uppercase())?;
}
} else {
self.0.write_str(s)?;
}
Ok(())
}
}

/// Extract the bech32 prefix.
/// Returns the same slice when no prefix is found.
fn find_bech32_prefix(bech32: &str) -> &str {
Expand Down Expand Up @@ -743,4 +769,18 @@ mod tests {
hex_script!("001454d26dddb59c7073c6a197946ea1841951fa7a74")
);
}

#[test]
fn test_qr_string() {
for el in ["132F25rTsvBdp9JzLLBHP5mvGY66i1xdiM", "33iFwdLuRpW1uK1RTRqsoi8rR4NpDzk66k"].iter() {
let addr = Address::from_str(el).unwrap();
assert_eq!(addr.to_qr_string(), *el);
}

for el in ["bcrt1q2nfxmhd4n3c8834pj72xagvyr9gl57n5r94fsl", "bc1qwqdg6squsna38e46795at95yu9atm8azzmyvckulcc7kytlcckxswvvzej"].iter() {
let addr = Address::from_str(el).unwrap();
assert_eq!(addr.to_qr_string(), el.to_ascii_uppercase());
}
}

}