-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
VRF V2.5 gas optimisations #11932
VRF V2.5 gas optimisations #11932
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
Go solidity wrappers are out-of-date, regenerate them via the |
Go solidity wrappers are out-of-date, regenerate them via the |
21730ac
to
a9ebb68
Compare
Go solidity wrappers are out-of-date, regenerate them via the |
LinkTokenInterface public LINK; | ||
// s_totalBalance tracks the total link sent to/from | ||
// this contract through onTokenTransfer, cancelSubscription and oracleWithdraw. | ||
// A discrepancy with this contract's link balance indicates someone | ||
// sent tokens using transfer and so we may need to use recoverFunds. | ||
uint96 public s_totalBalance; | ||
|
||
/// @dev may not be provided upon construction on some chains due to lack of availability | ||
AggregatorV3Interface public LINK_NATIVE_FEED; | ||
// s_totalNativeBalance tracks the total native sent to/from | ||
// this contract through fundSubscription, cancelSubscription and oracleWithdrawNative. | ||
// A discrepancy with this contract's native balance indicates someone | ||
// sent native using transfer and so we may need to use recoverNativeFunds. | ||
uint96 public s_totalNativeBalance; | ||
|
||
uint96 internal s_withdrawableTokens; | ||
uint96 internal s_withdrawableNative; | ||
// subscription nonce used to construct subId. Rises monotonically | ||
uint64 public s_currentSubNonce; |
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.
as discussed offline, I think this results in gas optimization for other code paths but not hot paths (request/fulfillment)
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.
Ah VRF doesn't have Foundry tests? A quick snapshot would give you gas for every path :D (yes I'm trying to convert you, the last project that still used HH!)
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.
Upon running:
$ forge snapshot --gas-report > gas_report.txt
$ git checkout ec1479e07de09340239a8ab1b7a6a80dce22d31f
$ forge snapshot --snap .develop-gas-snapshot
$ forge snapshot --diff .develop-gas-snapshot
Running 2 tests for test/v0.8/foundry/vrf/VRFV2Wrapper_Migration.t.sol:VRFV2PlusWrapperTest
[PASS] testMigrateWrapperLINKPayment() (gas: 540249)
[PASS] testMigrateWrapperNativePayment() (gas: 487257)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 2.58ms
Running 3 tests for test/v0.8/foundry/vrf/VRFV2Wrapper.t.sol:VRFV2PlusWrapperTest
[PASS] testRequestAndFulfillRandomWordsLINKWrapper() (gas: 367113)
[PASS] testRequestAndFulfillRandomWordsNativeWrapper() (gas: 299229)
[PASS] testSetLinkAndLinkNativeFeed() (gas: 3508860)
Test result: ok. 3 passed; 0 failed; 0 skipped; finished in 1.94ms
Running 11 tests for test/v0.8/foundry/vrf/VRFV2Plus.t.sol:VRFV2Plus
[PASS] testCancelSubWithNoLink() (gas: 160553)
[PASS] testCreateSubscription() (gas: 164143)
[PASS] testDeregisterProvingKey() (gas: 86749)
[PASS] testGetActiveSubscriptionIds() (gas: 3460238)
[PASS] testRegisterProvingKey() (gas: 101470)
[PASS] testRequestAndFulfillRandomWordsLINK() (gas: 725068)
[PASS] testRequestAndFulfillRandomWordsNative() (gas: 671454)
[PASS] testRequestAndFulfillRandomWords_NetworkGasPriceExceedsGasLane() (gas: 597450)
[PASS] testRequestAndFulfillRandomWords_OnlyPremium_LinkPayment() (gas: 725502)
[PASS] testRequestAndFulfillRandomWords_OnlyPremium_NativePayment() (gas: 671928)
[PASS] testSetConfig() (gas: 67933)
Test result: ok. 11 passed; 0 failed; 0 skipped; finished in 4.91ms
Running 14 tests for test/v0.8/foundry/vrf/VRFCoordinatorV2Mock.t.sol:VRFCoordinatorV2MockTest
[PASS] testAddConsumer() (gas: 113218)
[PASS] testAddConsumerToInvalidSub() (gas: 16216)
[PASS] testAddMaxConsumers() (gas: 4706289)
[PASS] testCancelSubscription() (gas: 56244)
[PASS] testCreateSubscription() (gas: 92220)
[PASS] testFundInvalidSubscription() (gas: 16206)
[PASS] testFundSubscription() (gas: 70523)
[PASS] testRemoveConsumerAgain() (gas: 94336)
[PASS] testRemoveConsumerFromInvalidSub() (gas: 16183)
[PASS] testRemoveConsumerFromSub() (gas: 92264)
[PASS] testRequestRandomWordsHappyPath() (gas: 242705)
[PASS] testRequestRandomWordsInsufficientFunds() (gas: 160571)
[PASS] testRequestRandomWordsInvalidConsumer() (gas: 70829)
[PASS] testRequestRandomWordsUserOverride() (gas: 229957)
Test result: ok. 14 passed; 0 failed; 0 skipped; finished in 11.60ms
Running 2 tests for test/v0.8/foundry/vrf/TrustedBlockhashStore.t.sol:TrustedBlockhashStoreTest
[PASS] testGenericBHSFunctions() (gas: 53684)
[PASS] testTrustedBHSFunctions() (gas: 99343)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 24.41ms
Running 8 tests for test/v0.8/foundry/vrf/ChainSpecificUtil.t.sol:ChainSpecificUtilTest
[PASS] testGetBlockNumberArbitrum() (gas: 7930)
[PASS] testGetBlockNumberOptimism() (gas: 419)
[PASS] testGetBlockhashArbitrum() (gas: 18896)
[PASS] testGetBlockhashOptimism() (gas: 612)
[PASS] testGetCurrentTxL1GasFeesArbitrum() (gas: 10586)
[PASS] testGetCurrentTxL1GasFeesOptimism() (gas: 39204)
[PASS] testGetL1CalldataGasCostArbitrum() (gas: 12278)
[PASS] testGetL1CalldataGasCostOptimism() (gas: 45883)
Test result: ok. 8 passed; 0 failed; 0 skipped; finished in 12.26ms
Running 2 tests for test/v0.8/foundry/vrf/TrustedBlockhashStore.t.sol:TrustedBlockhashStoreTest
[PASS] testGenericBHSFunctions() (gas: 53684)
[PASS] testTrustedBHSFunctions() (gas: 99343)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 24.41ms
Running 8 tests for test/v0.8/foundry/vrf/ChainSpecificUtil.t.sol:ChainSpecificUtilTest
[PASS] testGetBlockNumberArbitrum() (gas: 7930)
[PASS] testGetBlockNumberOptimism() (gas: 419)
[PASS] testGetBlockhashArbitrum() (gas: 18896)
[PASS] testGetBlockhashOptimism() (gas: 612)
[PASS] testGetCurrentTxL1GasFeesArbitrum() (gas: 10586)
[PASS] testGetCurrentTxL1GasFeesOptimism() (gas: 39204)
[PASS] testGetL1CalldataGasCostArbitrum() (gas: 12278)
[PASS] testGetL1CalldataGasCostOptimism() (gas: 45883)
Test result: ok. 8 passed; 0 failed; 0 skipped; finished in 12.26ms
Running 32 tests for test/v0.8/foundry/vrf/VRFV2PlusSubscriptionAPI.t.sol:VRFV2PlusSubscriptionAPITest
[PASS] testAddConsumer() (gas: 226772)
[PASS] testAddConsumerReaddSameConsumer() (gas: 231240)
[PASS] testAddConsumerTooManyConsumers() (gas: 5402706)
[PASS] testCreateSubscription() (gas: 149655)
[PASS] testCreateSubscriptionRecreate() (gas: 219584) [83/12605]
[PASS] testDefaultState() (gas: 20161)
[PASS] testFundSubscriptionWithNative() (gas: 199347)
[PASS] testFundSubscriptionWithNativeInvalidSubscriptionId() (gas: 26350)
[PASS] testOnTokenTransferCallerNotLink() (gas: 15389)
[PASS] testOnTokenTransferInvalidCalldata() (gas: 324431)
[PASS] testOnTokenTransferInvalidSubscriptionId() (gas: 326632)
[PASS] testOnTokenTransferSuccess() (gas: 484262)
[PASS] testOwnerCancelSubscriptionLinkFundsOnly() (gas: 415139)
[PASS] testOwnerCancelSubscriptionNativeAndLinkFunds() (gas: 452297)
[PASS] testOwnerCancelSubscriptionNativeFundsOnly() (gas: 175888)
[PASS] testOwnerCancelSubscriptionNoFunds() (gas: 126259)
[PASS] testRecoverFundsAmountToTransfer() (gas: 307417)
[PASS] testRecoverFundsBalanceInvariantViolated() (gas: 302392)
[PASS] testRecoverFundsLINKNotSet() (gas: 12856)
[PASS] testRecoverFundsNothingToTransfer() (gas: 487295)
[PASS] testRecoverNativeFundsAmountToTransfer() (gas: 20650)
[PASS] testRecoverNativeFundsBalanceInvariantViolated() (gas: 34603)
[PASS] testRecoverNativeFundsNothingToTransfer() (gas: 204137)
[PASS] testSetLINKAndLINKNativeFeed() (gas: 62660)
[PASS] testSubscriptionOwnershipTransfer() (gas: 160656)
[PASS] testWithdrawInsufficientBalance() (gas: 301593)
[PASS] testWithdrawLinkInvalidOwner() (gas: 19372)
[PASS] testWithdrawNativeInsufficientBalance() (gas: 19622)
[PASS] testWithdrawNativeInvalidOwner() (gas: 19348)
[PASS] testWithdrawNativeSufficientBalance() (gas: 54570)
[PASS] testWithdrawNoLink() (gas: 14997)
[PASS] testWithdrawSufficientBalanceLinkSet() (gas: 341143)
Test result: ok. 32 passed; 0 failed; 0 skipped; finished in 17.97ms
Running 7 tests for test/v0.8/foundry/vrf/VRFCoordinatorV2Plus_Migration.t.sol:VRFCoordinatorV2Plus_Migration
[PASS] testDeregister() (gas: 101372)
[PASS] testMigrateRevertsWhenInvalidCaller() (gas: 33273)
[PASS] testMigrateRevertsWhenInvalidCoordinator() (gas: 19960)
[PASS] testMigrateRevertsWhenPendingFulfillment() (gas: 237917)
[PASS] testMigrateRevertsWhenReentrant() (gas: 358016)
[PASS] testMigration() (gas: 465964)
[PASS] testMigrationNoLink() (gas: 436780)
Test result: ok. 7 passed; 0 failed; 0 skipped; finished in 16.22ms
Test result: ok. 7 passed; 0 failed; 0 skipped; finished in 16.22ms
Ran 8 test suites: 79 tests passed, 0 failed, 0 skipped (79 total tests)
testGetBlockNumberArbitrum() (gas: 0 (0.000%))
testGetBlockNumberOptimism() (gas: 0 (0.000%))
testGetBlockhashArbitrum() (gas: 0 (0.000%))
testGetBlockhashOptimism() (gas: 0 (0.000%)) [39/12605]
testGetCurrentTxL1GasFeesArbitrum() (gas: 0 (0.000%))
testGetCurrentTxL1GasFeesOptimism() (gas: 0 (0.000%))
testGetL1CalldataGasCostArbitrum() (gas: 0 (0.000%))
testGetL1CalldataGasCostOptimism() (gas: 0 (0.000%))
testGenericBHSFunctions() (gas: 0 (0.000%))
testTrustedBHSFunctions() (gas: 0 (0.000%))
testAddConsumer() (gas: 0 (0.000%))
testAddConsumerToInvalidSub() (gas: 0 (0.000%))
testAddMaxConsumers() (gas: 0 (0.000%))
testCancelSubscription() (gas: 0 (0.000%))
testCreateSubscription() (gas: 0 (0.000%))
testFundInvalidSubscription() (gas: 0 (0.000%))
testFundSubscription() (gas: 0 (0.000%))
testRemoveConsumerAgain() (gas: 0 (0.000%))
testRemoveConsumerFromInvalidSub() (gas: 0 (0.000%))
testRemoveConsumerFromSub() (gas: 0 (0.000%))
testRequestRandomWordsHappyPath() (gas: 0 (0.000%))
testRequestRandomWordsInsufficientFunds() (gas: 0 (0.000%))
testRequestRandomWordsInvalidConsumer() (gas: 0 (0.000%))
testRequestRandomWordsUserOverride() (gas: 0 (0.000%))
testDeregister() (gas: 0 (0.000%))
testMigrateRevertsWhenInvalidCaller() (gas: 0 (0.000%))
testMigrateRevertsWhenInvalidCoordinator() (gas: 0 (0.000%))
testRecoverFundsLINKNotSet() (gas: 0 (0.000%))
testWithdrawLinkInvalidOwner() (gas: 0 (0.000%))
testWithdrawNoLink() (gas: 0 (0.000%))
testAddConsumerTooManyConsumers() (gas: 103 (0.002%))
testSetLinkAndLinkNativeFeed() (gas: 103 (0.003%))
testMigrationNoLink() (gas: -14 (-0.003%))
testMigrateRevertsWhenPendingFulfillment() (gas: 22 (0.009%))
testCreateSubscription() (gas: -17 (-0.011%))
testOnTokenTransferInvalidSubscriptionId() (gas: -44 (-0.013%))
testOnTokenTransferInvalidCalldata() (gas: -44 (-0.014%))
testDeregisterProvingKey() (gas: 17 (0.020%))
testWithdrawInsufficientBalance() (gas: -66 (-0.022%))
testMigrateWrapperLINKPayment() (gas: 151 (0.028%))
testMigrateWrapperNativePayment() (gas: 151 (0.031%))
testAddConsumer() (gas: -95 (-0.042%))
testRegisterProvingKey() (gas: 44 (0.043%))
testWithdrawNativeSufficientBalance() (gas: 27 (0.050%))
testMigrateRevertsWhenReentrant() (gas: 197 (0.055%))
testSubscriptionOwnershipTransfer() (gas: -94 (-0.058%))
testRecoverNativeFundsNothingToTransfer() (gas: 158 (0.077%))
testFundSubscriptionWithNative() (gas: 158 (0.079%))
testCancelSubWithNoLink() (gas: 143 (0.089%))
testAddConsumerReaddSameConsumer() (gas: -227 (-0.098%))
testRecoverNativeFundsBalanceInvariantViolated() (gas: 34 (0.098%))
testRecoverNativeFundsAmountToTransfer() (gas: 22 (0.107%))
testOwnerCancelSubscriptionNativeFundsOnly() (gas: 188 (0.107%))
testWithdrawNativeInsufficientBalance() (gas: 22 (0.112%))
testWithdrawNativeInvalidOwner() (gas: 22 (0.114%))
testGetActiveSubscriptionIds() (gas: 4681 (0.135%))
testOnTokenTransferCallerNotLink() (gas: 22 (0.143%))
testCreateSubscriptionRecreate() (gas: 323 (0.147%))
testOwnerCancelSubscriptionNoFunds() (gas: 207 (0.164%))
testFundSubscriptionWithNativeInvalidSubscriptionId() (gas: -45 (-0.170%))
testSetConfig() (gas: -180 (-0.264%))
testSetLINKAndLINKNativeFeed() (gas: -177 (-0.282%))
testOwnerCancelSubscriptionNativeAndLinkFunds() (gas: -1350 (-0.298%))
testRecoverFundsAmountToTransfer() (gas: -2049 (-0.662%))
testMigration() (gas: -11356 (-2.379%))
testRequestAndFulfillRandomWords_NetworkGasPriceExceedsGasLane() (gas: -16909 (-2.752%))
testOwnerCancelSubscriptionLinkFundsOnly() (gas: -17251 (-3.990%))
testRecoverFundsNothingToTransfer() (gas: -21693 (-4.262%))
testOnTokenTransferSuccess() (gas: -21641 (-4.278%))
testRequestAndFulfillRandomWordsLINKWrapper() (gas: -18934 (-4.905%))
testRequestAndFulfillRandomWords_OnlyPremium_LinkPayment() (gas: -40981 (-5.347%))
testRequestAndFulfillRandomWordsLINK() (gas: -40981 (-5.350%))
testRequestAndFulfillRandomWordsNativeWrapper() (gas: -17101 (-5.406%))
testRequestAndFulfillRandomWords_OnlyPremium_NativePayment() (gas: -39141 (-5.505%))
testRequestAndFulfillRandomWordsNative() (gas: -39141 (-5.508%))
testWithdrawSufficientBalanceLinkSet() (gas: -21821 (-6.012%))
testRecoverFundsBalanceInvariantViolated() (gas: -21901 (-6.753%))
testDefaultState() (gas: -2053 (-9.242%))
testCreateSubscription() (gas: -17042 (-9.406%))
Overall gas change: -345553 (-1.078%)
LGTM once David's comment is addressed |
I was flagged as code owner, I think due to the hh config. Could you edit the codeowners to make vrf and automation teams the owner of that file? |
e89e912
to
24757e7
Compare
a1c4339
to
dad63b1
Compare
SonarQube Quality Gate 0 Bugs No Coverage information |
* VRF V2.5 gas optimisations * Minor changes * Removed changes in SubscriptionAPI.sol due to no actual gain in hot paths * Minor changes
* VRF V2.5 gas optimisations (#11932) * VRF V2.5 gas optimisations * Minor changes * Removed changes in SubscriptionAPI.sol due to no actual gain in hot paths * Minor changes * VRF-878 Gas Optimization V2 Plus (#11982) * Optimize deregisterProvingKey * Optimize fulfillRandomWords * Optimize deregisterMigratableCoordinator * Optimize _isTargetRegistered * Optimize pendingRequestExists * Optimize _deleteSubscription * Optimize getActiveSubscriptionIds * Optimize requestRandomWords * Replace post-increment with pre-increment * Optimize _getFeedData * Optimize ownerCancelSubscription * Optimize getSubscription * Optimize createSubscription * Optimize requestSubscriptionOwnerTransfer * Optimize acceptSubscriptionOwnerTransfer * Optimize addConsumer * Update geth wrappers * Remove proving keys length check in pendingRequestExists * Add native payment to RandomWordsFulfilled event (#12085) * Add native payment to RandomWordsFulfilled event * Minor change --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com> --------- Co-authored-by: Sri Kidambi <1702865+kidambisrinivas@users.noreply.github.com> Co-authored-by: Lee Yik Jiun <lee.yikjiun@gmail.com> Co-authored-by: Lee Yik Jiun <yikjiun.lee@smartcontract.com>
* VRF V2.5 gas optimisations * Minor changes * Removed changes in SubscriptionAPI.sol due to no actual gain in hot paths * Minor changes
* VRF V2.5 gas optimisations * Minor changes * Removed changes in SubscriptionAPI.sol due to no actual gain in hot paths * Minor changes
_chargePayment SLOAD optimisation
Gas Savings: 206
$ forge test -vvv --match-path test/v0.8/foundry/vrf/VRFV2Plus.t.sol
requestCommitment optimisation - Struct packing
Gas Savings: 36
SubscriptionAPI.sol struct packing