Add reverseBits operations to Bytes.sol

[ernestognw]

Needed for #5680

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: fff6ad2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

Should that be in Math, or should we start a Byte.sol file for this, and CLZ (and other) ?

It doesn't feel like this is a math operation strictly speaking.

[ernestognw]

Should that be in Math, or should we start a Byte.sol file for this, and CLZ (and other) ?

It doesn't feel like this is a math operation strictly speaking.

After the review and changes made I think it makes sense to add it to the Bytes.sol library so I updated accordingly. wdyt?

[Amxx]

For the record, I tried seeing is assembly would save gas:

    function reverseBits256(bytes32 value) internal pure returns (bytes32) {
        assembly ("memory-safe") {
            let mask
            // swap bytes
            mask := mul(div(not(0), 0xFFFF), 0xFF)
            value := or(and(shr(8, value), mask), shl(8, and(value, mask)))
            // swap 2-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFF), 0xFFFF)
            value := or(and(shr(16, value), mask), shl(16, and(value, mask)))
            // swap 4-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFFFFFFFFFF), 0xFFFFFFFF)
            value := or(and(shr(32, value), mask), shl(32, and(value, mask)))
            // swap 8-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF), 0xFFFFFFFFFFFFFFFF)
            value := or(and(shr(64, value), mask), shl(64, and(value, mask)))
            // swap 16-bytes long pairs
            value := or(shr(128, value), shl(128, value))
        }
        return value;
    }

It doesn't !

[Amxx]

It all looks good to me, except the name of the functions.

Lets take the example of reverseBits16. The name sugest it reverses the 16 bits that compose the object. So I would expect that it turn the binary 0b0000000000000001 (0x0001) into 0b1000000000000000 (0x8000) ... but the output is 0b0000000100000000 (0x0100).

what it reverses are not the inidividual bits, its the bytes.

So IMO we should either:

• change the behavior to reverse at the bit level
• change the name from reverseBits16 to reverseBytes2

[Amxx]

IMO, most of the unit tests are not really necessary, and the fuzzing covers its already.
But that is definitelly not a merge blocker.

LGTM.