OpenZeppelin

[best-practicers]

OpenZeppelin

[dievardump]

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)

[dievardump]

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 Ownableis 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.