ebtc-protocol / ebtc

GNU General Public License v3.0
56 stars 25 forks source link

Feat/storage optimization experimental with idx moved to sorted list #754

Closed rayeaster closed 9 months ago

rayeaster commented 10 months ago

As part of https://github.com/ebtc-protocol/ebtc/pull/753 brainstorming, this PR tries to move arrayIndex in struct Cdp to SortedList:

rayeaster commented 10 months ago

a straight-forward gas comparison using hardhat-marmite shows that the version with less struct member item (i.e., arrayIndex moved out) will save 15000 gas for 100 reads

pragma solidity 0.8.17;

contract TestCompareStructSLOAD {

    struct V1 {
       uint256 debt;
       uint256 coll;
       uint256 stake;
       uint256 lrs;
       uint128 arrayIndex;  
    }

    struct V2 {
       uint256 debt;
       uint256 coll;
       uint256 stake;
       uint256 lrs;
    }

    mapping(uint256 => V1) _v1s;    
    mapping(uint256 => V2) _v2s;    
    mapping(uint256 => uint128) _v2idx;

    function compareStructSLOADTest() external returns (uint256 loop) { 
       loop = 100;

       @start<structV1>     
       for (uint256 i = 0;i < loop;i++){
              _v1s[i] = V1(i, i, i, i, uint128(i + 1));
       }
       for (uint256 i = 0;i < loop;i++){
              V1 memory _v = _v1s[i];
              require((_v.debt + _v.coll) == (_v.stake + _v.lrs), "V1 read");
       }
       @end

       @start<structV2>     
       for (uint256 i = 0;i < loop;i++){
              _v2s[i] = V2(i, i, i, i);
              _v2idx[i] = uint128(i + 1);
       }
       for (uint256 i = 0;i < loop;i++){
              V2 memory _v = _v2s[i];
              require((_v.debt + _v.coll) == (_v.stake + _v.lrs), "V2 read");
       }
       @end
    }

}

the output is


                               structV1    │    structV2
compareStructSLOADTest    │    11152973    │    11137166
rayeaster commented 10 months ago

forge test gas-report between the two branches (feat/storage-optimization-experimental-withIdxMovedToSortedList and feat/storage-optimization-experimental) show this PR could save around 200 gas on average for a single CdpManager.getSyncedDebtAndCollShares(_cdpId) read

https://www.diffchecker.com/nEf9vTVj/

image

jack-the-pug commented 10 months ago

_requireCallerIsCdpManagerStorage() should not include msg.sender == address(liquidationLibraryAddress)

Because liquidationLibraryAddress should not be considered as a standalone contract; it is always delegateCalled by cdpManager.

https://github.com/ebtc-protocol/ebtc/blob/2187c97302f598f60985dd5d840ddb921bb23a7f/packages/contracts/contracts/LiquidationLibrary.sol#L590/blob/85bb89f29c2ddc63514e84bf4641c5a2afec5bd0/packages/contracts/contracts/SortedCdps.sol#L724-L728

https://github.com/ebtc-protocol/ebtc/blob/2187c97302f598f60985dd5d840ddb921bb23a7f/packages/contracts/contracts/LiquidationLibrary.sol#L590/blob/85bb89f29c2ddc63514e84bf4641c5a2afec5bd0/packages/contracts/contracts/SortedCdps.sol#L746-L751

Recommendation

Remove _requireCallerIsCdpManagerStorage() and use _requireCallerIsCdpManager() directly.