Skip to content
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.

Sign up for GitHub

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

Jump to bottom

VRF-878 Gas Optimization V2 Plus #11982

Merged
merged 18 commits into from
Feb 14, 2024
Merged

VRF-878 Gas Optimization V2 Plus #11982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
244578b
Optimize deregisterProvingKey
leeyikjiun Feb 6, 2024
52a18cf
Optimize fulfillRandomWords
leeyikjiun Feb 6, 2024
c4053f4
Optimize deregisterMigratableCoordinator
leeyikjiun Feb 6, 2024
b0950d9
Optimize _isTargetRegistered
leeyikjiun Feb 6, 2024
3c46fa3
Optimize pendingRequestExists
leeyikjiun Feb 6, 2024
4e7ff95
Optimize _deleteSubscription
leeyikjiun Feb 6, 2024
1cfd44c
Optimize getActiveSubscriptionIds
leeyikjiun Feb 6, 2024
61a9c0e
Optimize requestRandomWords
leeyikjiun Feb 9, 2024
b6a2327
Replace post-increment with pre-increment
leeyikjiun Feb 9, 2024
ab9a4a8
Optimize _getFeedData
leeyikjiun Feb 9, 2024
a977c05
Optimize ownerCancelSubscription
leeyikjiun Feb 9, 2024
06b96c3
Optimize getSubscription
leeyikjiun Feb 9, 2024
271c22b
Optimize createSubscription
leeyikjiun Feb 9, 2024
676913c
Optimize requestSubscriptionOwnerTransfer
leeyikjiun Feb 9, 2024
939a266
Optimize acceptSubscriptionOwnerTransfer
leeyikjiun Feb 9, 2024
4b4ef32
Optimize addConsumer
leeyikjiun Feb 9, 2024
6b50182
Update geth wrappers
leeyikjiun Feb 10, 2024
2fb6ce4
Remove proving keys length check in pendingRequestExists
leeyikjiun Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 38 additions & 27 deletions contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,11 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
* @dev notably can be called even if there are pending requests, outstanding ones may fail onchain
*/
function ownerCancelSubscription(uint256 subId) external onlyOwner {
if (s_subscriptionConfigs[subId].owner == address(0)) {
address owner = s_subscriptionConfigs[subId].owner;
if (owner == address(0)) {
revert InvalidSubscription();
}
_cancelSubscriptionHelper(subId, s_subscriptionConfigs[subId].owner);
_cancelSubscriptionHelper(subId, owner);
}

/**
Expand Down Expand Up @@ -306,14 +307,15 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
override
returns (uint96 balance, uint96 nativeBalance, uint64 reqCount, address owner, address[] memory consumers)
{
if (s_subscriptionConfigs[subId].owner == address(0)) {
owner = s_subscriptionConfigs[subId].owner;
if (owner == address(0)) {
revert InvalidSubscription();
}
return (
s_subscriptions[subId].balance,
s_subscriptions[subId].nativeBalance,
s_subscriptions[subId].reqCount,
s_subscriptionConfigs[subId].owner,
owner,
s_subscriptionConfigs[subId].consumers
);
}
Expand All @@ -324,13 +326,14 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
function getActiveSubscriptionIds(
uint256 startIndex,
uint256 maxCount
) external view override returns (uint256[] memory) {
) external view override returns (uint256[] memory ids) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: So if you don't use named return variables, there's an additional cost (like an additional temp var copy operation)? What happens in the background if you don't use this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing that there will be an additional temp var copy operation. Not sure, will have to dive into the opcodes generated.

uint256 numSubs = s_subIds.length();
if (startIndex >= numSubs) revert IndexOutOfRange();
uint256 endIndex = startIndex + maxCount;
endIndex = endIndex > numSubs || maxCount == 0 ? numSubs : endIndex;
uint256[] memory ids = new uint256[](endIndex - startIndex);
for (uint256 idx = 0; idx < ids.length; idx++) {
uint256 idsLength = endIndex - startIndex;
ids = new uint256[](idsLength);
for (uint256 idx = 0; idx < idsLength; ++idx) {
ids[idx] = s_subIds.at(idx + startIndex);
}
return ids;
Expand All @@ -339,13 +342,14 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
/**
* @inheritdoc IVRFSubscriptionV2Plus
*/
function createSubscription() external override nonReentrant returns (uint256) {
function createSubscription() external override nonReentrant returns (uint256 subId) {
// Generate a subscription id that is globally unique.
uint256 subId = uint256(
keccak256(abi.encodePacked(msg.sender, blockhash(block.number - 1), address(this), s_currentSubNonce))
uint64 currentSubNonce = s_currentSubNonce;
subId = uint256(
keccak256(abi.encodePacked(msg.sender, blockhash(block.number - 1), address(this), currentSubNonce))
);
// Increment the subscription nonce counter.
s_currentSubNonce++;
s_currentSubNonce = currentSubNonce + 1;
// Initialize storage variables.
address[] memory consumers = new address[](0);
s_subscriptions[subId] = Subscription({balance: 0, nativeBalance: 0, reqCount: 0});
Expand All @@ -369,8 +373,9 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
address newOwner
) external override onlySubOwner(subId) nonReentrant {
// Proposing to address(0) would never be claimable so don't need to check.
if (s_subscriptionConfigs[subId].requestedOwner != newOwner) {
s_subscriptionConfigs[subId].requestedOwner = newOwner;
SubscriptionConfig storage subscriptionConfig = s_subscriptionConfigs[subId];
if (subscriptionConfig.requestedOwner != newOwner) {
subscriptionConfig.requestedOwner = newOwner;
emit SubscriptionOwnerTransferRequested(subId, msg.sender, newOwner);
}
}
Expand All @@ -379,13 +384,13 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
* @inheritdoc IVRFSubscriptionV2Plus
*/
function acceptSubscriptionOwnerTransfer(uint256 subId) external override nonReentrant {
if (s_subscriptionConfigs[subId].owner == address(0)) {
address oldOwner = s_subscriptionConfigs[subId].owner;
if (oldOwner == address(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say we define a constant value called address ZERO_ADDRESS = address(0) somewhere in some shared file (of course, that increases the contract size a bit). In that case, we replace each occurrence of address(0) with the constant's name. Now, this value is re-used within all function calls that require it and no temp var is created just for the sake of the condition check. From the Solidity perspective, would that approach be better or worse and why? Again, I'm just curious and trying to learn a thing or two about contract optimization techniques.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there will be no difference as the compiler would replace each constant values with the actual values wherever it is used. Gas cost for ISZERO and EQ are both 3.

These questions are good. Keep them coming. I wouldn't say I'm an expert in contract optimisation. I'm just tweaking things around and see which actually saves gas. But I do love to optimise things.

revert InvalidSubscription();
}
if (s_subscriptionConfigs[subId].requestedOwner != msg.sender) {
revert MustBeRequestedOwner(s_subscriptionConfigs[subId].requestedOwner);
}
address oldOwner = s_subscriptionConfigs[subId].owner;
s_subscriptionConfigs[subId].owner = msg.sender;
s_subscriptionConfigs[subId].requestedOwner = address(0);
emit SubscriptionOwnerTransferred(subId, oldOwner, msg.sender);
Expand All @@ -396,36 +401,42 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
*/
function addConsumer(uint256 subId, address consumer) external override onlySubOwner(subId) nonReentrant {
// Already maxed, cannot add any more consumers.
if (s_subscriptionConfigs[subId].consumers.length == MAX_CONSUMERS) {
address[] storage consumers = s_subscriptionConfigs[subId].consumers;
if (consumers.length == MAX_CONSUMERS) {
revert TooManyConsumers();
}
if (s_consumers[consumer][subId] != 0) {
mapping(uint256 => uint64) storage nonces = s_consumers[consumer];
if (nonces[subId] != 0) {
// Idempotence - do nothing if already added.
// Ensures uniqueness in s_subscriptions[subId].consumers.
return;
}
// Initialize the nonce to 1, indicating the consumer is allocated.
s_consumers[consumer][subId] = 1;
s_subscriptionConfigs[subId].consumers.push(consumer);
nonces[subId] = 1;
consumers.push(consumer);

emit SubscriptionConsumerAdded(subId, consumer);
}

function _deleteSubscription(uint256 subId) internal returns (uint96 balance, uint96 nativeBalance) {
SubscriptionConfig memory subConfig = s_subscriptionConfigs[subId];
Subscription memory sub = s_subscriptions[subId];
balance = sub.balance;
nativeBalance = sub.nativeBalance;
address[] storage consumers = s_subscriptionConfigs[subId].consumers;
leeyikjiun marked this conversation as resolved.
Show resolved Hide resolved
balance = s_subscriptions[subId].balance;
nativeBalance = s_subscriptions[subId].nativeBalance;
// Note bounded by MAX_CONSUMERS;
// If no consumers, does nothing.
for (uint256 i = 0; i < subConfig.consumers.length; i++) {
delete s_consumers[subConfig.consumers[i]][subId];
uint256 consumersLength = consumers.length;
for (uint256 i = 0; i < consumersLength; ++i) {
delete s_consumers[consumers[i]][subId];
}
delete s_subscriptionConfigs[subId];
delete s_subscriptions[subId];
s_subIds.remove(subId);
s_totalBalance -= balance;
s_totalNativeBalance -= nativeBalance;
if (balance != 0) {
s_totalBalance -= balance;
}
if (nativeBalance != 0) {
s_totalNativeBalance -= nativeBalance;
}
return (balance, nativeBalance);
}

Expand Down
Loading
Loading