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

Implement specific bit types for integers #677

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

rory-ocl
Copy link
Contributor

Use generic integer types with specific bit sizes to allow 1:1 mapping between rust and solidity types.

Motivation

I am doing some work on OffchainLabs/stylus-sdk-rs and am having some issues with the way that integer types are currently implemented here. For stylus, we require a 1:1 type mapping between rust types and solidity types. This breaks down for us because in alloy, currently, there is many solidity types that map to each rust integer type. For instance, both uint24 and uint32 in solidity map to u32 in rust.

Solution

Our proposed solution is to use alloy_primitves::{Signed, Uint} with the proper number of bits for each integer type.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Note

This is currently WIP as there are a few tests that are currently panicking due to failed library assertions. I believe the issue is in the int_impls! macro in core/crates/sol-types/src/types/data_type.rs. I need to review the usage of the generic bit sizes that are used here. I also need to check the try_from implementation used in a couple other tests.

This is currently WIP as there are a few tests that are currently
panicking due to failed library assertions.
@joshuacolvin0
Copy link

The motivation for this PR is the same as #545

@DaniPopes
Copy link
Member

DaniPopes commented Jul 30, 2024

OK with this, however this will have worse devex as ruint has worse APIs for conversions with primitives. Will look into the failures as well this week and get a breaking release out.

@@ -181,23 +181,128 @@ macro_rules! impl_sol_value {
)*};
}

type U24 = alloy_primitives::Uint<24, 1>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should re-export all of these in primitives::aliases and import them here with a glob import

@joshuacolvin0
Copy link

joshuacolvin0 commented Jul 30, 2024

OK with this, however this will have worse devex as ruint has worse APIs for conversions with primitives. Will look into the failures as well this week and get a breaking release out.

Note that we have already worked around the issue for now with this PR that will be merged soon:
OffchainLabs/stylus-sdk-rs#137

The concern about worse devex is certainly valid, and definitely open to other possible suggestions.

@DaniPopes
Copy link
Member

It's also intuitive that u24 -> U24 rather than u32, but there is also the concern that storing it is 8 bytes instead of 3/4

I'm OK with it TBH

wdyt @prestwich @gakonst @mattsse

also FYI if you update to latest alloy versions we have made the string representation of types const too, so you can use it in your const ABI stuff: https://docs.rs/alloy-sol-types/latest/alloy_sol_types/trait.SolType.html#associatedconstant.SOL_NAME

@DaniPopes DaniPopes marked this pull request as ready for review July 30, 2024 21:39
@mattsse
Copy link
Member

mattsse commented Jul 30, 2024

reasonable imo

@DaniPopes DaniPopes merged commit 96dfb0a into alloy-rs:main Aug 7, 2024
30 checks passed
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.

4 participants