Closed tcharding closed 1 year ago
Review checkpoint e2527dc19c3e8e9a4cd678a955fc5b161e175c87
Force push has all your suggestions except https://github.com/rust-bitcoin/rust-bech32/pull/113#discussion_r1245231234. There was quite a bit of wild hacking today so I'd say we need to iterate again :)
The HrpstringChars::size_hint
function was incorrect. I noticed also that the hrp
module has size_hint/len incorrect because of my previous lack of iterator knowledge, will fix as a separate PR.
You can use Option::zip
to make the hrp_max
/chk_max
stuff a little bit cleaner.
Another iteration on the API, this time I've added functions on the iterators that can be adapted instead of the extension trait. Also there are multiple transitions through all the iterators, see example in iter
module.
And used Option::zip
as suggested. Thanks, I've never used that before :)
To make review easier I opened #116 and then I attempted to extensively describe what this PR changes.
@clarkmoody this PR is pretty heavy and very invasive, if you have any concerns at all please voice them. Also if I can do anything else to make your review easier please say.
Holy molly, this is really hard to get a nice clean iterator API that conforms to the bips, is general purpose, but is sane. Converting this to draft while I keep playing with it.
@apoelstra I think I may not have it exactly clear what we are trying to achieve, I was unable yesterday to answer this question clearly to myself:
What is the aim of the iter API?
Sadly, the more I read bips 173/350 the more I am convinced it is not possible to write a correct, clean, general crate that can be used without reading those bips many times. I am still attempting however to do so and hopefully when we add a segwit
module bitcoin users will get just such an API.
In #117 I've tried to make it easy and obvious how to use the API. Ergonomic for typical (from my limited understanding of typical) usecases but still fully general (I am having trouble working out all the general usecases).
@tcharding the goal for the iter API, in my mind, is to make it easy to
Fe32
s corresponding to the above (basically, same thing except the HRP is split into fes and nothing is converted to chars) that you can feed into a checksum engine for verification.I don't really care about other usecases. So, as restrictive as we can make the API while still enabling the above, that's good by me.
(Just the last patch, the rest is in #116)
Currently there are a bunch of types and traits in
lib.rs
for converting to/from base32 (incl. doing so without allocating).Observe that instead of custom types we can use iterator adapters to achieve this. Some benefits are:
checksum
module (i.e, better separation of concerns)Add two new sub-modules to the
primitives
module:iter
: Implement various adaptor iterators for allocationless checksumming etc.hrpstring
: Add theParsed
struct that can be used for parsing bech32 address strings.Remove everything that is now not needed from
lib.rs
while making an effort not to change the API too much i.e., keep the current public functions more or less as they are, with the following notable exceptions:encode_to_fmt
no longer returns a resultencode
now returns afmt::Error
as theResult
error variant (instead ofError
).decode
returns aParsed
instead of an HRP and vector of u5s, to get the HRP/u5's one can use thehrp()
andfe32_iter()
functions onParsed
.decode_without_checksum
return value changed as perdecode
.Error
type is totally re-written.All the recently added but as-yet-unreleased
arrayvec
stuff which was added to improve alloc free encoding/decoding is removed.Complete list of the public types/functions removed in this patch
(This functionality is now provided by
primitives
.)traits:
WriteBase32
WriteBase256
(unreleased)FromBase32
ToBase32
Base32Len
CheckBase32
types (and their trait implementations):
Bech32Writer
ComboError
(unreleased)Case
(exist still but is private)functions:
convert_bits
convert_bits_in
(unreleased)encode_to_fmt_anycase
(unreleased)decode_lowercase
(unreleased)