search

daifoundation / maker-otc

The OasisDEX protocol - Simple on-chain market for ERC20 tokens
GNU Affero General Public License v3.0
101 stars 39 forks source link

optimize synchronized modifier #172

Closed BrendanChou closed 5 years ago

BrendanChou commented 5 years ago

Makes the synchronized modifier closer to the current NonReentrant modifier in zeppelin-solidity. https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/utils/ReentrancyGuard.sol

Before: STORAGEADD + STORAGEKILL (25000 cost + 15000 refund) After: STORAGEMOD (5000 cost)

Saves 5000 in the worst case (probably something like calls to make(). Saves 20000 in the best case (probably in something like cancel() you won't ever see the gas refund since there is already so much being refunded from freeing up the storage of the order itself).

Therefore this saves approx 25000 gas per pair of make() and cancel() calls for Market Makers. This is more gas than a basic transaction takes.

rainbreak commented 5 years ago

I don't disagree with your overall conclusion, but I think the numbers are off as STORAGEADD is 20000 (not 25000).

BrendanChou commented 5 years ago

STORAGEKILL is (5000 cost + 15000 refund), so I meant STORAGEADD+STORAGEKILL being equivalent to (25000 cost + 15000 refund)

BrendanChou commented 5 years ago

Although I see now this change might be incompatible with your planned DEC18 changes given that you are now checking the locked bool in several additional places in your matching_market contract now

BrendanChou commented 5 years ago

Solution 1 is slightly more work but more efficient: use only external functions that are synchronized as public endpoints that point to internal functions with the actual implementation. These internal functions would be used when other parts of the code want to use those functions and the external functions would only be called by external addresses. That way you can use synchronized external functions in matching_market as well.

Solution 2 is slightly less work but still would give around 15k in gas savings per cancel call. That would involve using a uint that changes between UNLOCKED=1 and LOCKED=2 states rather than between 0 and 1. This would be a 10k cost versus the current 25k cost + 15k (non-refunded) refund

BrendanChou commented 5 years ago

Closing since this was not changed for the most recent version of OasisDEX and will be optimized away by https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1283.md