Re-write crate level API by tcharding · Pull Request #128 · rust-bitcoin/rust-bech32

tcharding · 2023-09-04T15:26:45Z

Now includes updates to fuzzing and is on top of #130 (bug fix).

Now we have all the new primitives pieces in place we can re-write the public API to take advantage of them - WIN!

Close: #126

tcharding · 2023-09-04T15:27:49Z

This PR more or less follows the proposal in #126 but I'm not auto-closing it because it does not include the suggested checksum example.

If you want to define your own checksum you should use the Checksum trait. We should provide an example, maybe of the BIP-93 checksum.

FTR, I did not work off of the proposal except for the docs stuff, I just hacked up what I thought was a useful API (on the plane). I rekon this needs a bit of thought to see if we like it or not? Once we are more-or-less happy with the API I'll go over the docs again and add examples etc. as needed.

Note the segwit API and main API are not exactly uniform because I managed to get on the plane with a three month old git index so did not have the segwit module to look at. I've left it as I wrote it because I would like input from reviewers please as to what is best instead of me just picking the one I think is best.

tcharding · 2023-09-04T15:38:45Z

The commented out example function is code for some module level docs examples.

tcharding · 2023-09-11T15:09:48Z

This might need splitting up into separate PRs now because I've made some changes to the HRP constants to make them more readable.

I've audited the two top level APIs (lib.rs and segwit.rs) and I'm happy they are uniform. Open to naming improvements still of course.

I've improved docs and examples.

Note please the new crate::hrp module that is soley to improve import ergonomics because I noticed we use hrp::BC any time we use the segwit API but one used to have to reach into primitives to get it.

Now includes Codex32 checksum.

I changed the embedded tests so that one uses bech32 and one uses bech32m

tcharding · 2023-09-11T16:46:32Z

Ok, if CI goes green this is ready for review please @apoelstra, I edited the "what I did" post above, so in case you read a stale version please not this now adds Codex32. If you are more-or-less happy with the current state I can spilt the PR up. I'll ping you @clarkmoody when we are at a "final" re-viewable state so you don't have to follow along if you don't want to.

tcharding · 2023-09-11T17:00:30Z

FYI to check the embedded tests locally one must pin with cargo update -p proc-macro2 --precise 1.0.60.

I updated to latest nightly while debugging the cargo check command, you might need to do that also.

tcharding · 2023-09-11T17:06:23Z

In case its useful to you @clarkmoody I'm building the docs locally to review them using

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links

apoelstra · 2023-09-12T16:55:50Z

embedded/no-allocator/src/main.rs

-    let hrp = Hrp::parse("bech32").unwrap();
-
-    bech32::encode_to_fmt_anycase(&mut encoded, hrp, &base32, Variant::Bech32).unwrap().unwrap();
+    bech32::encode_to_fmt::<Bech32, ArrayString<30>>(&mut encoded, hrp, &data)


In 4da0a7b:

Could use <Bech32, _> rather than repeating ArrayString<30>.

Done, thanks.

apoelstra · 2023-09-12T17:01:15Z

src/primitives/mod.rs

+/// The codex32 checksum algorithm, defined in [BIP-93].
+/// [BIP-350]: <https://github.com/bitcoin/bips/blob/master/bip-0093.mediawiki>
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub enum Codex32 {}


In 4da0a7b:

This can live in a doccomment or something but we probably shouldn't export it from this library.

apoelstra · 2023-09-14T14:09:42Z

In 4da0a7b:

I'm a bit confused why decode and decode_bech32m/decode_bech32.
Could you add an analogous encode_to_write / decode_to_read pair to encode_to_fmt? Then no-alloc users could encode the string either to an io::Write or fmt::Write. (Annoyingly this will require a std gate, but that's ok, it would still be useful I think.)

Otherwise this looks great!

clarkmoody

Nice top-level examples in the doc code!

clarkmoody · 2023-09-14T17:20:07Z

src/lib.rs

+/// Re-exports the hrp types from [`primitives::hrp`] to make importing ergonomic for the top level APIs.
+pub mod hrp;


Love the re-exports to the top-level hrp module.

Do the doc comments need to point out that these are re-exports?

There is a stupid compiler warning now if re-exports don't have doccomments. Since they appear in the docs under Re-exports with a link to their definition, this isn't really sensible, so we've been putting "placeholder comments" like this in rust-bitcoin.

Commented on this below.

clarkmoody · 2023-09-14T17:25:57Z

src/primitives/mod.rs

 pub enum Bech32m {}

+/// The codex32 checksum algorithm, defined in [BIP-93].
+/// [BIP-350]: <https://github.com/bitcoin/bips/blob/master/bip-0093.mediawiki>


Should this be [BIP-93] here?

Thanks, odd that -D rustdoc::broken-intra-doc-links didn't catch that. I've moved the codex32 stuff to a rustdoc comment and remove the link altogether.

clarkmoody · 2023-09-14T17:29:17Z

src/lib.rs

+pub mod hrp;
+/// All the primitive types and functionality used in encoding and decoding.
 pub mod primitives;
+/// API for encoding and decoding segwit addresses.


In the docs, this comment collides with the existing module-level docs in segwit.rs, giving us:

API for encoding and decoding segwit addresses. Segregated Witness API - enables typical usage for encoding and decoding segwit addresses.

We should remove one of the comments to avoid the redundant sentence.

Ha, I saw this once and didn't fix it. Thanks for taking the time to build the docs and check them!

I've removed the docs from rust-bech32/src/hrp.rs and clippy seems to be ok with it, although I'm unsure why. Like @apoelstra said above we saw a new lint trigger for this situation in rust-bitcoin, will see how it goes in CI.

tcharding · 2023-09-15T15:37:11Z

In 4da0a7b:

I'm a bit confused why decode and decode_bech32m/decode_bech32.

Ok, I removed them and added more docs about decoding with the checksum algorithm to the decode docs.

Could you add an analogous encode_to_write / decode_to_read pair to encode_to_fmt? Then no-alloc users could encode the string either to an io::Write or fmt::Write. (Annoyingly this will require a std gate, but that's ok, it would still be useful I think.)

Done!

Otherwise this looks great!

Thanks man, this force push was the result of a pretty wild session, you can wait to review until I've had a chance to re-look at it with fresh eyes to catch my own mistakes if you like.

apoelstra · 2023-09-19T01:48:58Z

src/segwit.rs

+) -> Result<(Hrp, Fe32, Vec<u8>), DecodeFromReaderError> {
+    let mut s = String::new();
+    r.read_to_string(&mut s)?;
+    Ok(decode(&s)?)


In 68c4dfd:

I'll go ahead and ACK this since it's not API-visible, but the string allocation (and UTF-8 checks and everything that comes with it) are all unnecessary here. You should be able to just use the bytes() iterator on the Read, adapt it wiht char::from to get characters, then use the iterator API.

I like it! I'll have go.

I had a bit of a play with this, unless I'm missing something:

The iterator API (you mean iter.rs, right?) is for encoding not decoding

The API in decode.rs could be changed to enable decoding from a reader but its non-trivial because all the decoding types (eg, UncheckedHrpstring) have lifetimes that expect a string somewhere to reference.

We could read into a buffer because a bech32 string is max 90 chars, then parse it out however we like.

Or am I totally missing something?

No, you may be correct. Several iterations ago I had an idea for decoding through the iterator API and I guess that got dropped.

I think it's fine to just drop this function then. I don't have a clear usecase for it and it doesn't even work with nostd.

Removed, both in lib.rs and segwit.rs

apoelstra

ACK 68c4dfd

ea644f0 decode: Add unit test for parsing segwit address (Tobin C. Harding) 5d6e5c3 decode: Add empty data check (Tobin C. Harding) Pull request description: Noob bug here, I accessed an array without first checking that it was non-empty :( Adds unit test as a separate patch, re-arrange to verify the bug. Discovered while fuzzing #128. ACKs for top commit: apoelstra: ACK ea644f0 Tree-SHA512: dda0b91f8e2f94ba47b0d6f386e1edc17938bf4c71851ec387f922389ba384693655ed863e98bb95f60970bbf36844cc6c58b654c666b6ab1116cd6dea970ba2

apoelstra

ACK 9a90d64

Now we have all the new `primitives` pieces in place we can re-write the public API to take advantage of them - WIN!

tcharding · 2023-09-22T23:12:52Z

Rebaes only, no other changes.

apoelstra

ACK 79918f6

clarkmoody

ACK 79918f6

In rust-bitcoin#128 we dropped the benchmarks. They can be restore with only minor tweaks, so do so.

e0b95f1 benchmarks: add benchmarks for checksum param generation (Andrew Poelstra) cce8a7a benchmarks: restore benchmarks (Andrew Poelstra) 72421dc clippy: remove a couple redundant test imports (Andrew Poelstra) Pull request description: In #128 we dropped the benchmarks. They can be restored with only minor tweaks, so do so. ACKs for top commit: clarkmoody: ACK e0b95f1 Tree-SHA512: 6a23ba658ac8536ef6feb9400feddfbc4a561ff5ef1e10eb90bab6cf637c631f73901e7d8a738f8bad3389340fb532d74fe6129e2eb3d115ad01436c2ee298e5

tcharding force-pushed the 09-01-api branch 2 times, most recently from 92f020a to c5bcd1e Compare September 4, 2023 15:37

tcharding force-pushed the 09-01-api branch 4 times, most recently from b6c4fda to d2623ec Compare September 11, 2023 15:03

tcharding force-pushed the 09-01-api branch 3 times, most recently from c036c7f to d45d759 Compare September 11, 2023 16:42

tcharding force-pushed the 09-01-api branch from d45d759 to 692bac7 Compare September 11, 2023 16:58

tcharding force-pushed the 09-01-api branch from 692bac7 to 404a09d Compare September 11, 2023 17:03

tcharding force-pushed the 09-01-api branch from 404a09d to 4da0a7b Compare September 11, 2023 17:16

apoelstra reviewed Sep 12, 2023

View reviewed changes

clarkmoody reviewed Sep 14, 2023

View reviewed changes

tcharding force-pushed the 09-01-api branch from 4da0a7b to 86c31d4 Compare September 15, 2023 15:34

tcharding force-pushed the 09-01-api branch 3 times, most recently from ecf941f to 68c4dfd Compare September 18, 2023 23:58

tcharding mentioned this pull request Sep 19, 2023

Fix index out of bounds bug #130

Merged

apoelstra reviewed Sep 19, 2023

View reviewed changes

apoelstra approved these changes Sep 19, 2023

View reviewed changes

tcharding force-pushed the 09-01-api branch from 68c4dfd to 9a90d64 Compare September 22, 2023 00:01

tcharding mentioned this pull request Sep 22, 2023

Polishing/stabilization roadmap? #60

Closed

apoelstra approved these changes Sep 22, 2023

View reviewed changes

Re-write crate level API

79918f6

Now we have all the new `primitives` pieces in place we can re-write the public API to take advantage of them - WIN!

tcharding force-pushed the 09-01-api branch from 9a90d64 to 79918f6 Compare September 22, 2023 23:12

apoelstra approved these changes Sep 23, 2023

View reviewed changes

clarkmoody approved these changes Sep 29, 2023

View reviewed changes

apoelstra merged commit f982bb9 into rust-bitcoin:master Sep 29, 2023

apoelstra deleted the 09-01-api branch September 29, 2023 17:17

apoelstra added a commit to apoelstra/rust-bech32 that referenced this pull request Mar 29, 2024

benchmarks: restore benchmarks

82a577d

In rust-bitcoin#128 we dropped the benchmarks. They can be restore with only minor tweaks, so do so.

apoelstra mentioned this pull request Mar 29, 2024

benchmarks: restore benchmarks #182

Merged

apoelstra added a commit to apoelstra/rust-bech32 that referenced this pull request Jul 6, 2024

benchmarks: restore benchmarks

cce8a7a

In rust-bitcoin#128 we dropped the benchmarks. They can be restore with only minor tweaks, so do so.

		/// Re-exports the hrp types from [`primitives::hrp`] to make importing ergonomic for the top level APIs.
		pub mod hrp;

Conversation

tcharding commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 4, 2023

Uh oh!

tcharding commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 11, 2023

Uh oh!

tcharding commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 11, 2023

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!

apoelstra commented Sep 14, 2023

Uh oh!

clarkmoody 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!

tcharding commented Sep 15, 2023

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!

apoelstra 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 Sep 22, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

clarkmoody left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Sep 4, 2023 •

edited

Loading

tcharding commented Sep 4, 2023 •

edited

Loading

tcharding commented Sep 11, 2023 •

edited

Loading

tcharding commented Sep 11, 2023 •

edited

Loading