Skip to content

Add Memory utility library#5189

Merged
Amxx merged 27 commits into
OpenZeppelin:masterfrom
ernestognw:utils/memory
Jul 10, 2025
Merged

Add Memory utility library#5189
Amxx merged 27 commits into
OpenZeppelin:masterfrom
ernestognw:utils/memory

Conversation

@ernestognw

@ernestognw ernestognw commented Sep 4, 2024

Copy link
Copy Markdown
Member

Picked from #5094

PR Checklist

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

@changeset-bot

changeset-bot Bot commented Sep 4, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f03d149

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

@ernestognw ernestognw requested review from Amxx and cairoeth September 4, 2024 17:35
@ernestognw ernestognw marked this pull request as ready for review September 4, 2024 17:35
Comment thread contracts/utils/Memory.sol
Comment thread test/utils/Memory.t.sol Outdated
@Amxx

Amxx commented Sep 5, 2024

Copy link
Copy Markdown
Collaborator

IMO this PR should include the usage of this lib

@ernestognw ernestognw requested a review from Amxx September 5, 2024 18:21
Comment thread docs/modules/ROOT/pages/utilities.adoc Outdated
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@Amxx Amxx added this to the 5.4 milestone Mar 6, 2025
@Amxx

Amxx commented Mar 6, 2025

Copy link
Copy Markdown
Collaborator

Its sad we missed that in 5.3, as it is not controversial. Lets fix conflicts and make sure it gets into 5.4

Comment thread contracts/utils/Memory.sol Outdated
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
Comment thread contracts/utils/Memory.sol Outdated
@Amxx Amxx modified the milestones: 5.4, 5.5 Jun 5, 2025
Comment thread contracts/token/ERC20/extensions/ERC4626.sol Outdated
Comment thread contracts/token/ERC20/utils/SafeERC20.sol Outdated
Comment thread contracts/access/manager/AuthorityUtils.sol Outdated
Comment thread contracts/utils/cryptography/SignatureChecker.sol Outdated
@ernestognw ernestognw marked this pull request as draft June 8, 2025 19:15
Comment thread test/utils/Memory.test.js
@ernestognw ernestognw marked this pull request as ready for review June 9, 2025 00:08
@ernestognw ernestognw mentioned this pull request Jun 9, 2025
3 tasks
@ernestognw ernestognw mentioned this pull request Jun 9, 2025
3 tasks
Comment thread contracts/utils/Memory.sol Outdated
}
}

/// @dev Returns a `Pointer` to the content of a `bytes` buffer. Skips the length word.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Everything from here onward (line 33 to 103) was recently added with no discussion about the usecase. I was personally happy with having utils to "cleanup" memory manually, which AFAIK was the original idea behind this PR.

Some of the things here ressemble `Bytes._unsafeReadBytesOffset, which was left private (and not internal) because we could not guarantee the memory safety. This library goes way furter, with assembly marked memory safe when its not something we know (depends on the user input).

Using the copy function, its quite easy to break stuff by writting to the wrong locations (for example to 0x60).

@ernestognw ernestognw Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The background for these changes is that they're used by the RLP library and we previously kept private:
0332ffe#diff-0399261b2cd954212dda0147b86044380b879ab21e1d1b89c564cb48f3552216L304-L328

I agree with your take. However I think it's worth pursuing this pattern since it composes pretty well imo (as long as we add proper WARNINGS and docs). See how it looks in some places in RLP:

function readRawBytes(Item memory item) internal pure returns (bytes memory) {
    uint256 itemLength = item.length;
    bytes memory result = new bytes(itemLength);
    result.contentPointer().copy(item.ptr, itemLength);

    return result;
}

I would be fine removing these functions. Want to hear your thoughts on the pattern

@0xClandestine 0xClandestine Jul 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rather than exposing these in Memory when they're only used in the RLP library, maybe make them private within the RLP library?

Would also consider constraining destPtr to be greater than fmp offset.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Amxx Worth noting the original proveth implementation does essentially the same, but manually, without mcopy.

  /*
  * @param src Pointer to source
  * @param dest Pointer to destination
  * @param len Amount of memory to copy from the source
  */
  function copy(uint src, uint dest, uint len) private pure {
      if (len == 0) return;

      // copy as many word sizes as possible
      for (; len >= WORD_SIZE; len -= WORD_SIZE) {
          assembly {
              mstore(dest, mload(src))
          }

          src += WORD_SIZE;
          dest += WORD_SIZE;
      }

      // left over bytes. Mask is used to remove unwanted bytes from the word
      uint mask = 256 ** (WORD_SIZE - len) - 1;
      assembly {
          let srcpart := and(mload(src), not(mask)) // zero out src
          let destpart := and(mload(dest), mask) // retrieve the bytes
          mstore(dest, or(destpart, srcpart))
      }
  }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed all new controversial functions. Opened #5792 instead. Also updated the RLP PR (#5680) accordingly as suggested @0xClandestine

@ernestognw ernestognw requested a review from a team as a code owner July 4, 2025 21:59
@ernestognw ernestognw requested a review from Amxx July 9, 2025 18:33
Comment thread contracts/utils/Memory.sol Outdated
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
Comment thread contracts/utils/Memory.sol
Comment thread docs/modules/ROOT/pages/utilities.adoc Outdated
Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com>
arr00
arr00 previously approved these changes Jul 9, 2025
@Amxx

Amxx commented Jul 9, 2025

Copy link
Copy Markdown
Collaborator

Should be covered by #5094

@Amxx Amxx merged commit 21cd7e8 into OpenZeppelin:master Jul 10, 2025
17 of 18 checks passed
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.

4 participants