-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Conversation
🦋 Changeset detectedLatest commit: fb9aac4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
contracts/utils/ShortStrings.sol
Outdated
/** | ||
* @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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
}
?
There was a problem hiding this comment.
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 😕).
Please review the PR title according to the guidelines: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/GUIDELINES.md#pull-requests |
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
Co-authored-by: Francisco <frangio.1@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Allow string → bytes32 with string storage fallback.
This provides an effective way to store short strings (<32chars) in immutable storage.
PR Checklist
npx changeset add
)