OpenZeppelin/openzeppelin-contracts

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

Closed this issue · 2 comments

invocamanman commented

🧐 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

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

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.