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 clone variant with per-instance immutable arguments #5109

Merged
merged 18 commits into from
Sep 4, 2024

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jul 10, 2024

Notes about EOF:

  • The clones are not EOF objects. They are legacy contract. As a consequence, they can be used with either EOF or Legacy implementations.
  • Legacy implementations are not an issue. The clone being a legacy contract, it is observable.
  • EOF implementations will not be able to read the params directly. They could however do it indirectly by doing a static call on an helper (Legacy) contract, that would do an extcodecopy on the caller.
/// compiled in legacy mode
contract ImmutableArgsReader {
    function selfFetchCloneArgs() external returns (bytes memory) {
        return Clones.fetchCloneArgs(msg.sender); 
    }
    
    function fetchCloneArgs(address instance) external returns (bytes memory) {
        return Clones.fetchCloneArgs(instance);
    }
}

/// compiled in EOF 
contract MyEOFImplementation {
    ImmutableArgsReader private immutable immutableArgsReader = ImmutableArgsReader(...);
    
    function somefunction(...) public {
       (uint256 arg1, address arg2, ...) = abi.decode(immutableArgsReader.selfFetchCloneArgs(), (uint256, address, ...));
       //...
    }
}

PR Checklist

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

Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: c9aa715

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

@Vectorized
Copy link
Contributor

If you need an ERC1967 minimal proxy with immutable args, this is the formulation of the init code.

abi.encodePacked(
    hex"61",
    uint16(args.length + 0x3d),
    hex"3d8160233d3973",
    implementation,
    hex"60095155f3363d3d373d3d363d7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc545af43d6000803e6038573d6000fd5b3d6000f3",
    args
)

For minimalism, this does not emit the Upgraded event on deployment.

contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
@ernestognw ernestognw added this to the 5.x milestone Aug 15, 2024
Copy link
Member

@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.

Mostly looks good, but I'd rather have WithImmutableArgs as a suffix (e.g. predictDeterministicAddressWithImmutableArgs instead of predictWithImmutableArgsDeterministicAddress)

contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
@Amxx Amxx force-pushed the features/clone-with-immutable-args branch from a56b8bc to 959b36a Compare August 28, 2024 16:27
test/proxy/Clones.test.js Outdated Show resolved Hide resolved
ernestognw
ernestognw previously approved these changes Aug 30, 2024
ernestognw
ernestognw previously approved these changes Sep 2, 2024
@Amxx Amxx modified the milestones: 5.x, 5.2 Sep 2, 2024
Copy link
Member

@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.

Before merging we've been discussing how the code size limit error for the clone contract should be.

Our guidelines indicate to follow the ERC6093 rationale for custom errors, I'm personally fine with EIP170ContractCodeSizeLimit but it's missing the "error prefix". I'd like to propose the following:

  • EIP170ExceededContractSizeLimit (same rationale as in other errors like ERC20ExceededCap or ERC4626ExceededMaxWithdraw)
  • CloneArgumentsTooLong (similar rationale to StringTooLong)

Perhaps we may go even more generic and just have a generic InitcodeSizeExceeded in Errors.sol.

wdyt @Amxx ?

@Amxx
Copy link
Collaborator Author

Amxx commented Sep 3, 2024

My preference goes for CloneArgumentsTooLong because it is clear what the issue is.

If you get EIP170ExceededContractSizeLimit, you must understand that there is a contract size limit, then understand that the immutable args are part of the code, then understand that the immutable args are so long that they cause the size of the proxy to exceed the limit ...

CloneArgumentsTooLong is clear: the argument are too long, you don't need to know why, just that its not supported.

@ernestognw ernestognw changed the title Add clone variant with per-instance parameters stored in "immutable storage" Add clone variant with per-instance immutable arguments Sep 3, 2024
Copy link
Member

@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.

Sounds good.

I think CloneArgumentsTooLong is good enough and I don't want to block this anymore. Ideally, I'd like the naming to meet our guidelines for custom errors and be CloneTooLongArguments but I understand you dislike it because it's not as clearly readable as the current naming. I agree, though.

I'm approving since I don't have a major issue and error backwards compatibility is not guaranteed anyway.

@Amxx Amxx merged commit cb7faaf into OpenZeppelin:master Sep 4, 2024
21 checks passed
@Amxx Amxx deleted the features/clone-with-immutable-args branch September 4, 2024 07:41
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.

3 participants