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.

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

Bech32m Support #50

Merged
merged 5 commits into from Feb 14, 2021
Merged

Bech32m Support #50

merged 5 commits into from Feb 14, 2021

Conversation

@clarkmoody
Copy link
Member

@clarkmoody clarkmoody commented Feb 12, 2021

Adds support for BIP-0350, Bech32m encoding. Bumps version to 0.8.0.

Breaking Changes

  • decode returns the variant as third value upon success. This will be used downstream when validating SegWit addresses against the allowed encoding variant.
  • encode, encode_to_fmt, and Bech32Writer::new require variant parameters. When creating SegWit addresses, the proper variant will be selected based on the script version.

Closes #48

clarkmoody added 4 commits Feb 12, 2021
This breaks the API by requiring a `variant` parameter in a few places,
which instructs the library whether to use Bech32 or Bech32m.

Test cases from BIP-0350
Copy link
Contributor

@dr-orlovsky dr-orlovsky left a comment

Have not reviewed against the spec; few suggestion after the first code read

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {
/// The original Bech32 described in [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki)
Bech32,

This comment has been minimized.

@dr-orlovsky

dr-orlovsky Feb 12, 2021
Contributor

can we pls have the actual constants assigned as the values here? When we will not need BECH32_CONST, BECH32M_CONST, and can simply use Variant::Bech32 as u32 instead of constant() function call (and avoid one call)

This comment has been minimized.

@sgeisler

sgeisler Feb 12, 2021
Member

I'm fine with @clarkmoody's version, it's more "standard" imo, do you feel strongly about this?

This comment has been minimized.

@clarkmoody

clarkmoody Feb 12, 2021
Author Member

I'm not exactly sure what you mean @dr-orlovsky. Could you post a code example?

Performance-wise, the compiler will inline these simple methods for sure.

@@ -400,22 +410,52 @@ pub fn encode_to_fmt<T: AsRef<[u5]>>(
}
}

/// Used for encode/decode operations for the two variants of Bech32
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {

This comment has been minimized.

@dr-orlovsky

dr-orlovsky Feb 12, 2021
Contributor

I suggest also to add repr(u32) here

This comment has been minimized.

@clarkmoody

clarkmoody Feb 14, 2021
Author Member

What does this do for us?

This comment has been minimized.

@sgeisler

sgeisler Feb 14, 2021
Member

It's necessary to implement the Variant::Bech32 as u32 API afaik (making the enum castable to its internal representation). I don't really like it because it isn't too well-known this is possible and also seems a bit like abusing this feature here for probably non-existent performance gains.

This comment has been minimized.

@clarkmoody

clarkmoody Feb 14, 2021
Author Member

Makes sense. I'll leave this out for now, and we can circle back later if the need arises.

Copy link
Member

@sgeisler sgeisler left a comment

I like it, very elegant 😃

ACK 411d8c4

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {
/// The original Bech32 described in [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki)
Bech32,

This comment has been minimized.

@sgeisler

sgeisler Feb 12, 2021
Member

I'm fine with @clarkmoody's version, it's more "standard" imo, do you feel strongly about this?

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Feb 12, 2021

Btw, do you know what's wrong with CI? It just doesn't run that one job, maybe we should switch to GH actions too …

@clarkmoody
Copy link
Member Author

@clarkmoody clarkmoody commented Feb 12, 2021

Btw, do you know what's wrong with CI? It just doesn't run that one job, maybe we should switch to GH actions too …

I think it's a modern version of Clippy failing on our 1.22-compatible code.

I agree that we should switch to GH actions. I'll work on that. Do you think it's also time to raise our version to 1.29 to match the rust-bitcoin project?

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Feb 12, 2021

Do you think it's also time to raise our version to 1.29 to match the rust-bitcoin project?

Oh lol, I was totally unaware that we didn't do that yet, otoh no surprise given this crate's stability. If it helps sure! Idk how much better clippy can cope with 1.29 though, it's also ancient.

@clarkmoody clarkmoody force-pushed the bech32m branch 3 times, most recently from 1ee3bc7 to 335f6bb Feb 13, 2021
Copy link
Member

@sgeisler sgeisler left a comment

The new tests are a bit redundant (why check when you are going to build and test anyway?) and I'd prefer to only test if there's not a good argument to be made for checking. Later we will probably expand the matrix to test features like no-std and this could cost us time when we hit the maximum parallel worker limit.

@clarkmoody clarkmoody force-pushed the bech32m branch from 335f6bb to 388d270 Feb 14, 2021
@clarkmoody
Copy link
Member Author

@clarkmoody clarkmoody commented Feb 14, 2021

The new tests are a bit redundant (why check when you are going to build and test anyway?) and I'd prefer to only test if there's not a good argument to be made for checking. Later we will probably expand the matrix to test features like no-std and this could cost us time when we hit the maximum parallel worker limit.

Agree. Removed the check job, as that functionality is duplicated by test.

Copy link
Member

@sgeisler sgeisler left a comment

ACK 335f6bb

@clarkmoody clarkmoody merged commit 6c87d18 into master Feb 14, 2021
5 checks passed
5 checks passed
Test Suite (1.29.0)
Details
Test Suite (stable)
Details
Test Suite (nightly)
Details
Rustfmt (stable)
Details
Clippy (stable)
Details
@clarkmoody clarkmoody deleted the bech32m branch Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants