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

Add Base64Url encoding #4822

Merged
merged 17 commits into from
Jan 16, 2024
Merged

Add Base64Url encoding #4822

merged 17 commits into from
Jan 16, 2024

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 8, 2024

Fixes #4692

PR Checklist

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

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: c05a170

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 Amxx added this to the 5.1 milestone Jan 8, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

I think we can reuse the logic in some way (from the original encode), I don't feel comfortable shipping repeated code although the small differences may justify it.

What do you think of this:

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v5.0.0) (utils/Base64.sol)

pragma solidity ^0.8.20;

/**
 * @dev Provides a set of functions to operate with Base64 strings.
 */
library Base64 {
    /**
     * @dev Base64 Encoding/Decoding Table
     * See sections 4 and 5 of https://datatracker.ietf.org/doc/html/rfc4648
     */
    string internal constant _TABLE = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
    string internal constant _TABLE_URL = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";

    /**
     * @dev Converts a `bytes` to its Bytes64 `string` representation.
     */
    function encode(bytes memory data) internal pure returns (string memory) {
        return _encode(_TABLE, data, true);
    }

    /**
     * @dev Converts a `bytes` to its Bytes64Url `string` representation.
     */
    function encodeUrl(bytes memory data) internal pure returns (string memory) {
        return _encode(_TABLE_URL, data, false);
    }

    function _encode(string memory lookupTable, bytes memory data, bool pad) private pure returns (string memory) {
        /**
         * Inspired by Brecht Devos (Brechtpd) implementation - MIT licence
         * https://github.com/Brechtpd/base64/blob/e78d9fd951e7b0977ddca77d92dc85183770daf4/base64.sol
         */
        if (data.length == 0) return "";

        uint256 resultLength = pad
            ? // The final length should be `bytes` data length divided by 3 rounded up and then multiplied by 4
            // so that it leaves room for padding the last chunk
            // - `data.length + 2`  -> Round up
            // - `/ 3`              -> Number of 3-bytes chunks
            // - `4 *`              -> 4 characters for each chunk
            4 * ((data.length + 2) / 3)
            : // The final length should be `bytes` data length multiplied by 4/3 rounded up
            // as opposed to when padding is required to fill the last chunk.
            // - `4 *`              -> 4 characters for each chunk
            // - `data.length + 2`  -> Round up
            // - `/ 3`              -> Number of 3-bytes chunks
            (4 * data.length + 2) / 3;

        string memory result = new string(resultLength);

        /// @solidity memory-safe-assembly
        assembly {
            // Prepare the lookup table (skip the first "length" byte)
            let tablePtr := add(lookupTable, 1)

            // Prepare result pointer, jump over length
            let resultPtr := add(result, 32)

            // Run over the input, 3 bytes at a time
            for {
                let dataPtr := data
                let endPtr := add(data, mload(data))
            } lt(dataPtr, endPtr) {

            } {
                // Advance 3 bytes
                dataPtr := add(dataPtr, 3)
                let input := mload(dataPtr)

                // To write each character, shift the 3 bytes (18 bits) chunk
                // 4 times in blocks of 6 bits for each character (18, 12, 6, 0)
                // and apply logical AND with 0x3F which is the number of
                // the previous character in the ASCII table prior to the Base64 Table
                // The result is then added to the table to get the character to write,
                // and finally write it in the result pointer but with a left shift
                // of 256 (1 byte) - 8 (1 ASCII char) = 248 bits

                mstore8(resultPtr, mload(add(tablePtr, and(shr(18, input), 0x3F))))
                resultPtr := add(resultPtr, 1) // Advance

                mstore8(resultPtr, mload(add(tablePtr, and(shr(12, input), 0x3F))))
                resultPtr := add(resultPtr, 1) // Advance

                mstore8(resultPtr, mload(add(tablePtr, and(shr(6, input), 0x3F))))
                resultPtr := add(resultPtr, 1) // Advance

                mstore8(resultPtr, mload(add(tablePtr, and(input, 0x3F))))
                resultPtr := add(resultPtr, 1) // Advance
            }

            if eq(pad, 1) {
                // When data `bytes` is not exactly 3 bytes long
                // it is padded with `=` characters at the end
                switch mod(mload(data), 3)
                case 1 {
                    mstore8(sub(resultPtr, 1), 0x3d)
                    mstore8(sub(resultPtr, 2), 0x3d)
                }
                case 2 {
                    mstore8(sub(resultPtr, 1), 0x3d)
                }
            }
        }

        return result;
    }
}

.changeset/twenty-feet-grin.md Outdated Show resolved Hide resolved
contracts/utils/Base64.sol Outdated Show resolved Hide resolved
contracts/utils/Base64.sol Outdated Show resolved Hide resolved
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM. On a side note, I noticed the gas reports have been broken for a while and wanted to check the difference for the encode function.

I'll take a look and try to get it working to this branch but feel free to merge.

EDIT: Wait, I'll try to fuzz it with vm.ffi first and see if I can come with an edge case.

@ernestognw
Copy link
Member

In order to run the fuzz tests we need to enable --ffi. According to the Foundry docs:

It is generally advised to use this cheat code as a last resort, and to not enable it by default, as anyone who can change the tests of a project will be able to execute arbitrary commands on devices that run the tests.

However, I don't see a problem in the current setup. The forge test -vv in the checks.yml configuration doesn't have access to anything sensible.

What do you think @Amxx? I'm open to revert the changes but I think they're worth it.

@ernestognw
Copy link
Member

I'll take a look and try to get it working to this branch but feel free to merge.

I couldn't find why it is failing. If I run npm run gas-report the report is not even produced. The most relevant related issue I could find was cgewecke/hardhat-gas-reporter#150 but there's no clear reason.

I suspect it may have to do with the ethers.js migration because there's nothing else that could have affected

@@ -82,7 +82,7 @@ jobs:
- name: Set up environment
uses: ./.github/actions/setup
- name: Run tests
run: forge test -vv
run: forge test -vv --ffi
Copy link
Collaborator Author

@Amxx Amxx Jan 11, 2024

Choose a reason for hiding this comment

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

Note: We agreed to not include that in the package. The foundry tests will be disabled in CI (until base64 eventually becomes foundray-native)

@ernestognw
Copy link
Member

Removed --ffi and added a unit test for + in encode and - in encodeURL

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM

@ernestognw
Copy link
Member

We need to wait for #4834

@ernestognw
Copy link
Member

The gas report showed that the deployment cost of the library increased (as expected). Note that the deployment cost is not as important since functions from libraries are inlined.

This is the individual result for encode

- Base64Test:testEncode(bytes) (runs: 256, μ: 13038, ~: 11985)
+ Base64Test:testEncode(bytes) (runs: 256, μ: 13125, ~: 12072)

The difference seems reasonable.

Merging

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.

Base64URL
2 participants