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

Remove dependency on micromath, compute height with integers only #18

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Jan 26, 2023

This reduces the overall dependency tree, which was only being used in computing the tree height.
It may also be benefitial in some environments (e.g.: zk) to be able to use integer math only.

The snippet leaves_count as f32 caused some trouble. Let's remove it.

(Description stolen from #16 because it's honestly much better than what I had originally typed up.)

@sashaduke
Copy link

sashaduke commented Jan 26, 2023

To expand on this - the use of floats is not supported on CosmWasm's WASM VM, so removing this means the crate will now be able to used in smart contracts deployed on CosmWasm without any issues.

@matthiasgoergens
Copy link
Contributor Author

To expand on this - the use of floats is not supported in WebAssembly, so removing this means the crate will now be able to used in programs which are compiled to WASM without any issues.

To be more precise WebAssembly supports floats just fine by itself, but Sasha has an application that only supports a subset of WASM that doesn't support floats.

@matthiasgoergens matthiasgoergens changed the title Avoid use of floats Removing dependency on micromath. computing height with integers only Feb 11, 2023
@matthiasgoergens matthiasgoergens changed the title Removing dependency on micromath. computing height with integers only Removing dependency on micromath, compute height with integers only Feb 11, 2023
@matthiasgoergens matthiasgoergens changed the title Removing dependency on micromath, compute height with integers only Remove dependency on micromath, compute height with integers only Feb 11, 2023
@antouhou
Copy link
Owner

@matthiasgoergens @sashaduke This uses std though, which is not compatible with the no-std feature. I'll try to think how to do that without using an f32 and std, thank you for reporting this!

@matthiasgoergens
Copy link
Contributor Author

Thanks for checking!

I wonder if we can ask the compiler or so to enforce the no-std rule?

@sashaduke
Copy link

sashaduke commented Feb 24, 2023

no_std compatibility can be maintained by using core::mem::sizeof instead of std::mem::sizeof

@matthiasgoergens
Copy link
Contributor Author

@sashaduke Thanks, I implemented your suggestion!

@antouhou I can build the fixed version with cargo build --no-default-features (and just to confirm, this check fails when trying on my original version).

I think about how to automate this check.

@antouhou antouhou merged commit fa99c5d into antouhou:master Mar 14, 2023
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.

None yet

3 participants