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

Turn ERC1967Upgrade into a library #4204

Closed
Amxx opened this issue May 1, 2023 · 10 comments · Fixed by #4325
Closed

Turn ERC1967Upgrade into a library #4204

Amxx opened this issue May 1, 2023 · 10 comments · Fixed by #4325
Labels
breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@Amxx
Copy link
Collaborator

Amxx commented May 1, 2023

Solidity 0.8.20 should include ethereum/solidity#10996, which would finally allow ERC1967Upgrade to be written as a library (instead of an abstract contract).

This raises the question of what to do with IERC1967.

@Amxx Amxx added this to the 5.0 milestone May 1, 2023
@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label May 3, 2023
@balajipachai
Copy link
Contributor

@Amxx If I understood it correctly, since now we can emit Interface events from within library thus, we are trying to change ERC1967 from abstract to library, right?

@Amxx
Copy link
Collaborator Author

Amxx commented May 24, 2023

We were always able to emit events (defined in libraries or in contracts ... not in interfaces) from libraries.

In the past these events were correctly emitted but the corresponding ABI was not included in the contract that uses the library. Since 0.8.20, the event definition is added to the contracts ABI

@balajipachai
Copy link
Contributor

Okay, got it, thank you for the answer, this helps.
That being said, let me get started with this and submit a PR with all necessary changes.

@Amxx
Copy link
Collaborator Author

Amxx commented May 24, 2023

Thank you for your proposal, but please don't. We already have code/branch that starts implementing that and we would not want to ave effort duplication.

@Amxx
Copy link
Collaborator Author

Amxx commented May 24, 2023

Also not that we are currently blocked by this:
ethereum/solidity#14206

The compiler does not let us do

import "../../interfaces/IERC1967.sol";

library ERC1967Upgrade {
    function upgradeTo(address newImplementation) internal {
        _setImplementation(newImplementation);
        emit IERC1967.Upgraded(newImplementation); // This gives an error
    }
}

Its unclear how what workaround we want to use (if any)

@balajipachai
Copy link
Contributor

Okay, let me check ethereum/solidity#14206

@Amxx
Copy link
Collaborator Author

Amxx commented May 26, 2023

Blocker was resolved by solidity, and should be available in 0.8.21.

@balajipachai
Copy link
Contributor

If that's the case then it requires bumping the pragma solidity version in the contract to get started, however, this would impact all other contracts pragma solidity version to be bumped too.

@frangio
Copy link
Contributor

frangio commented May 26, 2023

@balajipachai Yes, all pragmas will be bumped to near-latest Solidity in Contacts 5.0.

@balajipachai
Copy link
Contributor

Great, whose working on bumping the pragma version in all other contracts? I would like to work alongside him/her/them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants