Skip to content
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
use shared logic with _setHexString
  • Loading branch information
cairoeth committed Jun 3, 2024
commit db76d546a18402de2cdf5ed2b1db782f5ae2fbbf
30 changes: 17 additions & 13 deletions contracts/utils/Strings.sol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redesigned toChecksumHexString to avoid double allocation.

  • removed the need for _unsafeSetHexString
  • removed the need for HEX_DIGITS_UPPERCASE

Copy link
Member

Choose a reason for hiding this comment

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

Right this is extremely cleaner. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great changes, thxs!

Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,15 @@ library Strings {
* @dev Converts a `uint256` to its ASCII `string` hexadecimal representation with fixed length.
*/
function toHexString(uint256 value, uint256 length) internal pure returns (string memory) {
uint256 localValue = value;
if (length < Math.log256(value) + 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

computing the log is quite expensive. Checking at the end is less expensive (when everything works fine, which is the case we should optimize for). Any reason to not keep it?

revert StringsInsufficientHexLength(value, length);
}

bytes memory buffer = new bytes(2 * length + 2);
buffer[0] = "0";
buffer[1] = "x";
for (uint256 i = 2 * length + 1; i > 1; --i) {
buffer[i] = HEX_DIGITS[localValue & 0xf];
localValue >>= 4;
}
if (localValue != 0) {
revert StringsInsufficientHexLength(value, length);
}
_setHexString(buffer, 2, value);

return string(buffer);
}

Expand All @@ -91,12 +89,8 @@ library Strings {
* representation, according to EIP-55.
*/
function toChecksumHexString(address addr) internal pure returns (string memory) {
uint160 localValue = uint160(addr);
bytes memory lowercase = new bytes(40);
for (uint256 i = 40; i > 0; --i) {
lowercase[i - 1] = HEX_DIGITS[localValue & 0xf];
localValue >>= 4;
}
_setHexString(lowercase, 0, uint160(addr));
bytes32 hashedAddr = keccak256(abi.encodePacked(lowercase));
Copy link
Collaborator

@Amxx Amxx Jun 4, 2024

Choose a reason for hiding this comment

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

You often do abi.encodePacked(x) when x is already a bytes. (here and on line 107)

Used like this, abi.encodePacked is the identity function ... the output exactly match the input ... so it is not necessary. One thing that happens however, is that memory is allocated for this copy... there is also a copy loop (or a mcopy if we are lucky).

Anyway, this increasse costs and leaks memory, so we should avoid it!


bytes memory buffer = new bytes(42);
Expand All @@ -122,4 +116,14 @@ library Strings {
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 Sets the hexadecimal representation of a value in the specified buffer starting from the given offset.
*/
function _setHexString(bytes memory buffer, uint256 offset, uint256 value) private pure {
for (uint256 i = buffer.length; i > offset; --i) {
buffer[i - 1] = HEX_DIGITS[value & 0xf];
value >>= 4;
}
}
}