Nullus157 / bs58-rs

Another Rust Base58 codec implementation
Apache License 2.0
75 stars 24 forks source link

decode: Add const-compatible decoder #116

Closed joncinque closed 5 months ago

joncinque commented 6 months ago

Problem

The base58 decoding functions in this crate work extremely well, but they are not available in const contexts.

In case you'd like to hear about a specific use-case, in Solana programs, we define the canonical program-id at the top-level of a crate to be consumed by others, ie:

https://github.com/solana-labs/solana-program-library/blob/7e022ac1b2ac527d6607712521685e64d0ff50f8/token/program/src/lib.rs#L85

Currently, that's done with a string literal, so we can decode it in a macro using the normal bs58::decode. It only works with string literals, however, and not &str or any other macro that returns a string.

Summary of changes

To allow users to decode any string to base58 in a const context, this PR adds in a new separate decoder which is only meant for const usage.

It's unfortunately much more limited than the normal decoder due to the limitations on const in Rust, so many features needed to be completely dropped for it:

Please let me know if you have any questions or comments! Everything is very well documented in this crate, so I aimed to follow the existing conventions.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 83.07692% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 76.42%. Comparing base (be42edf) to head (b6ad26a). Report is 1 commits behind head on main.

Files Patch % Lines
src/decode.rs 82.81% 11 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #116 +/- ## ========================================== + Coverage 72.91% 76.42% +3.50% ========================================== Files 4 4 Lines 288 352 +64 ========================================== + Hits 210 269 +59 - Misses 78 83 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Nemo157 commented 6 months ago

Thanks for the PR. I do think this is worth supporting till we get const-traits, since those seem quite far away. I have a couple ideas around the API that I want to test out, I should have time to do that in the next few days.

joncinque commented 6 months ago

Thanks for your response, sounds great! Let me know if I can help in any way.

Nemo157 commented 5 months ago

I've pushed the API idea I had to https://github.com/Nullus157/bs58-rs/compare/main...Nemo157:bs58-rs:const-integrated, essentially making as much as possible of the decode API const fn, then just introducing new finishing methods onto it to actually do the decode. If you think that looks good I can push it into this PR and get it merged.

joncinque commented 5 months ago

This looks great to me! It's definitely clearer to have it all in one place and calling out the const-compatible functions at the decoder level.

And it works perfectly for my use-case, I tested your branch locally. Feel free to push the changes however you think is best, and thanks for doing the work!