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 ERC: Minimal Upgradeable Proxies #604

Merged
merged 21 commits into from
Aug 27, 2024

Conversation

Vectorized
Copy link
Contributor

@Vectorized Vectorized commented Aug 23, 2024

@Vectorized Vectorized changed the title Add ERC: Minimal upgradeable proxies Add ERC: Minimal Upgradeable Proxies Aug 23, 2024
@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Aug 23, 2024

✅ All reviewers have approved.

ERCS/erc-9999.md Outdated
@@ -0,0 +1,397 @@
---
eip: 9999
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
eip: 9999
eip: 7760

Assigning next sequential EIP/ERC/RIP number.

Please also update the filename.

ERCS/erc-9999.md Outdated
title: Minimal Upgradeable Proxies
description: Minimal upgradeable proxies with immutable arguments and support for onchain implementation queries
author: Atarpara (@Atarpara), JT Riley (@jtriley-eth), xiaobaiskill (@xiaobaiskill), Vectorized (@Vectorized)
discussions-to: https://ethereum-magicians.org/t/minimal-upgradeable-proxies/20868
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
discussions-to: https://ethereum-magicians.org/t/minimal-upgradeable-proxies/20868
discussions-to: https://ethereum-magicians.org/t/erc-7760-minimal-upgradeable-proxies/20868

Updated topic title.

The discussions topic can just contain a link to the PR. There isn't a need to replicate the ERC text (as this may differ).

ERCS/erc-7760.md Outdated

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174.

### Overview
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Overview
### General specifications

Copy link
Collaborator

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

Editorially I think this ERC draft is ready to submit as a Draft status. Whether this is mathematically "minimal", and/or whether the reference implementation is free of bugs or security flaws is up for technical peer reviews, hence the approve.

ERCS/erc-7760.md Outdated

### Minimal ERC-1967 transparent upgradeable proxy

The transparent upgradeable proxy MUST be deployed by a factory that is responsible for authenticating upgrades.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"MUST be deployed by a factory" seems vaguely defined. What do you try to say here? If a proxy was deployed by EOA via a regular contract call, would that EOA be considered a factory?

Copy link
Contributor Author

@Vectorized Vectorized Aug 26, 2024

Choose a reason for hiding this comment

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

Ok, i’ll change to RECOMMENDED. The rationale is that a factory will be very helpful in preparing the exact bytecode needed.

An EOA can always deploy the same bytecode in place of the factory, and it will still work properly.


Emitting the ERC-1967 events during initialization is OPTIONAL. Indexers MUST NOT expect the initialization code to emit the ERC-1967 events.

### Minimal ERC-1967 transparent upgradeable proxy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Minimal ERC-1967 transparent upgradeable proxy
### Basic form of minimal ERC-1967 transparent upgradeable proxy

ERCS/erc-7760.md Outdated

As it is beneficial to install the factory at a vanity address with leading zero bytes so that the proxy's bytecode can be optimized to be shorter, variants for 14-byte factory address are provided.

#### Minimal ERC-1967 transparent upgradeable proxy (20-byte factory address regular variant)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#### Minimal ERC-1967 transparent upgradeable proxy (20-byte factory address regular variant)
#### 20-byte factory address variant of minimal ERC-1967 transparent upgradeable proxy

ERCS/erc-7760.md Outdated
where `________________________________________` is the 20-byte factory address.


#### Minimal ERC-1967 transparent upgradeable proxy (14-byte factory address regular variant)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#### Minimal ERC-1967 transparent upgradeable proxy (14-byte factory address regular variant)
#### 14-byte factory address variant of minimal ERC-1967 transparent upgradeable proxy

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and below, I think the subsection names of specifications are important as one of the key proposal in this ERC is to propose different forms of minimal proxies. Current naming of these sections are a bit confusing I think, suggest revisit for better readability

ERCS/erc-7760.md Outdated

### Transparent upgradeable proxy factory security considerations

The transparent upgradeable proxy factory MUST implement proper access control to allow proxies to be upgraded by only authorized accounts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

MUST is all caps creates a feel like this is still specification because of RFC 2119. It'd suggest you change the tone

Suggested change
The transparent upgradeable proxy factory MUST implement proper access control to allow proxies to be upgraded by only authorized accounts.
To ensure security, the transparent upgradeable proxy factory must implement proper access control to allow proxies to be upgraded by only authorized accounts.

ERCS/erc-7760.md Outdated

### Onchain querying of implementation for I-variants

The bytecode of the proxies before any optional immutable arguments MUST be verified with the following steps:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these requirements be in the specification section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will move to specification section.


## Motivation

Having standardized minimal bytecode for upgradeable proxies enables the following:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you provide details about how these bytecodes are generated so people can verify the correctness and minimal-ness?

Copy link
Contributor Author

@Vectorized Vectorized Aug 26, 2024

Choose a reason for hiding this comment

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

To the best of our knowledge, the transparent proxy can have 1 opcode shaved off it for its upgrade logic. The rest of the proxies are as minimal as possible, given the constraints of avoiding PUSH0 and optimizing first for runtime gas.

But since it has already been used in the wild, including the version that has been already used is more beneficial than including the theoretically more minimal version. The purpose of this standard is to aid block explorer auto verification. Support for existing proxies outweighs the 2-3 runtime gas saved.

Also, users who want to squeeze out every single bit of runtime gas savings and bytecode size will use the UUPS variant.

Historical context: ERC-1167 was not minimal at the time of finalization. The 0age proxy is more minimal in both runtime gas and bytecode size. But ERC-1167 was chosen as the base for ERC-6551's proxy due to wider support for auto-verification.

With all this in consideration, the transparent variant is minimal-enough.

@eip-review-bot eip-review-bot enabled auto-merge (squash) August 27, 2024 02:16
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eip-review-bot eip-review-bot merged commit 3758136 into ethereum:master Aug 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants