-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gas optimization #896
Gas optimization #896
Conversation
Please fix the build actions as some are not passing |
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.
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,
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -80,101 +80,149 @@ contract Modifiers { | |||
|
|||
/// @notice Checks that method is called by a contract owner | |||
modifier onlyOwner() { | |||
LibDiamond.enforceIsContractOwner(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a big change is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimizer is already set to true
in config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think #895 (comment) the comment is not enough for a comparison, can you paste us the forge results where edges are optimized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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%)
Imo, This affects user experience in searching error code meaning. |
Hard disagree if we need to explain within 32 characters vs unlimited character length hosted on our website with examples and diagrams if necessary. Footnotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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 likefeat/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
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 |
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:
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: |
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. |
it looks like it took action on the PR closing but not on the issue #895 (comment) |
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: |
Resolves #895