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 EIP: Minimal Upgradable Proxy Contract #7229

Closed
wants to merge 14 commits into from

Conversation

xiaobaiskill
Copy link

@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Jun 24, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jun 24, 2023

File EIPS/eip-7229.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot changed the title eip-1168: minimal upgradable proxy contract Add EIP: Minimal Upgradable Proxy Contract Jun 24, 2023
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Jun 24, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 24, 2023
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
EIPS/eip-7229.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added w-ci Waiting on CI to pass and removed w-ci Waiting on CI to pass labels Jun 25, 2023
@github-actions
Copy link

The commit 8184ce4 (as a parent of b87b9f0) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 25, 2023
@xiaobaiskill
Copy link
Author

all errors are fixed. view please.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

You could slim down your example code a bit. We try to keep reference implementations minimal.


The big overall comment here is that you aren't proposing a standard, you're proposing a specific implementation. We don't generally allow specific implementations to be published as EIPs.

EIPs shine when multiple different parties have to coordinate to implement a system. For example, ERC-20 is a great example of what an EIP should be because every wallet wants to work with every ERC-20 token, and every ERC-20 token wants to work with every wallet.

This EIP doesn't really coordinate anything. If someone wants to use your bytecode, they can just use it without having to coordinate with anyone else.

This isn't me saying your bytecode doesn't have value. It absolutely does, and you should promote it as much as possible. Just not in the EIPs repository.


I'm going to close this PR, but feel free to re-open it if you would like to rewrite it as a standard instead of an implementation.

type: Standards Track
category: ERC
created: 2023-06-24
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
---
requires: 3855
---

@@ -0,0 +1,249 @@
---
eip: 7229
title: Minimal Upgradable Proxy Contract
Copy link
Contributor

Choose a reason for hiding this comment

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

Your title doesn't differentiate itself enough form the other proxy contract standards. Maybe something like:

Suggested change
title: Minimal Upgradable Proxy Contract
title: Gas Optimized Proxy Contract

---
eip: 7229
title: Minimal Upgradable Proxy Contract
description: A lightweight contract upgrade pattern designed to save gas costs while providing the ability to upgrade contracts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably mention custom assembly in here somewhere.


## Abstract

This proposal introduces the Minimal Upgradable Contract, a lightweight alternative to the existing upgradable contract implementation provided by OpenZeppelin. The Minimal Upgradable Contract aims to significantly reduce gas consumption during deployment while maintaining upgradability features.
Copy link
Contributor

Choose a reason for hiding this comment

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

The abstract should be a terse technical summary of the proposal. The reader should be able to get a high level understanding of how the proposal accomplishes its goal (in this case, handrolled assembly.)

The exact bytecode of the standard minimal upgradable contract is this:
`7fxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx73yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy81556009604c3d396009526010605560293960395ff3365f5f375f5f365f7f545af43d5f5f3e3d5f82603757fd5bf3`; In this bytecode, the 1st to 32nd byte (inclusive) needs to be replaced with a 32-byte slot, and the 34th to 53rd byte (inclusive) needs to be replaced with a 20-byte address.
Please note that the placeholders `xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx` represent the 32-byte slot and `yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy` represents the 20-byte address.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably mention what the "slot" and "address" mean. To someone very familiar with solidity/proxies, it's obvious that the address is the implementation contract, but it's good to be explicit.

@@ -0,0 +1,36 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

No UNLICENSED.

@@ -0,0 +1,43 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

No UNLICENSED

@@ -0,0 +1,35 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

No UNLICENSED.

@@ -0,0 +1,36 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

No UNLICENSED

@@ -0,0 +1,36 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

No UNLICENSED.

@SamWilsn SamWilsn closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants