Comments (2)
Recommendation to not have to copy & modify some of the OZ contracts (until we can do PRs I will write comments)
The projects has been copying and modifying a lot OpenZeppelin's contracts. This is something that needs to be done very carefully and they did well document the modifications.
However, for the contracts Ownable and OwnableUpgradeable, there is a way to achieve what they want without having to copy nor modify the OZ contracts. Even though this has a higher cost in gas, it is almost negligible compared to the need to maintain a copy of a modified contract. It would also add some security, because the current modifications didn't take this into account:
I would recommend to not make copies of both Ownable contracts
And to replace what we have here (and on some other places):
https://github.com/weiWard/weiward-contracts/blob/main/contracts/exchanges/ETHtxAMM/ETHtxAMM.sol#L70
__Ownable_init_unchained(owner_);
By
__Ownable_init_unchained();
transferOwnership(owner_);
This would work the same way for a bit more gas, but prevent to have modify OZ contracts.
this also add the check if owner_
is different of address(0)
from reviews.
Recommendation: No need for so many extends
On several contracts, we can see the contracts extending:
Initializable,
ContextUpgradeable,
OwnableUpgradeable,
PausableUpgradeable
and then in the initializer function:
function init(address owner_) public virtual initializer {
__Context_init_unchained();
__Ownable_init_unchained(owner_);
There shouldn't be a need to extend Initializable
since ContextUpgradeable
, OwnableUpgradeable
and PausableUpgradeable
already do it.
Also, if the previous recommendation to not modify Ownable
is followed, there shouldn't be a need to extend ContextUpgradeable
. Extending OwnableUpgradeable
should be enough, with the use of __Ownable_init()
in the constructor. This would would launch the chain that does the Context initialization.
And in the case Ownable
is still modified, a lot of those contracts also extend PausableUpgradeable
which could be used to init the context, before initializing Ownable
, with a call to __Pausable_init();
This allows to less extends. I suppose this is all done to save the few ~200 gas this way of doing allows, but this seems again a lot of code & maintaining for so few gas saving.
from reviews.
Related Issues (20)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from reviews.