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

Removing Core Contracts and Splitting Contracts into 2 repos #241

Open
YamenMerhi opened this issue Aug 1, 2022 · 7 comments
Open

Removing Core Contracts and Splitting Contracts into 2 repos #241

YamenMerhi opened this issue Aug 1, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@YamenMerhi
Copy link
Member

A core contract is efficient to avoid duplicated code especially when the code will be used as shared behavior in other contracts. In our case it's different, we use the core contract to use the code between normal contract and Init contracts (Proxies).

Having the code in the core contract is logical but will introduce a problem when extending these contracts and using custom implementations. And this is what we expect, we're building some contracts that will be absolutely extended and built as a custom implementation.

The problem is that the contracts are using modules and contracts that are only relevant for the normal version and not the Init version. An example would be the address library from OZ, we use the Address.sol lib in Core contracts, which will make the Init contracts also use Address.sol, in the time that the Init contracts should use AddressUpgreadable.

It's not about using the Init contracts for minimal proxies or upgradeable, it's about making the contracts extendible and using the right contracts that will not conflict with other contracts.

A solution for this would be to remove the Core contracts and move the Init contracts to their own repo - No point in keeping them in the same repo, as the person who wants to use the normal contracts will not use the upgradeable.

The Init repo will be maintained by transpiling every code introduced in the normal repo automatically. In this way, we don't need to work twice on the same changes.

@frozeman
Copy link
Member

frozeman commented Aug 4, 2022

@frozeman frozeman added enhancement New feature or request and removed discussion labels Aug 4, 2022
@CJ42
Copy link
Member

CJ42 commented Dec 7, 2022

I agree on going forward for this. This will also help when making refactor changes or for optimisation.

At the moment, gas optimisation is quite difficult sometimes, as some optimisations techniques (e.g: using immutable with values only set on deployment) can only be applied to the standard contracts, and not the proxy.

As a result, we cannot apply any of these optimisations in the Core contract.

@CJ42
Copy link
Member

CJ42 commented Mar 23, 2023

Here are more details on the transpiler that we could use.
Although it is intended to be built mainly for the OZ contracts, it could be adapted + tested for the @lukso/lsp-smart-contracts code base.

Transpiler Repo -➝ https://github.com/OpenZeppelin/openzeppelin-transpiler/

Transpiler Technical notes -➝ https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/patches/UPGRADEABLE.md

Transpiler PR changes (to transpile on @openzeppelin/contracts` repository -➝ OpenZeppelin/openzeppelin-contracts#4041

@blazejkrzak
Copy link

Hey, one question here since you are refactoring something.
Why we have _beforeTokenTransfer(address(0), to, amount, data); in function _mint(),
Why not _beforeTokenMint() ?
It seems counterintuitive to me, since I want to affect mint in my extension, not the transfer

image

@blazejkrzak
Copy link

its lsp7 latest tag

@blazejkrzak
Copy link

* @dev Hook that is called before any token transfer, including minting and burning. => Minting and burning should have separate hooks IMO

@YamenMerhi
Copy link
Member Author

@blazejkrzak You can have custom handling within the hook.

if (first_parameter == address(0)) ==> mint
if (second_parameter == address(0)) ==> burn
else ==> transfer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

4 participants