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

[Feature] BytesLib.toUint(128 and/or 96)! #24

Closed
bh2smith opened this issue Jun 9, 2019 · 5 comments · Fixed by #28
Closed

[Feature] BytesLib.toUint(128 and/or 96)! #24

bh2smith opened this issue Jun 9, 2019 · 5 comments · Fixed by #28

Comments

@bh2smith
Copy link
Contributor

bh2smith commented Jun 9, 2019

It would be awesome, since it appears downcasting uint is not a native task, if this library would have a toUint128 and/or toUint96!

@bh2smith
Copy link
Contributor Author

bh2smith commented Jun 9, 2019

The following should work for each of 24, 63, 96 and 128 (seeing as how 8, 16 and 32 are included, no reason to leave these out).

    function toUint64(bytes memory _bytes, uint _start) internal  pure returns (uint64) {
        require(_bytes.length >= (_start + 8));
        uint32 tempUint;

        assembly {
            tempUint := mload(add(add(_bytes, 0x8), _start))
        }

        return tempUint;
    }

    function toUint96(bytes memory _bytes, uint _start) internal  pure returns (uint96) {
        require(_bytes.length >= (_start + 12));
        uint32 tempUint;

        assembly {
            tempUint := mload(add(add(_bytes, 0xc), _start))
        }

        return tempUint;
    }

    function toUint128(bytes memory _bytes, uint _start) internal  pure returns (uint128) {
        require(_bytes.length >= (_start + 16));
        uint24 tempUint;

        assembly {
            tempUint := mload(add(add(_bytes, 0x10), _start))
        }

        return tempUint;
    }

@GNSPS
Copy link
Owner

GNSPS commented Jun 14, 2019

You're totally right @bh2smith! 😄 Sorry for taking this long to answer!

Care to make a PR? If not I'll just add these over the weekend! 👍

EDIT: can I close your previous issue? (seems to be related to this)

@GeoHickle
Copy link

these function may be wrong. should be
mload(add(add(_bytes, 0x20), _start))
the first word is the length.

@bh2smith
Copy link
Contributor Author

bh2smith commented Mar 9, 2021

@GeoHickle - Thanks for your concern. Since this is just a potential issue with an example snippet of code, I might direct you to the PR (#25) that was merged regarding this feature and cross-reference your claim with the unit tests. Given that they are passing, I would think that this is alright as is, but perhaps there is a missing test case you might want to introduce.

Cheers!

@GNSPS
Copy link
Owner

GNSPS commented Mar 9, 2021

Hey, @GeoHickle! 👋 Thanks for looking into the code attentively!

I'll echo @bh2smith and say that the best way to prove any problem would be with adding test cases. However, I can try to briefly explain why these seem to be correct the way they are.

Essentially, what we are doing with mload is taking 32 bytes from memory and putting them on the top-most stack slot.

Since the types in question are smaller than 32 bytes, Solidity does not (or needs to) read all those 32 bytes. What this means, in practice, is that Solidity masks the most significant bits (i.e., MSBs) of the contents of the stack slot in question and only effectively uses the least significant ones.

This means, that we can leave whatever we want in the MSBs of the stack slot and can have the length value live there because it will simply be disregarded.
So, the technique above is just a way to save gas and not have to read the contents of the array after the length slot and then shift the bytes to the right the relevant number of bytes.

I'll end with something, though. We are all fallible humans! 😄 And, as such, I'd welcome you to try and come up with a test case that signals any malfunction!

🤗

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 a pull request may close this issue.

3 participants