search

OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
25.02k stars 11.82k forks source link

ERC4626 replace _asset with asset() in order to be easily overrideable #5320

Open invocamanman opened 6 days ago

invocamanman commented 6 days ago

🧐 Motivation

Overriding asset() function which is defined as virtual might be kinda common practice. If a project wants to do so it would be nice that all the places that uses the internal variables _asset will be overrided as well.

📝 Details

The internal variable _asset is used in: totalAssets, _deposit and _withdraw. I suggest to replace them with the function asset(), to have a more consistent override

ernestognw commented 5 days ago

Hi @invocamanman, thanks for raising

I opened a PR for this in #5322. My first impression is that the change would be breaking if an ERC4626Upgradeable contract overrode asset() and then performs an upgrade. Though, I haven't found a concrete security concern.

Another thing that came to my mind is that I've found it weird to call a public function inside an internal one. Nothing wrong with it but I remember we had a similar conversation in another issue.

Amxx commented 5 days ago

Another thing that came to my mind is that I've found it weird to call a public function inside an internal one. Nothing wrong with it but I remember we had a similar conversation in another issue.

We do that quite often. I don't think it is an issue.