search

rust-bitcoin / rust-bech32

Bech32 format encoding and decoding
MIT License
93 stars 49 forks source link

decode: Add accessors #167

Closed tcharding closed 8 months ago

tcharding commented 8 months ago

This is a sexy little PR right here.

Add an ascii accessor method to the UncheckedHrpstring and CheckedHrpstring types.

Fix: #160

apoelstra commented 8 months ago

2be20a198f0680213168e43d9afffb4ec134fb3e looks great! ascii is a far better name than data for the field.

For the method, can you rename it to data_part or data_part_ascii (or some variant of this). Just ascii implies that we might return the whole string, but we're skipping the HRP and separator.

As a separate request, can you add a commit with method to UncheckedHrpstring which returns the witness version and drops it from the ascii field? This is ad-hoc and segwit-specific but it's easy to implement. If you don't want to do that, maybe we could add to_raw_parts and from_raw_parts methods which let the user manually "pop apart" the HrpString and modify it. Or we could do both :).

tcharding commented 8 months ago

Ok, I spent waaay too long on this task. Please see the new ridiculously long function names. Since this is pretty niche usage I think they are warranted - do not ask me how many times I re-wrote this solution.

As a separate request, can you add a commit with method to UncheckedHrpstring which returns the witness version and drops it from the ascii field? This is ad-hoc and segwit-specific but it's easy to implement. If you don't want to do that, maybe we could add to_raw_parts and from_raw_parts methods which let the user manually "pop apart" the HrpString and modify it. Or we could do both :).

Please see the last patch, I know its not exactly what you specify but is it enough to serve your needs?

apoelstra commented 8 months ago

@tcharding no, 0ee999d6cbfb6d4c301ef88fd0f502a0632449e5 is not sufficient. I can implement a pure accessor myself using the data_part_ascii accessor. What I can't do is strip the witness version from the data part. (Though I suppose I don't really need to do this, since I can just pull the data part out and modify it myself.) So as written, this provides an obscure accessor which isn't sufficient for the one use-case it might be used for.

On reflection I wouldn't mind just dropping this commit if there's no simple way to do it.

apoelstra commented 8 months ago

Nice, thanks! This looks great.