Keep functions simple and save a bit of gas in the process #133
Conversation
|
|
||
| ETH.safeTransfer(_account, _amount); | ||
| } | ||
|
|
||
| function sendETHToDefaultPool(uint256 _amount) external override { | ||
| _requireCallerIsTroveManager(); | ||
|
|
||
| address defaultPoolAddressCached = defaultPoolAddress; |
There was a problem hiding this comment.
This kind of caching optimization is not needed when we're no longer calling a function inbetween. The compiler is capable of optimizing away the 2 accesses. The problem is when you call a function, the compiler has to assume that the state variable may have changed. Technically, for internal calls, that shouldn't even be a concern, as the compiler should be able to analyze that the function being called can't possibly change this bit of state, but maybe it wasn't being that smart.
danielattilasimon
force-pushed
the
gas-refactor-refactor
branch
from
94da9e4
to
65a8cfa
Compare
| @@ -136,25 +136,19 @@ contract ActivePool is Ownable, CheckContract, IActivePool { | |||
| function sendETH(address _account, uint256 _amount) external override { | |||
| _requireCallerIsBOorTroveMorSP(); | |||
|
|
|||
| _accountForSendETH(_account, _amount); | |||
| emit ActivePoolETHBalanceUpdated(ETHBalance -= _amount); | |||
There was a problem hiding this comment.
I don’t like having the operation inside the event. In my opinion it doesn’t help with readability, specially being inside an event (which I tend to overlook)
There was a problem hiding this comment.
I wasn't sure myself either. That settles it then, I'm moving it outside. BTW is there a reason for emitting events before the transfer when sending ETH, but the other way around when receiving?
There was a problem hiding this comment.
No, to be honest, I’m not worrying too much about events, and I was hoping we get them right in #98.
I would use the order that makes it easier to process later, or that feels more natural.
There was a problem hiding this comment.
TBH I've never found these events useful for anything, I didn't even request adding ActivePool/DefaultPool to Dune's tables. I will probably be proposing that we remove them 😆
This is a further optimization of #129, mostly to keep the code simple and easy to read. As a side effect, it also saves a small bit of gas (1-47).
Before:
After: