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

decode: Add const-compatible decoder #116

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Conversation

joncinque
Copy link
Contributor

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:

  • no allocations are allowed, so no vec, small vec, tiny vec
  • it's impossible to use mutable references in general, so the decoder simply returns an array rather than supporting decoding into a container
  • Sha2 hashing is not available in const contexts, to the "check" and "cb58" features cannot be supported, which means that no checks are possible
  • the implementation also differs because you can't use iterators, so everything uses while loops
  • you can't unwrap() errors in const, so the error conditions are simply assertions

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.

Copy link

codecov bot commented Feb 29, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Nemo157
Copy link
Member

Nemo157 commented Mar 7, 2024

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
Copy link
Contributor Author

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

@Nemo157
Copy link
Member

Nemo157 commented Mar 10, 2024

I've pushed the API idea I had to 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
Copy link
Contributor Author

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!

@Nemo157 Nemo157 enabled auto-merge March 19, 2024 11:37
@Nemo157 Nemo157 added this pull request to the merge queue Mar 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 19, 2024
@Nemo157 Nemo157 added this pull request to the merge queue Mar 19, 2024
@Nemo157 Nemo157 removed this pull request from the merge queue due to a manual request Mar 19, 2024
@Nemo157 Nemo157 added this pull request to the merge queue Mar 19, 2024
Merged via the queue into Nullus157:main with commit 7d3c928 Mar 19, 2024
8 checks passed
@joncinque joncinque deleted the const branch March 19, 2024 12:46
@joncinque joncinque restored the const branch March 19, 2024 12:46
@joncinque joncinque deleted the const branch March 19, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants