search

makerdao / lockstake

GNU Affero General Public License v3.0
19 stars 11 forks source link

(12) Can add an unchecked to `freed` calculation #21

Closed oldchili closed 8 months ago

oldchili commented 8 months ago

We probably want to require burn <= WAD in the ctr and make this unchecked - https://github.com/makerdao/lockstake/blob/cab89b5a5d737e9f4e399b323884131e37adb2d6/src/LockstakeEngine.sol#L342

sunbreak1211 commented 8 months ago

In the practice we could, but there isn't anything limiting the fee value to <= WAD so it actually works as a check. Maybe it would look better putting the freed assignment before the burn.

oldchili commented 8 months ago

Well fee should be <= WAD right? That's why I thought we can add the ctr check and use the unchecked to save a bit of gas.

sunbreak1211 commented 8 months ago

Well fee should be <= WAD right? That's why I thought we can add the ctr check and use the unchecked to save a bit of gas.

It should but it is not enforced, which we could do in the constructor if we keep immutable.

sunbreak1211 commented 8 months ago

I can do this change as makes sense. However if we end up with a upgradable fee, do we want to put the restriction in the file function?

oldchili commented 8 months ago

Yes, I think that's fine to put it in the file if that ends up being the case. We tend to live with such checks lately, as long as there are not too many of them and they are simple enough.

sunbreak1211 commented 8 months ago

Done: https://github.com/makerdao/lockstake/commit/876c2ccb1ebdfcf8a5264d50309ce32feb279605