chore(protocol): with transient storage, the default should be 0, not 1 #15745
Conversation
dantaik
changed the title
| @@ -44,7 +44,7 @@ abstract contract OwnerUUPSUpgradable is UUPSUpgradeable, OwnableUpgradeable { | |||
| if (_loadReentryLock() == _TRUE) revert REENTRANT_CALL(); | |||
| _storeReentryLock(_TRUE); | |||
| _; | |||
| _storeReentryLock(_FALSE); | |||
There was a problem hiding this comment.
I wonder if this _storeReentryLock(_FALSE) call is even necessary. A transient storage is cleared automatically at the end of the transition, no?
There was a problem hiding this comment.
Transient stroage cleared when transaction ends (for all the contracts). But contract A might call B's function (which is nonReentrant) multiple times. So if we don't clear, the second call would fail.
| @@ -44,7 +44,7 @@ abstract contract OwnerUUPSUpgradable is UUPSUpgradeable, OwnableUpgradeable { | |||
| if (_loadReentryLock() == _TRUE) revert REENTRANT_CALL(); | |||
| _storeReentryLock(_TRUE); | |||
| _; | |||
| _storeReentryLock(_FALSE); | |||
| _storeReentryLock(0); | |||
There was a problem hiding this comment.
using 0 is the same as clearing the storage.
There was a problem hiding this comment.
Agreed that this is less confusing, only a single false value now instead of both 0 and _FALSE.
|
Cannot we make: |
|
That makes pausing slightly more expensive, but yeah that doesn't matter, so simply changing the false value does seem like a good approach as well. |
|
A second thought is that we should not merge this PR.... |
|
With this change, the transient slot is actually cleared earlier in the call frames rather than at the end of the tx. But realisticly, it makes no difference.