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.

Sign up for GitHub

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

Jump to bottom

refactor!: remove Core contracts and OwnableUnset #253

Merged
merged 19 commits into from
Oct 9, 2024
Merged

refactor!: remove Core contracts and OwnableUnset #253

merged 19 commits into from
Oct 9, 2024

Conversation

CJ42
Copy link
Collaborator

@CJ42 CJ42 commented Sep 2, 2024 •
edited
Loading

What does this PR introduce?

This PR is a big ♻️ refactor that aims to help simplifying the overall inheritance of the ERC725 contracts and any contract that inherit them.

⚠️ BREAKING CHANGES

  • Remove the OwnableUnset contract in favour of using Ownable and OwnableUpgradeable from @openzeppelin contract libraries.

OwnableUnset is a bad architectural choice because it is an inheritance fork of Ownable from OpenZeppelin.
This creates friction when inheriting these contracts with other contracts from OpenZeppelin that also inherit from Ownable or OwnableUpradeable. For instance, compiler errors around which event to pick, etc... (screenshots below)

To create less frictions, use the Ownable and OwnableUpgradeable from OZ in the ERC725 inheritance. This makes the inheritance graph easier and simpler. Other child contracts can then re-derive from the same AST node in the inheritance tree

Also note that OwnableUnset is barely used at all anywhere in eth ecosystem.

  • Remove the Core contracts in favour of duplicating the core code logic between the Standard (with constructor) and InitAbstract (with _initialize(...)) functions.

📦 Build

  • Upgrade OpenZeppelin contracts dependencies to latest patch release from 4.9.3 -> 4.9.6

Contract Size Difference

image

PR Checklist

  • Wrote Tests
  • Wrote Documentation
  • Ran npm run lint && npm run lint:solidity
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

@CJ42 CJ42 marked this pull request as draft September 2, 2024 19:34
This was referenced Sep 6, 2024
Closed
Closed
@CJ42 CJ42 marked this pull request as ready for review September 6, 2024 02:05
@CJ42 CJ42 mentioned this pull request Sep 6, 2024
Merged
b00ste
b00ste approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@b00ste b00ste left a comment

Choose a reason for hiding this comment

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

LGTM ✅

implementations/contracts/ERC725.sol Show resolved Hide resolved
CJ42 and others added 10 commits September 9, 2024 11:01
@CJ42
chore: upgrade linter dependencies
bfa19d2
@dependabot @CJ42
chore(deps-dev): bump undici from 5.21.0 to 5.26.3 in /implementations
749f1c5 
Bumps [undici](https://github.com/nodejs/undici) from 5.21.0 to 5.26.3.
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.21.0...v5.26.3)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps): bump @babel/traverse in /implementations
8a5244b 
Bumps [@babel/traverse](https://github.com/babel/babel/tree/HEAD/packages/babel-traverse) from 7.18.10 to 7.23.2.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.23.2/packages/babel-traverse)

---
updated-dependencies:
- dependency-name: "@babel/traverse"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps-dev): bump axios from 1.5.1 to 1.6.1 in /implementations
f616d6c 
Bumps [axios](https://github.com/axios/axios) from 1.5.1 to 1.6.1.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v1.5.1...v1.6.1)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps-dev): bump follow-redirects in /implementations
57d60f9 
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.1 to 1.15.4.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.1...v1.15.4)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps): bump undici from 5.26.3 to 5.28.4 in /implementations
eca4edc 
Bumps [undici](https://github.com/nodejs/undici) from 5.26.3 to 5.28.4.
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.26.3...v5.28.4)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps): bump follow-redirects in /implementations
a22fe9b 
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.1 to 1.15.8.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.1...v1.15.8)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps): bump braces from 3.0.2 to 3.0.3 in /implementations
6a8d4de 
Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3.
- [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md)
- [Commits](micromatch/braces@3.0.2...3.0.3)

---
updated-dependencies:
- dependency-name: braces
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot @CJ42
chore(deps-dev): bump axios from 1.6.1 to 1.7.7 in /implementations
0b185bf 
Bumps [axios](https://github.com/axios/axios) from 1.6.1 to 1.7.7.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v1.6.1...v1.7.7)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@CJ42
test: add more tests for ERC725 to ensure it supports both X and Y …
072261d 
…interfaces
@CJ42 CJ42 force-pushed the refactor/reove-ownableunset branch from 4acb584 to 072261d Compare September 9, 2024 04:01
This was referenced Sep 9, 2024
Closed
Merged
YamenMerhi
YamenMerhi approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

LGTM, the diffs are correct

implementations/contracts/ERC725InitAbstract.sol Show resolved Hide resolved
skimaharvey
skimaharvey approved these changes Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@skimaharvey skimaharvey left a comment

Choose a reason for hiding this comment

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

LGTM