Nullus157 / bs58-rs

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

encode: introduce EncodeBuilder::apply_to method #85

Closed mina86 closed 2 years ago

mina86 commented 2 years ago

base58 is often used for inputs, e.g. Bitcoin addresses or 32-byte hashes, which can use small statically-sized on-stack buffers when encoding. On the other hand, utility code which does not know what kind of input it’s going to deal with cannot assume a input length limit. This usually leads to encoding into a Vec or a String even if the caller doesn’t actually need to own the buffer with encoded data.

For those situations, introduce EncodeBuilder::apply_to method which checks the maximum length of the output and if it’s small enough performs encoding onto the stack. The limit on the output length is set such that a 64-byte buffer plus version and checuskm can be handled without any heap allocation.

With this change, also refine estimation of the output size. Instead of using 8/5 ratio (i.e. 1.6) use 1.5 which is closer to the actual 1.37 and also produces simpler expression.

Nemo157 commented 2 years ago

One alternative would be integrating with tinyvec behind a feature flag to offload the stack/heap handling and avoid a callback API. That could result in the example looking like

impl<'a> core::fmt::Display for Base58<'a> {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        let data = bs58::encode(self.0).into_tinyvec();
        f.write_str(std::str::from_utf8(&data).unwrap());
    }
}

If we had const-generic-fn-defaults I would use them like fn into_tinyvec<const LEN: usize = 102>(self) -> TinyVec<[u8; LEN]>, but since we don't it would have to give a hard-coded length and using other values would just have to go through the into(impl EncodeTarget) API.

mina86 commented 2 years ago

Yes, you’re probably right. It’s better to implement EncodeTarget for that type. Though the question becomes whether it be tinyvec, arrayvec or maybe both? I’m gonna close this PR ’cuz even though apply_to is more convenient for me, it’s probably not the best API.

By the way, I think the API would benefit if rather than into_foo methods there were Form\ implementation for all various types. This currently is a bit tricky because there’s into method unrelated to Into trait.

Nemo157 commented 2 years ago

By the way, I think the API would benefit if rather than into_foo methods there were Form implementation for all various types. This currently is a bit tricky because there’s into method unrelated to Into trait.

That's actually a really nice idea. I just remembered there's a breaking change that I still have to release (#77) so it's a good time to rename the current into method and add those trait impls.