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 31 commits
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 `parseUint`, `parseInt`, `parseHex` and `parseAddress` to parse strings into numbers and addresses. Also provide variants of these functions that parse substrings, and `tryXxx` variants that do not revert on invalid input.
5 changes: 5 additions & 0 deletions .changeset/rude-cougars-look.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Bytes`: Add a library for common operations on `bytes` objects.
62 changes: 10 additions & 52 deletions contracts/governance/Governor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ import {IERC165, ERC165} from "../utils/introspection/ERC165.sol";
import {SafeCast} from "../utils/math/SafeCast.sol";
import {DoubleEndedQueue} from "../utils/structs/DoubleEndedQueue.sol";
import {Address} from "../utils/Address.sol";
import {Bytes} from "../utils/Bytes.sol";
import {Context} from "../utils/Context.sol";
import {Nonces} from "../utils/Nonces.sol";
import {Strings} from "../utils/Strings.sol";
import {IGovernor, IERC6372} from "./IGovernor.sol";

/**
Expand Down Expand Up @@ -760,68 +762,24 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
address proposer,
string memory description
) internal view virtual returns (bool) {
uint256 len = bytes(description).length;
uint256 length = bytes(description).length;

// Length is too short to contain a valid proposer suffix
if (len < 52) {
if (length < 52) {
return true;
}

// Extract what would be the `#proposer=0x` marker beginning the suffix
bytes12 marker;
assembly ("memory-safe") {
// - Start of the string contents in memory = description + 32
// - First character of the marker = len - 52
// - Length of "#proposer=0x0000000000000000000000000000000000000000" = 52
// - We read the memory word starting at the first character of the marker:
// - (description + 32) + (len - 52) = description + (len - 20)
// - Note: Solidity will ignore anything past the first 12 bytes
marker := mload(add(description, sub(len, 20)))
}
// Extract what would be the `#proposer=` marker beginning the suffix
bytes10 marker = bytes10(Bytes.unsafeReadBytesOffset(bytes(description), length - 52));

// If the marker is not found, there is no proposer suffix to check
if (marker != bytes12("#proposer=0x")) {
if (marker != bytes10("#proposer=")) {
return true;
}

// Parse the 40 characters following the marker as uint160
uint160 recovered = 0;
for (uint256 i = len - 40; i < len; ++i) {
(bool isHex, uint8 value) = _tryHexToUint(bytes(description)[i]);
// If any of the characters is not a hex digit, ignore the suffix entirely
if (!isHex) {
return true;
}
recovered = (recovered << 4) | value;
}

return recovered == uint160(proposer);
}

/**
* @dev Try to parse a character from a string as a hex value. Returns `(true, value)` if the char is in
* `[0-9a-fA-F]` and `(false, 0)` otherwise. Value is guaranteed to be in the range `0 <= value < 16`
*/
function _tryHexToUint(bytes1 char) private pure returns (bool isHex, uint8 value) {
uint8 c = uint8(char);
unchecked {
// Case 0-9
if (47 < c && c < 58) {
return (true, c - 48);
}
// Case A-F
else if (64 < c && c < 71) {
return (true, c - 55);
}
// Case a-f
else if (96 < c && c < 103) {
return (true, c - 87);
}
// Else: not a hex char
else {
return (false, 0);
}
}
// Check that the last 42 characters (after the marker) is a properly formatted address.
(bool success, address recovered) = Strings.tryParseAddress(description, length - 42, length);
return !success || recovered == proposer;
}

/**
Expand Down
18 changes: 18 additions & 0 deletions contracts/utils/Bytes.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {Math} from "./math/Math.sol";

/**
* @dev Bytes operations.
*/
library Bytes {
/// @dev Reads a bytes32 from a bytes array without bounds checking.
function unsafeReadBytesOffset(bytes memory buffer, uint256 offset) internal pure returns (bytes32 value) {
// This is not memory safe in the general case
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not memory safe if it's just reading? I think we can safely annotate it:

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not memory safe because it's possible that the block reads from memory outside the bounds allocated by solidity (it's not checking bounds)

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it. I consider it may be better to annotate as memory-safe and check bounds. Otherwise, optimizations get disabled, perhaps it's cheaper to check and annotate than not checking (when optimizations are applied)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The entier point of this function is to not check the bound. Checking the bound require loading the length, which is one expensive mload.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the entire point was to make it cheaper. I think disabling optimizations we're doing the opposite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically this function is already doing stuff as simple as possible. Not sure any further optimization is required from the compiler for this block.

Copy link
Member

@ernestognw ernestognw Oct 11, 2024

Choose a reason for hiding this comment

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

From here:

Assuming that you’re using the via-ir pipeline, having assembly blocks that are not memory safe (i.e. have memory access within the block, but are not explicitly marked as memory safe) will prevent the ‘stack-to-memory’ mover, and will also not inline as aggressively (inlining is reduced to smaller functions than would be the case with memory safe blocks)

My point is that by having this assembly block that's not memory safe we might be hurting the performance of the whole contract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@ernestognw ernestognw Oct 11, 2024

Choose a reason for hiding this comment

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

Although I agree on adding a Bytes library, I'm not sure this particular function should go in. Perhaps this whole discussion indicates that the function should remain private in its contract.

I'd be in favor of not adding the Bytes library right now if we agree

Copy link
Collaborator Author

@Amxx Amxx Oct 11, 2024

Choose a reason for hiding this comment

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

If it was just in this contract I'd agree.

But the truth is that this function is needed:

So if we keep it private, we have to define it 3 times, in 3 different contracts/libraries.

EDIT: my bad its not needed in #5252, so that is just 2 implementations. Let go with that!

assembly {
value := mload(add(buffer, add(0x20, offset)))
}
}
}
3 changes: 2 additions & 1 deletion contracts/utils/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ Miscellaneous contracts and libraries containing utility functions you can use t
* {Address}: Collection of functions for overloading Solidity's https://docs.soliditylang.org/en/latest/types.html#address[`address`] type.
* {Arrays}: Collection of functions that operate on https://docs.soliditylang.org/en/latest/types.html#arrays[`arrays`].
* {Base64}: On-chain base64 and base64URL encoding according to https://datatracker.ietf.org/doc/html/rfc4648[RFC-4648].
* {Bytes}: Common bytes operations.
* {Strings}: Common operations for strings formatting.
* {ShortString}: Library to encode (and decode) short strings into (or from) a single bytes32 slot for optimizing costs. Short strings are limited to 31 characters.
* {SlotDerivation}: Methods for deriving storage slot from ERC-7201 namespaces as well as from constructions such as mapping and arrays.
* {StorageSlot}: Methods for accessing specific storage slots formatted as common primitive types.
* {StorageSlot}: Methods for accessing specific storage slots formatted as common primitive types.
* {TransientSlot}: Primitives for reading from and writing to transient storage (only value types are currently supported).
* {Multicall}: Abstract contract with a utility to allow batching together multiple calls in a single transaction. Useful for allowing EOAs to perform multiple operations at once.
* {Context}: A utility for abstracting the sender and calldata in the current execution context.
Expand Down
Loading