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 string and bytes support to the StorageSlots library #4008

Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 27, 2023

String manipulation usefull for an upcoming ShortString library.

See #3969

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jan 27, 2023

🦋 Changeset detected

Latest commit: 26db62a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

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

@Amxx Amxx requested a review from frangio January 27, 2023 16:46
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Overall this is good. Minor comment below.

.changeset/rare-kids-punch.md Outdated Show resolved Hide resolved
/**
* @dev Returns an `StringSlot` representation of the string storage pointer `store`.
*/
function getStringSlot(string storage store) internal pure returns (StringSlot storage r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make the assumption that a string will always have offset zero. But I would consider adding an assertion/require just to make sure. Do you know what I mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't

Copy link
Contributor

Choose a reason for hiding this comment

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

Storage pointers have an .offset field in addition to the .slot field. The offset indicates where in the slot the value begins. It can be non-zero when multiple values are packed in a slot. What I'm saying is that for a string pointer we can probably assume offset 0 but it's something we need to be aware of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not able to create a storage string pointer with an offset.

contract Test {
    struct MyStruct {
        uint256 a;
        bool b;
        string c;
    }

    uint256 private constant KEY = uint256(keccak256("some.key.for.the.mapping"));
    mapping(uint256 => MyStruct) private mymapping;


    function getMyStruct() public view returns (uint256 slot, uint256 offset) {
        MyStruct storage ptr = mymapping[KEY];
        assembly {
            slot := ptr.slot
            offset := ptr.offset
        }
    }

    function getMyStructString() public view returns (uint256 slot, uint256 offset) {
        string storage ptr = mymapping[KEY].c;
        assembly {
            slot := ptr.slot
            offset := ptr.offset
        }
    }
}

Offset is 0 in both cases,
slot for getMyStructString is 2 more than getMyStruct

scripts/generate/templates/StorageSlot.js Outdated Show resolved Hide resolved
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I think we can assume offset is zero. We've already asked anyway and should have confirmation soon.

@frangio frangio merged commit 91e8d0b into OpenZeppelin:master Feb 1, 2023
@Amxx Amxx deleted the feature/storageslot/string-and-bytes branch February 1, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants