Gas optimization #896

0xRizwan · 2024-02-13T12:29:51Z

Resolves #895

molecula451 · 2024-02-13T14:31:43Z

Please fix the build actions as some are not passing

0xRizwan · 2024-02-14T04:35:07Z

Hi team,

This is regarding to error pertaining to test coverage. error is due to mismatch of coverage and for this reason, check is not passing.

Development branch coverage: 79.0
PR branch coverage: 78.9

I have analysed the issue but not sure where 0.1 tests are decreased. I am quite sure, while testing none of the tests were deleted which can be checked in commits.

On overall gas saving,

Overall gas change from current implementation: 6.6 million which is ~2.3 % change.

I haven't checked each function gas saving which is bit time consuming. However i see the goal to gas saving is acheived and i think 6.6 million is pretty good number.

I would request the team to review the changes and revert for any clarifications.

0x4007

Look like you need to format your code. Also by "custom error codes" I thought you meant like how Gnosis Safe does it, like "GS018" and then the user goes to their documentation to see the full description of the error which makes a ton of sense to me.

0x4007 · 2024-02-14T05:18:08Z

packages/contracts/src/dollar/libraries/LibAppStorage.sol

@@ -80,101 +80,149 @@ contract Modifiers {

    /// @notice Checks that method is called by a contract owner
    modifier onlyOwner() {
-        LibDiamond.enforceIsContractOwner();


This seems like a big change is this correct?

not all, i mean, i may not be in favor of all the changes, he proposed a few changes but it highly refactors much of working code to pattern matching that rarely makes sense, like doing internal view functions to them pass it to a modifier

This is gas saving measure. It is recommended to move the modifiers require statements into an internal functions. This reduces the size of compiled contracts that use the modifiers. Putting the require in an internal function decreases contract size when a modifier is used multiple times.

contract size decrease can highly improve by activating compiler optimizer and it does handle very well, without heavily modifying working (tested and debugged code) i think you also need to add more testing at https://github.com/ubiquity/ubiquity-dollar/tree/development/packages/contracts/test

highlighting the use of the internal functions etc...

optimizer is already set to true in config.

i think #895 (comment) the comment is not enough for a comparison, can you paste us the forge results where edges are optimized

Sure, here it is.

test_Init() (gas: 0 (0.000%)) testCutFacetShouldRevertWhenAddToZeroAddress() (gas: 0 (0.000%)) testCutFacetShouldRevertWhenFunctionAlreadyExists() (gas: 0 (0.000%)) testCutFacetShouldRevertWhenNoSelectorsProvidedForFacetForCut() (gas: 0 (0.000%)) testCutFacetShouldRevertWhenNotOwner() (gas: 0 (0.000%)) testFacetsHaveCorrectSelectors() (gas: 0 (0.000%)) testHasMultipleFacets() (gas: 0 (0.000%)) testLoupeFacetAddressesEqualsTheListOfAvailableFacets() (gas: 0 (0.000%)) testSelectors_ShouldBeAssociatedWithCorrectFacet() (gas: 0 (0.000%)) test_ShouldSupportInspectingFacetsAndFunctions() (gas: 0 (0.000%)) testName_ShouldReturnTokenName() (gas: 0 (0.000%)) testPermit_ShouldIncreaseSpenderAllowance() (gas: 0 (0.000%)) testPermit_ShouldRevert_IfDeadlineExpired() (gas: 0 (0.000%)) testPermit_ShouldRevert_IfSignatureIsInvalid() (gas: 0 (0.000%)) testSymbol_ShouldReturnSymbolName() (gas: 0 (0.000%)) testGetRoleAdmin_ShouldReturnAdminRole() (gas: 0 (0.000%)) testGrantRole_ShouldRevertWhenNotAdmin() (gas: 0 (0.000%)) testGrantRole_ShouldWork() (gas: 0 (0.000%)) testHasRole_ShouldReturnTrue() (gas: 0 (0.000%)) testRevokeRole_ShouldRevertWhenNotAdmin() (gas: 0 (0.000%)) testRevokeRole_ShouldWork() (gas: 0 (0.000%)) testPurchaseTargetAmount(uint32,uint256) (gas: 0 (0.000%)) testPurchaseTargetAmountShouldRevertIfParamsNotSet() (gas: 0 (0.000%)) testPurchaseTargetAmountShouldRevertIfSupplyZero() (gas: 0 (0.000%)) testCannotGetRewardsOtherAccount() (gas: 0 (0.000%)) testGetStakingShareInfo() (gas: 0 (0.000%)) testPendingGovernance(uint256) (gas: 0 (0.000%)) testTotalShares() (gas: 0 (0.000%)) testGetInitialGovernanceMul() (gas: 0 (0.000%)) testGetManager_ShouldGet() (gas: 0 (0.000%)) test_burnCreditNftForCreditRevertsIfExpired() (gas: 0 (0.000%)) test_burnCreditNftForCreditRevertsIfNotEnoughBalance() (gas: 0 (0.000%)) test_burnCreditTokensForDollarsIfNotEnoughBalance() (gas: 0 (0.000%)) test_burnCreditTokensForDollarsRevertsIfPriceLowerThan1Ether() (gas: 0 (0.000%)) test_burnExpiredCreditNftForGovernanceRevertsIfNotExpired() (gas: 0 (0.000%)) test_exchangeDollarsForCreditRevertsIfPriceHigherThan1Ether() (gas: 0 (0.000%)) test_redeemCreditNftRevertsIfCreditNftExpired() (gas: 0 (0.000%)) test_redeemCreditNftRevertsIfPriceLowerThan1Ether() (gas: 0 (0.000%)) testGetCreditAmount_ShouldReturnAmount() (gas: 0 (0.000%)) testDepositMultipleTokens_ShouldRevert_IfAmountsIsNotPositive() (gas: 0 (0.000%)) testDepositMultipleTokens_ShouldRevert_IfDurationIsNotValid() (gas: 0 (0.000%)) testDeposit_ShouldRevert_IfAmountIsNotPositive() (gas: 0 (0.000%)) testDeposit_ShouldRevert_IfDurationIsNotValid() (gas: 0 (0.000%)) testDeposit_ShouldRevert_IfTokenIsNotInMetapool() (gas: 0 (0.000%)) testIsMetaPoolCoinReturnFalseIfTokenAddressIsNotInMetaPool() (gas: 0 (0.000%)) testIsMetaPoolCoinReturnTrueIfToken0IsPassed() (gas: 0 (0.000%)) testIsMetaPoolCoinReturnTrueIfToken1IsPassed() (gas: 0 (0.000%)) testIsMetaPoolCoinReturnTrueIfToken2IsPassed() (gas: 0 (0.000%)) testIsMetaPoolCoinReturnTrueIfUbiquityDollarTokenIsPassed() (gas: 0 (0.000%)) testWithdrawShouldRevertIfTokenIsNotInMetaPool() (gas: 0 (0.000%)) test_IsMetaPoolCoin() (gas: 0 (0.000%)) test_Should_DebugMockCall_And_Return_Selector() (gas: 0 (0.000%)) test_getDollarsToMintRevertsIfPriceLowerThan1USD() (gas: 0 (0.000%)) test_getDollarsToMintWorks() (gas: 0 (0.000%)) testCanCallGeneralFunctions_ShouldSucceed() (gas: 0 (0.000%)) testInitializeDollarTokenAddress_ShouldSucceed() (gas: 0 (0.000%)) testSetDollarTokenAddress_ShouldSucceed() (gas: 0 (0.000%)) testSetMinterRoleWhenInitializing_ShouldSucceed() (gas: 0 (0.000%)) testSetTwapOracleAddress_ShouldSucceed() (gas: 0 (0.000%)) testTransferOwnership_ShouldRevertWhenNotOwner() (gas: 0 (0.000%)) testTransferOwnership_ShouldWorkWhenNewOwnerIsNotContract() (gas: 0 (0.000%)) testCannotGetRewardsOtherAccount() (gas: 0 (0.000%)) testGetStakingShareInfo() (gas: 0 (0.000%)) testPendingGovernance(uint256) (gas: 0 (0.000%)) testTotalShares() (gas: 0 (0.000%)) testDurationMultiply_ShouldReturnAmount() (gas: 0 (0.000%)) test_correctedAmountToWithdraw_calcSharedAmount() (gas: 0 (0.000%)) test_correctedAmountToWithdraw_returnAmount() (gas: 0 (0.000%)) test_lpRewardsAddLiquidityNormalization((address,uint256,uint256,uint256,uint256,uint256),uint256[2],uint256) (gas: 0 (0.000%)) test_lpRewardsRemoveLiquidityNormalization((address,uint256,uint256,uint256,uint256,uint256),uint256[2],uint256) (gas: 0 (0.000%)) test_sharesForLP() (gas: 0 (0.000%)) test_overall() (gas: 0 (0.000%)) testAddCollateralToken_ShouldAddNewTokenAsCollateral() (gas: 0 (0.000%)) testAllCollaterals_ShouldReturnAllCollateralTokenAddresses() (gas: 0 (0.000%)) testCollateralInformation_ShouldReturnCollateralInformation() (gas: 0 (0.000%)) testGetDollarInCollateral_ShouldReturnAmountOfDollarsWhichShouldBeMintedForInputCollateral() (gas: 0 (0.000%)) testGetDollarPriceUsd_ShouldReturnDollarPriceInUsd() (gas: 0 (0.000%)) testUpdateChainLinkCollateralPrice_ShouldRevert_IfChainlinkAnswerIsInvalid() (gas: 0 (0.000%)) testUpdateChainLinkCollateralPrice_ShouldRevert_IfChainlinkAnswerIsStale() (gas: 0 (0.000%)) testUpdateChainLinkCollateralPrice_ShouldUpdateCollateralPrice() (gas: 0 (0.000%)) testGetStake_ShouldReturnStake() (gas: 0 (0.000%)) testSafeBatchTransferFrom_ShouldTransferTokenIds() (gas: 0 (0.000%)) testSafeTransferFrom_ShouldRevert_IfInsufficientBalance() (gas: 0 (0.000%)) testSafeTransferFrom_ShouldRevert_IfToAddressZero() (gas: 0 (0.000%)) testTotalSupply_ShouldReturn_TotalSupply() (gas: 0 (0.000%)) testSetIncentiveContract_ShouldRevert_IfNotAdmin() (gas: 0 (0.000%)) testBondsCount_ShouldReturnCorrectCount() (gas: 0 (0.000%)) testClaimBond_ShouldEmitLogClaimAndClaimBondRewards(uint256) (gas: 0 (0.000%)) testClaim_ShouldClaimRewards(uint256) (gas: 0 (0.000%)) testRewardsBondOf_ShouldReturnCorrectValues(uint256,uint256) (gas: 0 (0.000%)) testRewardsOf_ShouldReturnRewardsOfBond(uint256) (gas: 0 (0.000%)) testWithdraw_ShouldWithdrawBondTokens(uint256) (gas: 0 (0.000%)) testSetSticker_ShouldSetSticker() (gas: 0 (0.000%)) testSetVestingBlocks_ShouldSetVestingBlocks(uint256) (gas: 0 (0.000%)) testApprove_ShouldRevert_IfOperatorIsNotAllowed() (gas: 0 (0.000%)) testBeforeConsecutiveTokenTransfer_ShouldRevert() (gas: 0 (0.000%)) testConstructor_ShouldInitContract() (gas: 0 (0.000%)) testRandom_ShouldReturnRandomValue() (gas: 0 (0.000%)) testSetMinter_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testSetMinter_ShouldUpdateMinter() (gas: 0 (0.000%)) testSupportsInterface_ShouldReturnFalse_IfInterfaceIsNotSupported() (gas: 0 (0.000%)) testSupportsInterface_ShouldReturnTrue_IfInterfaceIsSupported() (gas: 0 (0.000%)) testTokenURI_ShouldRevert_IfTokenDoesNotExist() (gas: 0 (0.000%)) testBatchSetAllowance_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testReceive_ShouldRevert_IfAllowanceIsInsufficient() (gas: 0 (0.000%)) testSetAllowance_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testSetFundsAddress_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testSetTokenContract_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testWithdraw_ShouldRevert_IfCalledNotByOwner() (gas: 0 (0.000%)) testDeployStableSwapPool_ShouldSucceed() (gas: 48 (0.001%)) test_setPoolRevertsWhenFirstAddressIsNotDollarToken() (gas: 24 (0.002%)) testSafeTransferFrom_ShouldTransferTokenId() (gas: 5 (0.003%)) testCollateralUsdBalance_ShouldReturnTotalAmountOfCollateralInUsd() (gas: 8 (0.003%)) testMint_ShouldMint(uint128,uint128,uint256) (gas: 13 (0.004%)) testCollectRedemption_ShouldRevert_IfNotEnoughBlocksHaveBeenMined() (gas: -1 (-0.004%)) testDeposit_Staking(uint256,uint256) (gas: 24 (0.005%)) testIsIdIncludedReturnFalseIfIdIsNotInTheList() (gas: -15 (-0.006%)) testBond(uint256) (gas: 13 (0.006%)) testSetRatePerBlock_Default() (gas: 4 (0.006%)) testLockupMultiplier() (gas: 44 (0.006%)) testSetBaseUri_ShouldSetUri() (gas: 24 (0.006%)) testSetManager_ShouldSetDiamond() (gas: 4 (0.009%)) testSetUri_ShouldSetUri() (gas: 24 (0.009%)) test_redeemCreditNftRevertsIfNotEnoughBalance() (gas: 37 (0.009%)) testWithdrawMultiple_ShouldWithdraw() (gas: 64 (0.011%)) testSafeTransferFromWith4Params_ShouldTransferToken() (gas: 19 (0.011%)) testTransferFrom_ShouldTransferToken() (gas: 19 (0.011%)) testDepositMultipleTokens_ShouldDepositTokens() (gas: 58 (0.011%)) testSafeTransferFrom_ShouldTransferToken() (gas: 20 (0.011%)) test_redeemCreditNftRevertsIfZeroAmountOfDollars() (gas: 61 (0.012%)) testSetUriSingle_ShouldSetUri() (gas: 24 (0.013%)) test_getCreditNftAmount() (gas: 37 (0.013%)) test_redeemCreditNftRevertsIfNotEnoughDollars() (gas: 85 (0.013%)) testAllowance_ShouldReturnAllowance() (gas: 19 (0.013%)) test_getCreditNftAmount_revertsIfDebtTooHigh() (gas: 37 (0.014%)) testTransferFrom_ShouldTransferTokensFromAddress() (gas: 19 (0.014%)) testApprove_ShouldApproveAddressToTransferTokens() (gas: 19 (0.014%)) testSafeTransferFromWith4Params_ShouldRevert_IfOperatorIsNotAllowed() (gas: 24 (0.014%)) testSafeTransferFrom_ShouldRevert_IfOperatorIsNotAllowed() (gas: 24 (0.014%)) testTransferFrom_ShouldRevert_IfOperatorIsNotAllowed() (gas: 24 (0.014%)) testRenounceRole_ShouldWork() (gas: 9 (0.014%)) testUpdateStake_ShouldUpdateStake(uint128,uint128,uint256) (gas: 24 (0.014%)) test_burnExpiredCreditNftForGovernanceRevertsIfNotEnoughBalance() (gas: 37 (0.014%)) testApprove_ShouldApprove() (gas: 24 (0.014%)) testSetApprovalForAll_ShouldApproveOperatorToSpendAllTokens() (gas: 24 (0.014%)) testMintCreditNft_ShouldMintCreditNft() (gas: 37 (0.014%)) testDepositFromZeroState(uint256) (gas: 68 (0.015%)) testBurnCreditNft_ShouldBurnCreditNft() (gas: 45 (0.015%)) test_redeemCreditNftWorks() (gas: 93 (0.016%)) testSetParamsShouldRevertNotAdmin() (gas: 4 (0.016%)) test_mintClaimableDollars() (gas: 24 (0.017%)) testSetApprovalForAll_ShouldRevert_IfOperatorIsNotAllowed() (gas: 24 (0.017%)) testSafeMint_ShouldMint() (gas: 24 (0.017%)) test_burnCreditNftForCreditWorks() (gas: 69 (0.017%)) test_burnExpiredCreditNftForGovernanceWorks() (gas: 69 (0.018%)) testGetTotalOutstandingDebt_ReturnTotalDebt() (gas: 89 (0.018%)) test_exchangeDollarsForCreditNft() (gas: 85 (0.018%)) testUpdateTotalDebt_ShouldUpdateTotalDebt() (gas: 89 (0.019%)) testGetRewards(uint256) (gas: 48 (0.019%)) testGetRewards(uint256) (gas: 48 (0.019%)) test_WithdrawShouldWithdraw() (gas: 56 (0.020%)) testSafeMint_ShouldMintGoldToken() (gas: 1392 (0.020%)) testTransfer_ShouldTransferTokens() (gas: 24 (0.020%)) testTokenURI_ShouldReturnTokenURIForTokenTypeGold() (gas: 1416 (0.021%)) testIsIdIncludedReturnTrueIfIdIsInTheList() (gas: 56 (0.021%)) testTokenURI_ShouldReturnTokenURIForTokenTypeInvisible() (gas: 1032 (0.021%)) testToggleMintRedeemBorrow_ShouldToggleRedeeming() (gas: 24 (0.021%)) testToggleMintRedeemBorrow_ShouldToggleBorrowingByAmoMinter() (gas: 24 (0.021%)) testToggleMintRedeemBorrow_ShouldToggleMinting() (gas: 24 (0.021%)) testDeposit_ShouldDepositTokens() (gas: 48 (0.021%)) testRemoveLiquidity(uint256,uint256) (gas: 97 (0.022%)) testSetGovernanceShareForTreasury_ShouldRevertWhenNotAdmin() (gas: 4 (0.022%)) testSetMinPriceDiffToUpdateMultiplier_ShouldRevertWhenNotAdmin() (gas: 4 (0.022%)) testDeposit(uint32,uint256) (gas: 48 (0.022%)) testRemoveLiquidity(uint256,uint256) (gas: 103 (0.023%)) testSetCollateralChainLinkPriceFeed_ShouldSetPriceFeed() (gas: 24 (0.023%)) testGetCreditAmount_ShouldRevert_IfDebtIsTooHigh() (gas: -11 (-0.023%)) testTransfer_ShouldCallIncentivize_IfValidTransfer() (gas: 72 (0.025%)) testSetPoolCeiling_ShouldSetMaxAmountOfTokensAllowedForCollateral() (gas: 24 (0.025%)) testBatchSafeMint_ShouldMintMultipleTokens() (gas: -64 (-0.025%)) test_exchangeDollarsForCreditWorks() (gas: 72 (0.026%)) testToggleCollateral_ShouldToggleCollateral() (gas: 24 (0.026%)) testRaiseCapital_ShouldMintTokens() (gas: 24 (0.026%)) testBurn_ShouldBurnTokens() (gas: 24 (0.026%)) testMint_ShouldMintTokens() (gas: 24 (0.028%)) testTokenURI_ShouldReturnTokenURIForTokenTypeStandard() (gas: 48 (0.028%)) test_ShouldMint() (gas: 24 (0.030%)) testSetParams(uint32,uint256) (gas: 24 (0.030%)) testAmoMinterBorrow_ShouldBorrowCollateral() (gas: 23 (0.030%)) testBurnFrom_ShouldBurnTokensFromAddress() (gas: 48 (0.031%)) testSetManager_ShouldRevert_WhenNotAdmin() (gas: -5 (-0.031%)) testSetIncentiveToDollar_ShouldSucceed() (gas: 24 (0.031%)) test_burnCreditTokensForDollarsWorks() (gas: 72 (0.031%)) testMintDollar_ShouldMintDollars() (gas: 76 (0.032%)) testSetRatePerBlock_ShouldRevert_WhenNotAdmin() (gas: 6 (0.036%)) testCollectRedemption_ShouldCollectRedemption() (gas: 102 (0.036%)) testSafeBatchTransferFrom_ShouldRevert_IfPaused() (gas: 24 (0.037%)) testReceive_ShouldRevert_IfMaxSupplyIsReached() (gas: -45003 (-0.038%)) testSendDust_ShouldWork() (gas: 24 (0.039%)) testTransferFrom_ShouldRevert_IfContractIsPaused() (gas: 58 (0.040%)) testCannotStakeMoreThan4Years(uint256) (gas: 9 (0.040%)) testBurnFrom_ShouldRevert_IfContractIsPaused() (gas: 72 (0.041%)) testSafeTransferFrom_ShouldRevert_IfPaused() (gas: 24 (0.041%)) testRedeemDollar_ShouldRevert_OnCollateralSlippage() (gas: 106 (0.042%)) testSendDust_ShouldRevertWhenPartOfTheProtocol() (gas: 72 (0.042%)) testWithdraw_ShouldWithdraw() (gas: 29 (0.042%)) testMintDollar_ShouldRevert_OnReachingPoolCeiling() (gas: 54 (0.042%)) testRedeemDollar_ShouldRevert_OnInsufficientPoolCollateral() (gas: 54 (0.043%)) testTransfer_ShouldRevert_IfContractIsPaused() (gas: 24 (0.043%)) testRedeemDollar_ShouldRedeemCollateral() (gas: 128 (0.043%)) testFreeCollateralBalance_ShouldReturnCollateralAmountAvailableForBorrowingByAmoMinters() (gas: 128 (0.043%)) testPause_ShouldPauseContract() (gas: 24 (0.044%)) testSendDust_ShouldWorkWhenNotPartOfTheProtocol() (gas: 77 (0.044%)) testBurn_ShouldRevert_IfContractIsPaused() (gas: 24 (0.045%)) testCollectRedemption_ShouldRevert_IfRedeemingIsPaused() (gas: 23 (0.045%)) testSetConstant_ShouldUpdateCoef() (gas: 12 (0.045%)) testWithdraw(uint32,uint256) (gas: 96 (0.047%)) testMintDollar_ShouldRevert_OnCollateralAmountSlippage() (gas: 54 (0.047%)) testMintDollar_ShouldRevert_OnDollarAmountSlippage() (gas: 54 (0.047%)) testAllowance_ShouldReturnAllowance() (gas: 29 (0.048%)) testSetAllowance_ShouldSetAllowance() (gas: 29 (0.048%)) testWithdrawShouldRevertIfSenderIsNotBondOwner() (gas: 32 (0.049%)) testSetExcessDollarsDistributor_ShouldSucceed() (gas: 24 (0.049%)) testCannotDepositZeroWeeks() (gas: 9 (0.049%)) testSetUri_ShouldRevert_IfGovernanceTokenNotStakingManager() (gas: 15 (0.051%)) testMintDollar_ShouldRevert_IfDollarPriceUsdIsTooLow() (gas: 30 (0.051%)) testRedeemDollar_ShouldRevert_IfDollarPriceUsdIsTooHigh() (gas: 30 (0.051%)) testUpdateStake_ShouldRevert_IfNotMinter(uint128,uint128,uint256) (gas: 15 (0.051%)) testSetSushiSwapPoolAddress_ShouldSucceed() (gas: 24 (0.052%)) testSetFormulasAddress_ShouldSucceed() (gas: 24 (0.052%)) testSetStableSwapMetaPoolAddress_ShouldSucceed() (gas: 24 (0.052%)) testSetMasterChefAddress_ShouldSucceed() (gas: 24 (0.052%)) testSetStakingContractAddress_ShouldSucceed() (gas: 24 (0.052%)) testSetDollarMintCalculatorAddress_ShouldSucceed() (gas: 24 (0.052%)) testFails_distributeDollarsWorks() (gas: 39 (0.053%)) testMint_ShouldRevert_IfCalledNotByTheMinterRole() (gas: 15 (0.053%)) testBurnFrom_ShouldRevert_IfCalledNotByTheBurnerRole() (gas: 15 (0.054%)) testSetManager_ShouldRevert_WhenNotAdmin() (gas: 15 (0.056%)) testSetSymbol_ShouldRevert_IfMethodIsCalledNotByAdmin() (gas: 15 (0.056%)) testSetManager_ShouldRevert_WhenNotAdmin() (gas: 15 (0.056%)) testSetManager_ShouldRevert_WhenNotAdmin() (gas: 15 (0.056%)) testSetManager_ShouldRevert_WhenNotAdmin() (gas: 15 (0.056%)) testPause_ShouldRevert_IfCalledNotByThePauserRole() (gas: 15 (0.057%)) testMint_ShouldRevert_IfNotMinter(uint128,uint128,uint256) (gas: 18 (0.062%)) testMint_ShouldRevert_IfMintingToZeroAddress(uint128,uint128,uint256) (gas: 18 (0.062%)) testWithdrawMultiple_ShouldRevert_IfSenderIsNotBondOwner() (gas: 32 (0.062%)) testSetSymbol_ShouldSetSymbol() (gas: 24 (0.064%)) testSetFees_ShouldSetMintAndRedeemFees() (gas: 24 (0.066%)) testSetExemptAddress_ShouldRevertOrSet_IfAdmin() (gas: 39 (0.067%)) testUpdateStake_ShouldRevert_IfPaused(uint128,uint128,uint256) (gas: 39 (0.067%)) testUnpause_ShouldRevert_IfCalledNotByThePauserRole() (gas: 39 (0.068%)) testCollateralInformation_ShouldRevert_IfCollateralIsDisabled() (gas: 24 (0.070%)) testSwitchBuyIncentive_ShouldRevertOrSwitch_IfAdmin() (gas: 39 (0.072%)) testMint_ShouldRevert_IfPaused(uint128,uint128,uint256) (gas: 42 (0.072%)) testSwitchSellPenalty_ShouldRevertOrSwitch_IfAdmin() (gas: 39 (0.072%)) testSetManager_ShouldSetDiamond() (gas: 24 (0.073%)) testSetRewards_ShouldSetRewards(uint256) (gas: 29 (0.073%)) testSetManager_ShouldSetManager() (gas: 24 (0.073%)) testSetManager_ShouldSetManager() (gas: 24 (0.073%)) testSetManager_ShouldSetManager() (gas: 24 (0.073%)) testBond_ShouldRevert_IfTokenNotAllowed() (gas: 19 (0.076%)) testSetTokenContract_ShouldSetTokenContract() (gas: 29 (0.076%)) testSetConstant_ShouldRevert_IfCalledNotByAdmin() (gas: 14 (0.076%)) testGetRate_ShouldRevert_WhenBlockIsInThePast() (gas: -11 (-0.078%)) testBurnCreditNft_ShouldRevert_WhenNotCreditNftManager() (gas: -21 (-0.079%)) testMintCreditNft_ShouldRevert_WhenNotCreditNftManager() (gas: -21 (-0.079%)) testMint_ShouldRevert_IfContractIsPaused() (gas: 48 (0.081%)) testSetFundsAddress_ShouldSetFundsAddress() (gas: 29 (0.081%)) testSetGovernancePerBlock(uint256) (gas: 24 (0.081%)) testSetGovernancePerBlock_ShouldRevertWhenNotAdmin() (gas: 15 (0.081%)) testSetMinPriceDiff(uint256) (gas: 24 (0.081%)) testOnlyAmoMinter_ShouldRevert_IfCalledNoByAmoMinter() (gas: 15 (0.082%)) test_setCreditNftLength() (gas: 28 (0.082%)) testSetStakingDiscountMultiplier(uint256) (gas: 24 (0.082%)) testSetBlockCountInAWeek(uint256) (gas: 24 (0.082%)) testSetCreditNftAddress_ShouldSucceed() (gas: 24 (0.082%)) testSetCreditTokenAddress_ShouldSucceed() (gas: 24 (0.082%)) testSetStakingShareAddress_ShouldSucceed() (gas: 24 (0.082%)) testSetTreasuryAddress_ShouldSucceed() (gas: 24 (0.082%)) testSetGovernanceTokenAddress_ShouldSucceed() (gas: 24 (0.082%)) testAddAmoMinter_ShouldAddAmoMinter() (gas: 24 (0.084%)) testCutFacetAddWriteFacetWithInitializer() (gas: -194 (-0.086%)) testSetGovernanceDiv(uint256) (gas: 24 (0.090%)) testAmoMinterBorrow_ShouldRevert_IfBorrowingIsPaused() (gas: 49 (0.091%)) testSetPriceThresholds_ShouldSetPriceThresholds() (gas: 24 (0.092%)) testIncentivizeShouldRevertWhenCallerNotUAD() (gas: 15 (0.093%)) testRedeemDollar_ShouldRevert_IfRedeemingIsPaused() (gas: 54 (0.093%)) testMintDollar_ShouldRevert_IfMintingIsPaused() (gas: 54 (0.093%)) testCutFacetAddSimpleWriteFacet() (gas: -194 (-0.099%)) testRemoveAmoMinter_ShouldRemoveAmoMinter() (gas: 24 (0.101%)) testSetRedemptionDelayBlocks_ShouldSetRedemptionDelayInBlocks() (gas: 24 (0.103%)) testBatchSetAllowance_ShouldBatchSetAllowance() (gas: -116 (-0.103%)) test_setExpiredCreditNftConversionRate() (gas: 39 (0.106%)) testAddAmoMinter_ShouldRevert_IfAmoMinterHasInvalidInterface() (gas: 24 (0.109%)) testCutFacetAddSimplePureFacet() (gas: -194 (-0.111%)) testCutFacetReplaceFacet() (gas: -310 (-0.111%)) testPause_ShouldPause() (gas: 48 (0.112%)) testSafeMint_ShouldRevert_IfCalledNotByMinter() (gas: 15 (0.113%)) testBatchSafeMint_ShouldRevert_IfCalledNotByMinter() (gas: 15 (0.113%)) testIncentivizeSell() (gas: 192 (0.124%)) testAddAmoMinter_ShouldRevert_IfAmoMinterIsZeroAddress() (gas: 24 (0.127%)) testAmoMinterBorrow_ShouldRevert_IfCollateralIsDisabled() (gas: 49 (0.129%)) testIncentivizeShouldRevertIfSenderEqualToReceiver() (gas: 24 (0.129%)) testCollateralEnabled_ShouldRevert_IfCollateralIsDisabled() (gas: 42 (0.135%)) testSetAllowance_ShouldRevert_IfAddressIsZero() (gas: 18 (0.137%)) testSetTokenContract_ShouldRevert_IfAddressIsZero() (gas: 18 (0.137%)) testSetFundsAddress_ShouldRevert_IfAddressIsZero() (gas: 18 (0.138%)) testCutFacetShouldRevertWhenFacetInitializerReverts() (gas: -257 (-0.140%)) testCutFacetShouldRevertWithMessageWhenFacetInitializerWithMessageReverts() (gas: -257 (-0.140%)) testSetTreasury_ShouldSetTreasury() (gas: 29 (0.154%)) testUnpause_ShouldRevert_IfNotPauser() (gas: 87 (0.174%)) testIncentivizeBuy() (gas: 168 (0.176%)) testCutFacetRemoveFacetFunctions() (gas: -323 (-0.191%)) testPause_ShouldRevert_IfNotPauser() (gas: 39 (0.191%)) testSetTokenURI_ShouldRevert_IfCalledNotByMinter() (gas: 26 (0.192%)) testUnpause_ShouldUnpauseContract() (gas: 77 (0.208%)) testUnpause_ShouldUnpause() (gas: 76 (0.221%)) testTransferOwnership_ShouldRevertWhenNewOwnerIsZeroAddress() (gas: -124 (-0.522%)) test_NonReentrant() (gas: -12833 (-0.579%)) testUUPS_AdminAuth() (gas: -133337 (-4.029%)) testUUPS_initialization() (gas: -133352 (-4.034%)) testUUPS_ShouldUpgradeAndCall() (gas: -133352 (-4.038%)) testUUPS_ImplChanges() (gas: -133352 (-4.052%)) testUUPS_InitializedVersion() (gas: -266599 (-4.069%)) testUUPS_initialization() (gas: -133344 (-4.377%)) testUUPS_AdminAuth() (gas: -133329 (-4.385%)) testUUPS_ShouldUpgradeAndCall() (gas: -133344 (-4.397%)) testUUPS_ImplChanges() (gas: -133344 (-4.410%)) testUUPS_InitializedVersion() (gas: -266587 (-4.433%)) testUUPS_AdminAuth() (gas: -133328 (-4.463%)) testUUPS_initialization() (gas: -133343 (-4.470%)) testUUPS_ShouldUpgradeAndCall() (gas: -133343 (-4.474%)) testUUPS_ImplChanges() (gas: -133343 (-4.492%)) testUUPS_InitializedVersion() (gas: -266586 (-4.512%)) testUUPS_initialization() (gas: -284819 (-6.692%)) testUUPS_AdminAuth() (gas: -284804 (-6.699%)) testUUPS_ShouldUpgradeAndCall() (gas: -284819 (-6.711%)) testUUPS_ImplChanges() (gas: -284819 (-6.729%)) testUUPS_InitializedVersion() (gas: -569391 (-6.750%)) testUUPS_initialization() (gas: -409195 (-9.259%)) testUUPS_AdminAuth() (gas: -409180 (-9.272%)) testUUPS_ShouldUpgradeAndCall() (gas: -409195 (-9.285%)) testUUPS_ImplChanges() (gas: -409195 (-9.312%)) testUUPS_InitializedVersion() (gas: -818035 (-9.340%))

Total gas saving:

Overall gas change: -6612522 (-2.292%)

0xRizwan · 2024-02-14T05:36:18Z

Look like you need to format your code. Also by "custom error codes" I thought you meant like how Gnosis Safe does it, like "GS018" and then the user goes to their documentation to see the full description of the error which makes a ton of sense to me.

Imo, This affects user experience in searching error code meaning. require used with return string message adds more readability and easy to understand for a normal user.

0x4007 · 2024-02-14T07:14:17Z

require used with return string message adds more readability and easy to understand for a normal user.

In case of require, i see the string message length was more than 32 bytes at some instances which means additional 1 slot is required in such case. ¹

Hard disagree if we need to explain within 32 characters vs unlimited character length hosted on our website with examples and diagrams if necessary.

https://github.com/ubiquity/ubiquity-dollar/issues/895#issuecomment-1940414273 ↩

rndquu

I understand that @0xRizwan came from the Sherlock's audit and has context of the project hence he decided to create this PR what we really appreciate.
Gas optimizations is literally the last step to apply in the project. We have tons of other high priority things to do (one, two).
This is my personal opinion, but we already have a comlicated project with all of the upgradeable contracts, diamond, facets, etc... Gas optimizations in most cases decrease readability.

In my opinion such refactors (using internal function in a modifier) decrease readability:

modifier onlyDollarManager() {
        _onlyDollarManager();
        _;
}

function _onlyDollarManager() internal view {
        require(
            LibAccessControl.hasRole(CURVE_DOLLAR_MANAGER_ROLE, msg.sender),
            "CurveIncentive: Caller is not Ubiquity Dollar"
);
    }

I also don't really like using the unchecked because every time you see it in the code you should think "can it over/under flow or not in some cases". This PR intoduces quite a lot of such blocks.

As far as I understand, if we aim for Sherlock's coverage we are not allowed to merge such changes in the development branch. The best we can do is to maintain some separate branch like feat/gas-optimization and merge it later.

Overall this 3% gas savings decrease readability and add a headache with the unchecked block. I would close this issue as not planned and get back to it on the final project stage.

@gitcoindev @molecula451 Help

molecula451 · 2024-02-14T08:07:32Z

Yeah i think the PR won't make it either as the such refactors should be considered with prior conversation among core team members (best approach, just like #889), but the intention contribution worth it and give a bit of insight, i think we could perhaps compensate the user for these comment and the PR effort a bit and mark this as not planned and close it

my best thoughts

gitcoindev · 2024-02-14T12:35:14Z

Hi everyone, I am back. I analysed the changes. Now I see where the pull request comes from the optimization boils down to three kinds of refactoring:

refactoring requires out of modifiers into private internal functions
loops with unchecked {} variable incrementation, this was weird for me, but indeed it is safe from solc 0.8.0 onwards. I suspect this will be optimized in compiler itself soon
shorter require messages

Thank you as well for posting the detailed results. Overall it seems that most of the gas savings are detected in testUUPS_* tests , and for the diamond part the gas savings are negligible. I agree with @molecula451 that perhaps we could think of compensation for the effort , but I would not merge this as is. At least now, perhaps consider this and other gas saving optimizations in a later stage of the project and leave the current state on a separate branch as @rndquu suggested. I found a quite good article on possible optimizations that also gives other options that could be applied:
https://0xmacro.com/blog/solidity-gas-optimizations-cheat-sheet/

0x4007 · 2024-02-14T19:31:43Z

i think we could perhaps compensate the user for these comment and the PR effort a bit and mark this as not planned and close it

If the bot decides to work as designed, then we should be able to unassign 0xRizwan and close the issue as complete. This should generate payment permits for all on topic comments, as well as generate a larger reward for authoring the issue specification.

molecula451 · 2024-02-14T19:47:52Z

it looks like it took action on the PR closing but not on the issue #895 (comment)

0x4007 · 2024-02-14T20:15:00Z

Yes unfortunately the permit flow is a coin toss right now. I think it works about half the time. Check https://github.com/ubiquibot/comment-incentives/actions for the status on these runs

The most prominent problem is specified here:

65536 GitHub Comment Character Limit ubiquibot/comment-incentives#19

0xRizwan added 2 commits February 13, 2024 17:11

gas optimization

de61f09

gas optimization

e24ddbd

0xRizwan mentioned this pull request Feb 13, 2024

Gas optimization for Ubiquity contracts #895

Closed

0xRizwan added 2 commits February 13, 2024 20:21

fix: gas optimization

6b48390

fix: gas optimization

fcd0980

molecula451 requested review from rndquu, gitcoindev and molecula451 February 13, 2024 15:27

0x4007 reviewed Feb 14, 2024

View reviewed changes

rndquu reviewed Feb 14, 2024

View reviewed changes

ubiquibot bot closed this Feb 14, 2024

molecula451 mentioned this pull request Feb 15, 2024

Permit generated to wrong address ubiquibot/comment-incentives#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas optimization #896

Gas optimization #896

0xRizwan commented Feb 13, 2024

molecula451 commented Feb 13, 2024

0xRizwan commented Feb 14, 2024

0x4007 left a comment

0x4007 Feb 14, 2024

molecula451 Feb 14, 2024 •

edited

Loading

0xRizwan Feb 14, 2024

molecula451 Feb 14, 2024

0xRizwan Feb 14, 2024

molecula451 Feb 14, 2024

0xRizwan Feb 14, 2024

0xRizwan commented Feb 14, 2024

0x4007 commented Feb 14, 2024

rndquu left a comment •

edited

Loading

molecula451 commented Feb 14, 2024 •

edited

Loading

gitcoindev commented Feb 14, 2024

0x4007 commented Feb 14, 2024 •

edited

Loading

molecula451 commented Feb 14, 2024

0x4007 commented Feb 14, 2024 •

edited

Loading

Gas optimization #896

Gas optimization #896

Conversation

0xRizwan commented Feb 13, 2024

molecula451 commented Feb 13, 2024

0xRizwan commented Feb 14, 2024

0x4007 left a comment

Choose a reason for hiding this comment

0x4007 Feb 14, 2024

Choose a reason for hiding this comment

molecula451 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

0xRizwan Feb 14, 2024

Choose a reason for hiding this comment

molecula451 Feb 14, 2024

Choose a reason for hiding this comment

0xRizwan Feb 14, 2024

Choose a reason for hiding this comment

molecula451 Feb 14, 2024

Choose a reason for hiding this comment

0xRizwan Feb 14, 2024

Choose a reason for hiding this comment

0xRizwan commented Feb 14, 2024

0x4007 commented Feb 14, 2024

Footnotes

rndquu left a comment • edited Loading

Choose a reason for hiding this comment

molecula451 commented Feb 14, 2024 • edited Loading

gitcoindev commented Feb 14, 2024

0x4007 commented Feb 14, 2024 • edited Loading

molecula451 commented Feb 14, 2024

0x4007 commented Feb 14, 2024 • edited Loading

molecula451 Feb 14, 2024 •

edited

Loading

rndquu left a comment •

edited

Loading

molecula451 commented Feb 14, 2024 •

edited

Loading

0x4007 commented Feb 14, 2024 •

edited

Loading

0x4007 commented Feb 14, 2024 •

edited

Loading