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 a library for handling short strings in a gas efficient way #4023

Merged
merged 21 commits into from
Feb 6, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
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
5 changes: 5 additions & 0 deletions .changeset/violet-frogs-hide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`ShortStrings`: New library for handling shortstring storage in immutable variables.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions contracts/utils/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ Note that, in all cases, accounts simply _declare_ their interfaces, but they ar

{{Strings}}

{{ShortStrings}}

{{StorageSlot}}

{{Multicall}}
72 changes: 72 additions & 0 deletions contracts/utils/ShortStrings.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.8;

import "./StorageSlot.sol";

/**
* @dev Short string operations.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
library ShortStrings {
type ShortString is bytes32;
frangio marked this conversation as resolved.
Show resolved Hide resolved

error StringTooLong(string str);

/**
* @dev Encode a string of at most 31 chars into a `ShortString`.
*
* This will trigger a `StringTooLong` error is the input string is too long.
*/
function toShortString(string memory str) internal pure returns (ShortString) {
bytes memory bstr = bytes(str);
if (bstr.length > 31) {
revert StringTooLong(str);
}
return ShortString.wrap(bytes32(uint256(bytes32(bstr)) | bstr.length));
}

/**
* @dev Decode a `ShortString` back to a "normal" string.
*/
function toString(ShortString sstr) internal pure returns (string memory) {
uint256 len = length(sstr);
// using `new string(len)` would work locally but is not memory safe.
string memory str = new string(32);
/// @solidity memory-safe-assembly
assembly {
mstore(str, len)
mstore(add(str, 0x20), sstr)
}
return str;
}

/**
* @dev Return the length of a `ShortString`.
*/
function length(ShortString sstr) internal pure returns (uint256) {
return uint256(ShortString.unwrap(sstr)) & 0xFF;
}

/**
* @dev Encode a string into a `ShortString`, or write it to storage if it is too long.
*/
function setWithFallback(string memory value, string storage store) internal returns (ShortString) {
Copy link
Contributor

@frangio frangio Feb 2, 2023

Choose a reason for hiding this comment

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

I'm not convinced by the naming. How do we expect this function be used? As a method with using for?

Copy link
Collaborator Author

@Amxx Amxx Feb 3, 2023

Choose a reason for hiding this comment

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

I was thinking

contract NamedContract {
    ShortStrings.ShortString private immutable _name;
    string private _nameFallback;
    
    constructor(string memory name_) {
        _name = ShortStrings.setWithFallback(name_, _nameFallback);
    }
    
    function name() public view returns (string memory) {
        return ShortStrings.getWithFallback(_name, _nameFallback);
    }
}

How would you do it?

Copy link
Collaborator Author

@Amxx Amxx Feb 3, 2023

Choose a reason for hiding this comment

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

would you prefer

constructor(string memory name_) {
    _name = name_.toShortStringWithFallback(_nameFallback);
}
function name() public view returns (string memory) {
    return _name.toStringWithFallback(_nameFallback);
}

?

Copy link
Contributor

@frangio frangio Feb 3, 2023

Choose a reason for hiding this comment

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

I was thinking the same. Those are good.

We should add that as an example snippet in the docs (without an underscore suffix though 😕).

if (bytes(value).length < 32) {
return toShortString(value);
} else {
StorageSlot.getStringSlot(store).value = value;
return toShortString("");
frangio marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* @dev Decode a string that was encoded to `ShortString` or written to storage using {setWithFallback}.
*/
function getWithFallback(ShortString value, string storage store) internal pure returns (string memory) {
if (length(value) > 0) {
return toString(value);
} else {
return store;
}
}
}
14 changes: 7 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
"glob": "^8.0.3",
"graphlib": "^2.1.8",
"hardhat": "^2.9.1",
"hardhat-exposed": "^0.3.0",
"hardhat-exposed": "^0.3.1",
"hardhat-gas-reporter": "^1.0.4",
"hardhat-ignore-warnings": "^0.2.0",
"keccak256": "^1.0.2",
Expand Down
44 changes: 44 additions & 0 deletions test/utils/ShortStrings.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
const { expect } = require('chai');
const { expectRevertCustomError } = require('../helpers/customError');

const ShortStrings = artifacts.require('$ShortStrings');

function decode(sstr) {
return web3.utils.toUtf8(sstr).slice(0, parseInt(sstr.slice(64), 16));
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

contract('ShortStrings', function () {
before(async function () {
this.mock = await ShortStrings.new();
});

for (const str of [0, 1, 16, 31, 32, 64, 1024].map(length => 'a'.repeat(length))) {
describe(`with string length ${str.length}`, function () {
it('encode / decode', async function () {
if (str.length < 32) {
const encoded = await this.mock.$toShortString(str);
expect(decode(encoded)).to.be.equal(str);

const length = await this.mock.$length(encoded);
expect(length.toNumber()).to.be.equal(str.length);

const decoded = await this.mock.$toString(encoded);
expect(decoded).to.be.equal(str);
} else {
await expectRevertCustomError(this.mock.$toShortString(str), `StringTooLong("${str}")`);
}
});

it('set / get with fallback', async function () {
const { ret0 } = await this.mock
.$setWithFallback(str, 0)
.then(({ logs }) => logs.find(({ event }) => event == 'return$setWithFallback').args);
Amxx marked this conversation as resolved.
Show resolved Hide resolved

expect(await this.mock.$toString(ret0)).to.be.equal(str.length < 32 ? str : '');

const recovered = await this.mock.$getWithFallback(ret0, 0);
expect(recovered).to.be.equal(str);
});
});
}
});