Skip to content

Conversation

@ernestognw
Copy link
Member

@ernestognw ernestognw commented May 10, 2025

@changeset-bot
Copy link

changeset-bot bot commented May 10, 2025

🦋 Changeset detected

Latest commit: 371595d

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 changed the title Add RLP library Add RLP and TrieProof libraries May 11, 2025
@ernestognw ernestognw added this to the 5.x milestone Jun 4, 2025
// OpenZeppelin Contracts (last updated v5.4.0) (token/ERC20/extensions/ERC4626.sol)

pragma solidity ^0.8.20;
pragma solidity ^0.8.24;
Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're keeping it we should add it to the changelog. I did in 698221f

Copy link
Collaborator

Choose a reason for hiding this comment

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

This imports memory, and memory needs 0.24 because it does mcopy

uint256 lengthLength = prefix - SHORT_OFFSET - SHORT_THRESHOLD;

require(itemLength > lengthLength, RLPInvalidDataRemainder(lengthLength, itemLength));
require(bytes1(item.load(0)) != 0x00); // TODO: custom error for sanity checks
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe panic with RESOURCE_ERROR code?

Copy link
Member Author

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Almost there!

@ernestognw
Copy link
Member Author

The only thing left to discuss is the error to use in _decodeLength. See #5680 (comment)

Once that's done I think the PR should be good. The main problem may be the coverage hit but we may be fine with that (?)

@ernestognw
Copy link
Member Author

Can't approve my own PR, but LGTM

@Amxx Amxx merged commit 6538427 into OpenZeppelin:master Sep 19, 2025
20 of 21 checks passed
@ernestognw
Copy link
Member Author

The forward looks pretty cool, though!

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.

5 participants