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

Allocationless ecoding #35

Merged
merged 7 commits into from Jul 17, 2019

Conversation

Projects
None yet
3 participants
@sgeisler
Copy link
Member

commented Jun 16, 2019

Builds on top of and closes #34 and adds a Bech32Writer struct that allows allocationless encoding to an underlying std::fmt::Writer.

  • fuzz [u8] -> [u5] encoding is covered by current (adapted) fuzz tests
  • remove redundant encoding code
  • fix rust 1.22 compatibility

@sgeisler sgeisler force-pushed the sgeisler:no-struct branch 2 times, most recently from 2178f52 to 45eff37 Jun 17, 2019

@sgeisler sgeisler force-pushed the sgeisler:no-struct branch 4 times, most recently from cd7c81a to e606a36 Jun 27, 2019

@sgeisler sgeisler changed the title WIP: allocationless ecoding Allocationless ecoding Jun 27, 2019

@sgeisler sgeisler force-pushed the sgeisler:no-struct branch from e606a36 to 038647e Jun 27, 2019

@stevenroose

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

I tested this briefly with rust-bitcoin/rust-bitcoin#255 and it seems to work.

@stevenroose

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

There is something not working in the Rust 1.14 test. Could you look at that? Then we can get this merged and rust-bitcoin/rust-bitcoin#255 as well.

@sgeisler

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

@stevenroose

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

Oh yeah no sure 1.22 is fine, it's rust-bitcoin's version. Ah I clicked the old Travis report, my bad. Sure then it's good to go IMO. I gave my approval, but I'm not an official contributor to this repo. Can you merge without ACKs?

@sgeisler sgeisler requested a review from clarkmoody Jul 7, 2019

@sgeisler

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

No, I'd also like to hear @clarkmoody's opinion first.

@clarkmoody

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Looking over this now. Quite a lot of changes 😄

Can we update the examples in the README to incorporate these changes?

@clarkmoody
Copy link
Member

left a comment

This is the cleanest version of the interface yet. Well done guys!

Additional items:

  • Update README example (or axe it in favor of simpler explanation without code to maintain -- we have the doc code)
  • Bump version (unless there are more changes to get in to 0.7.0)
src/lib.rs Outdated
}

// Split at separator and check for two pieces
let (raw_hrp, raw_data) = match s.rfind("1") {

This comment has been minimized.

Copy link
@clarkmoody

clarkmoody Jul 8, 2019

Member

Use SEP here instead of "1"?

This comment has been minimized.

Copy link
@sgeisler

sgeisler Jul 16, 2019

Author Member

Fixed. It should be more efficient too (ignoring compiler optimization) since it's now matching against a char instead of a &str.

src/lib.rs Outdated
'g','f','2','t','v','d','w','0',
's','3','j','n','5','4','k','h',
'c','e','6','m','u','a','7','l'
'q', 'p', 'z', 'r', 'y', '9', 'x', '8', 'g', 'f', '2', 't', 'v', 'd', 'w', '0', 's', '3', 'j',

This comment has been minimized.

Copy link
@clarkmoody

clarkmoody Jul 8, 2019

Member

It looks like these have been auto-formatted, which makes them a little harder to visually verify against the BIP-0173 spec and other implementations. Not a deal-breaker, since errors will show up in test, but if there's a clean way to skip auto-formatting / linting for these few lines (and for CHARSET_REV below), I would prefer that.

This comment has been minimized.

Copy link
@sgeisler

sgeisler Jul 16, 2019

Author Member

I reverted the changes and added an ignore attribute.

This comment has been minimized.

Copy link
@clarkmoody

clarkmoody Jul 16, 2019

Member

Looks like tests are failing from that new attribute.

This comment has been minimized.

Copy link
@sgeisler

sgeisler Jul 16, 2019

Author Member

Yeah, I'm already searching for the right attribute to deactivate that, I used it recently but can't remember, I guess it's #[allow(unknown_attributes)].

This comment has been minimized.

Copy link
@sgeisler

sgeisler Jul 16, 2019

Author Member

I ended up just removing the attributes. unknown_attributes requires a feature only available on nightly.

@clarkmoody clarkmoody referenced this pull request Jul 10, 2019

Open

add signet support #291

@sgeisler sgeisler force-pushed the sgeisler:no-struct branch 2 times, most recently from 893093a to 9a3bb0a Jul 16, 2019

@sgeisler sgeisler force-pushed the sgeisler:no-struct branch from 9a3bb0a to 768d292 Jul 16, 2019

@sgeisler sgeisler requested a review from dongcarl Jul 17, 2019

@sgeisler sgeisler requested a review from TheBlueMatt Jul 17, 2019

@clarkmoody
Copy link
Member

left a comment

LGTM. Would love to hear comments from others. We can bump version separately, if necessary.

@sgeisler sgeisler merged commit ad1d07d into rust-bitcoin:master Jul 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sgeisler sgeisler deleted the sgeisler:no-struct branch Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.