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 toUint, toInt and hexToUint to Strings #5166

Merged
merged 39 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b2eedbe
Strings: add toUint, toInt and hexToUint
Amxx Aug 28, 2024
efd2f30
codespell
Amxx Aug 28, 2024
bc42b25
Update contracts/utils/Strings.sol
Amxx Aug 29, 2024
07f4b44
Update .changeset/eighty-hounds-promise.md
Amxx Sep 2, 2024
40ba631
Update contracts/utils/Strings.sol
Amxx Sep 3, 2024
07ec518
Update Strings.sol
Amxx Sep 3, 2024
95fb0db
Apply suggestions from code review
Amxx Sep 3, 2024
f263819
Update contracts/utils/Strings.sol
Amxx Sep 3, 2024
f51fbe6
Update Strings.sol
Amxx Sep 3, 2024
52a301b
Fix value variable
cairoeth Sep 3, 2024
027859e
make return explicit
Amxx Sep 4, 2024
a91a999
branchless
Amxx Sep 4, 2024
86abf5a
Update contracts/utils/Strings.sol
Amxx Sep 5, 2024
6dca3cb
Update contracts/utils/Strings.sol
Amxx Sep 5, 2024
a7a6e9e
add try variants + use for governor proposal parsing
Amxx Sep 9, 2024
ec9a659
parseAddress
Amxx Sep 11, 2024
568dc7b
use string literal for 0x
Amxx Sep 17, 2024
0292c31
Apply suggestions from code review
Amxx Sep 17, 2024
aea4a14
add support for + prefix in parseInt
Amxx Sep 17, 2024
cf78a9f
Remove invalid "memory-safe" annotation.
Amxx Sep 17, 2024
26cec97
Merge branch 'master' into feature/parse-strings
Amxx Sep 18, 2024
3a7f904
Merge branch 'master' into feature/parse-strings
Amxx Oct 11, 2024
4d18729
Add Bytes.sol
Amxx Oct 11, 2024
c7a7c94
codespell
Amxx Oct 11, 2024
d6319e8
cleanup
Amxx Oct 11, 2024
b3bf461
Update .changeset/eighty-hounds-promise.md
Amxx Oct 11, 2024
2ab63b7
Update .changeset/rude-cougars-look.md
Amxx Oct 11, 2024
231b93b
optimization
Amxx Oct 11, 2024
24f1490
optimization
Amxx Oct 11, 2024
43f0dc1
testing
Amxx Oct 11, 2024
7b7c1fd
comment update
Amxx Oct 11, 2024
2abfa49
Update contracts/utils/Strings.sol
Amxx Oct 11, 2024
f433e6d
making unsafeReadBytesOffset private
Amxx Oct 11, 2024
27c7c0d
optimize
Amxx Oct 11, 2024
75e1e4c
Update contracts/utils/README.adoc
Amxx Oct 11, 2024
4f48757
Update contracts/governance/Governor.sol
Amxx Oct 11, 2024
1ec1e3f
rename parseHex to parseHexUint
Amxx Oct 11, 2024
53d72d7
fix tests
Amxx Oct 11, 2024
c5790f8
optimize
Amxx Oct 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eighty-hounds-promise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Strings`: Add `toUint`, `toInt` and `hexToUint` to parse strings into numbers.`
Amxx marked this conversation as resolved.
Show resolved Hide resolved
86 changes: 86 additions & 0 deletions contracts/utils/Strings.sol
cairoeth marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
*/
error StringsInsufficientHexLength(uint256 value, uint256 length);

/**
* @dev The string being parsed contains characteres that are not in scope of the given base.

Check failure on line 22 in contracts/utils/Strings.sol

View workflow job for this annotation

GitHub Actions / codespell

characteres ==> characters
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
*/
error StringsInvalidChar(bytes1 chr, uint8 base);

/**
* @dev Converts a `uint256` to its ASCII `string` decimal representation.
*/
Expand Down Expand Up @@ -115,4 +120,85 @@
function equal(string memory a, string memory b) internal pure returns (bool) {
return bytes(a).length == bytes(b).length && keccak256(bytes(a)) == keccak256(bytes(b));
}

/**
* @dev Parse an decimal string and returns the value as a `uint256`.
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
*
* This function will revert if:
* - the string contains any character that is not in [0-9].
* - the result does not fit in a uint256.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
function toUint(string memory input) internal pure returns (uint256) {
bytes memory buffer = bytes(input);
ernestognw marked this conversation as resolved.
Show resolved Hide resolved

uint256 result = 0;
for (uint256 i = 0; i < buffer.length; ++i) {
result *= 10; // will revert if overflow
result += _parseChr(buffer[i], 10);
}
return result;
}
Amxx marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev Parse an decimal string and returns the value as a `int256`.
*
* This function will revert if:
* - the string contains any character (outside the prefix) that is not in [0-9].
* - the result does not fit in a int256.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
Amxx marked this conversation as resolved.
Show resolved Hide resolved
function toInt(string memory input) internal pure returns (int256) {
bytes memory buffer = bytes(input);

// check presence of a negative sign.
uint256 offset = bytes1(buffer) == 0x2d ? 1 : 0;
int8 factor = bytes1(buffer) == 0x2d ? int8(-1) : int8(1);

int256 result = 0;
for (uint256 i = offset; i < buffer.length; ++i) {
result *= 10; // will revert if overflow
result += factor * int8(_parseChr(buffer[i], 10)); // parseChr is at most 35, it fits into an int8
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}
return result;
}

/**
* @dev Parse an hexadecimal string (with or without "0x" prefix), and returns the value as a `uint256`.
*
* This function will revert if:
* - the string contains any character (outside the prefix) that is not in [0-9a-fA-F].
* - the result does not fit in a uint256.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
function hexToUint(string memory input) internal pure returns (uint256) {
bytes memory buffer = bytes(input);

// skip 0x prefix if present. Length check doesn't appear to be critical
Copy link
Contributor

Choose a reason for hiding this comment

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

second sentence feels out of place

Suggested change
// skip 0x prefix if present. Length check doesn't appear to be critical
// skip 0x prefix if present

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for the record, this is what the comment was about:

The string length may be less than 2. String could be empty, of just "1". In some cases, doing a input[1] or even a input[0] would revert (out of bound acces). Doing bytes2(input) does not revert if the string is to short.

So we check the length to verify that it ok to read the prefix ? It turn out no.

  • If the string is empty, then regardless of the result of that lookup (that could read dirty bytes), the loop will not run (because length == 0), and the result will be 0 -> that is ok
  • If the string has length 1 then we have two options
    • the check identifies the prefix
      • that means the string is "0", and there is a dirty x after.
      • In that case we have an offset of 2, and the length is 1, so the for loop does not run and the function returns 0 -> that is ok
    • the check does not find the prefix
      • the only "digit" is read by the loop, and the result should be just fine
  • If the string has length >= 2, then the prefix lookup is in the bounds

That is the long explanation (I'm happy its visible in the PR 😃) to something that is not really trivial, and can be missed, but missing it is not a risk.

We may get questions about it though ...

uint256 offset = bytes2(buffer) == 0x3078 ? 2 : 0;
Amxx marked this conversation as resolved.
Show resolved Hide resolved

uint256 result = 0;
for (uint256 i = offset; i < buffer.length; ++i) {
result *= 16; // will revert if overflow
result += _parseChr(buffer[i], 16);
}
return result;
}

function _parseChr(bytes1 chr, uint8 base) private pure returns (uint8) {
uint8 result;

// Try to parse `chr`:
// - Case 1: [0-9]
// - Case 2: [a-z]
// - Case 2: [A-Z]
// - otherwize not supported

Check failure on line 192 in contracts/utils/Strings.sol

View workflow job for this annotation

GitHub Actions / codespell

otherwize ==> otherwise
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
unchecked {
if (uint8(chr) > 47 && uint8(chr) < 58) result = uint8(chr) - 48;
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
else if (uint8(chr) > 96 && uint8(chr) < 123) result = uint8(chr) - 87;
else if (uint8(chr) > 64 && uint8(chr) < 91) result = uint8(chr) - 55;
else revert StringsInvalidChar(chr, base);
}

// check base
if (result >= base) revert StringsInvalidChar(chr, base);
return result;
}
}
27 changes: 27 additions & 0 deletions test/utils/Strings.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {Test} from "forge-std/Test.sol";

import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";

contract StringsTest is Test {
using Strings for *;

function testParse(uint256 value) external {
assertEq(value, value.toString().toUint());
}

function testParseSigned(int256 value) external {
assertEq(value, value.toStringSigned().toInt());
}

function testParseHex(uint256 value) external {
assertEq(value, value.toHexString().hexToUint());
}

function testParseChecksumHex(address value) external {
assertEq(value, address(uint160(value.toChecksumHexString().hexToUint())));
}
}
113 changes: 105 additions & 8 deletions test/utils/Strings.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic');

async function fixture() {
const mock = await ethers.deployContract('$Strings');
Expand Down Expand Up @@ -38,11 +39,13 @@ describe('Strings', function () {
it('converts MAX_UINT256', async function () {
const value = ethers.MaxUint256;
expect(await this.mock.$toString(value)).to.equal(value.toString(10));
expect(await this.mock.$toUint(value.toString(10))).to.equal(value);
});

for (const value of values) {
it(`converts ${value}`, async function () {
expect(await this.mock.$toString(value)).to.equal(value);
expect(await this.mock.$toString(value)).to.equal(value.toString(10));
expect(await this.mock.$toUint(value.toString(10))).to.equal(value);
});
}
});
Expand All @@ -51,39 +54,54 @@ describe('Strings', function () {
it('converts MAX_INT256', async function () {
const value = ethers.MaxInt256;
expect(await this.mock.$toStringSigned(value)).to.equal(value.toString(10));
expect(await this.mock.$toInt(value.toString(10))).to.equal(value);
});

it('converts MIN_INT256', async function () {
const value = ethers.MinInt256;
expect(await this.mock.$toStringSigned(value)).to.equal(value.toString(10));
expect(await this.mock.$toInt(value.toString(10))).to.equal(value);
});

for (const value of values) {
it(`convert ${value}`, async function () {
expect(await this.mock.$toStringSigned(value)).to.equal(value);
expect(await this.mock.$toStringSigned(value)).to.equal(value.toString(10));
expect(await this.mock.$toInt(value.toString(10))).to.equal(value);
});

it(`convert negative ${value}`, async function () {
const negated = -value;
expect(await this.mock.$toStringSigned(negated)).to.equal(negated.toString(10));
expect(await this.mock.$toInt(negated.toString(10))).to.equal(negated);
});
}
});
});

describe('toHexString', function () {
it('converts 0', async function () {
expect(await this.mock.getFunction('$toHexString(uint256)')(0n)).to.equal('0x00');
const value = 0n;
const string = '0x00';

expect(await this.mock.getFunction('$toHexString(uint256)')(value)).to.equal(string);
expect(await this.mock.getFunction('$hexToUint(string)')(string)).to.equal(value);
});

it('converts a positive number', async function () {
expect(await this.mock.getFunction('$toHexString(uint256)')(0x4132n)).to.equal('0x4132');
const value = 0x4132n;
const string = '0x4132';

expect(await this.mock.getFunction('$toHexString(uint256)')(value)).to.equal(string);
expect(await this.mock.getFunction('$hexToUint(string)')(string)).to.equal(value);
});

it('converts MAX_UINT256', async function () {
expect(await this.mock.getFunction('$toHexString(uint256)')(ethers.MaxUint256)).to.equal(
`0x${ethers.MaxUint256.toString(16)}`,
);
const value = ethers.MaxUint256;
const string = `0x${value.toString(16)}`;

expect(await this.mock.getFunction('$toHexString(uint256)')(value)).to.equal(string);
expect(await this.mock.getFunction('$hexToUint(string)')(string)).to.equal(value);
expect(await this.mock.getFunction('$hexToUint(string)')(string.replace(/0x/, ''))).to.equal(value);
});
});

Expand All @@ -97,7 +115,7 @@ describe('Strings', function () {
it('converts a positive number (short)', async function () {
const length = 1n;
await expect(this.mock.getFunction('$toHexString(uint256,uint256)')(0x4132n, length))
.to.be.revertedWithCustomError(this.mock, `StringsInsufficientHexLength`)
.to.be.revertedWithCustomError(this.mock, 'StringsInsufficientHexLength')
.withArgs(0x4132, length);
});

Expand Down Expand Up @@ -177,4 +195,83 @@ describe('Strings', function () {
expect(await this.mock.$equal(str1, str2)).to.be.true;
});
});

describe('Edge cases: invalid parsing', function () {
const ord = x => `0x${x.charCodeAt(0).toString(16)}`;

it('toUint overflow', async function () {
await expect(this.mock.$toUint((ethers.MaxUint256 + 1n).toString(10))).to.be.revertedWithPanic(
PANIC_CODES.ARITHMETIC_OVERFLOW,
);
});

it('toUint invalid character', async function () {
await expect(this.mock.$toUint('0x1'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('x'), 10);
await expect(this.mock.$toUint('1f'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('f'), 10);
await expect(this.mock.$toUint('-10'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('-'), 10);
await expect(this.mock.$toUint('1.0'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('.'), 10);
await expect(this.mock.$toUint('1 000'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord(' '), 10);
});

it('toInt overflow', async function () {
await expect(this.mock.$toInt((ethers.MaxInt256 + 1n).toString(10))).to.be.revertedWithPanic(
PANIC_CODES.ARITHMETIC_OVERFLOW,
);
await expect(this.mock.$toInt((ethers.MinInt256 - 1n).toString(10))).to.be.revertedWithPanic(
PANIC_CODES.ARITHMETIC_OVERFLOW,
);
});

it('toInt invalid character', async function () {
await expect(this.mock.$toInt('0x1'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('x'), 10);
await expect(this.mock.$toInt('1f'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('f'), 10);
await expect(this.mock.$toInt('1.0'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('.'), 10);
await expect(this.mock.$toInt('1 000'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord(' '), 10);
});

it('hexToUint overflow', async function () {
await expect(this.mock.$hexToUint((ethers.MaxUint256 + 1n).toString(16))).to.be.revertedWithPanic(
PANIC_CODES.ARITHMETIC_OVERFLOW,
);
});

it('hexToUint invalid character', async function () {
await expect(this.mock.$hexToUint('0123456789abcdefg'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('g'), 16);
await expect(this.mock.$hexToUint('-1'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('-'), 16);
await expect(this.mock.$hexToUint('-f'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('-'), 16);
await expect(this.mock.$hexToUint('-0xf'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('-'), 16);
await expect(this.mock.$hexToUint('1.0'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord('.'), 16);
await expect(this.mock.$hexToUint('1 000'))
.to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar')
.withArgs(ord(' '), 16);
});
});
});