Misc changes (#12489)

* Misc changes * Minor fix * Reuse RequestCommitmentV2Plus, fix interface name, integration tests fix * Updated changesets * Fix comment
smartcontractkit · Mar 20, 2024 · 3a49094 · 3a49094
1 parent 1a36386
commit 3a49094
Show file tree

Hide file tree

Showing 18 changed files with 101 additions and 82 deletions.
diff --git a/.changeset/hot-pets-sneeze.md b/.changeset/hot-pets-sneeze.md
@@ -0,0 +1,12 @@
+---
+"chainlink": minor
+---
+
+- Misc VRF V2+ contract changes
+  - Reuse struct RequestCommitmentV2Plus from VRFTypes
+  - Fix interface name IVRFCoordinatorV2PlusFulfill in BatchVRFCoordinatorV2Plus to avoid confusion with IVRFCoordinatorV2Plus.sol
+  - Remove unused errors
+  - Rename variables for readability
+  - Fix comments
+  - Minor gas optimisation (++i)
+- Fix integration tests
diff --git a/contracts/.changeset/clever-kings-smell.md b/contracts/.changeset/clever-kings-smell.md
@@ -0,0 +1,11 @@
+---
+"@chainlink/contracts": minor
+---
+
+- Misc VRF V2+ contract changes
+  - Reuse struct RequestCommitmentV2Plus from VRFTypes
+  - Fix interface name IVRFCoordinatorV2PlusFulfill in BatchVRFCoordinatorV2Plus to avoid confusion with IVRFCoordinatorV2Plus.sol
+  - Remove unused errors
+  - Rename variables for readability
+  - Fix comments
+  - Minor gas optimisation (++i)
diff --git a/contracts/src/v0.8/vrf/dev/BatchVRFCoordinatorV2Plus.sol b/contracts/src/v0.8/vrf/dev/BatchVRFCoordinatorV2Plus.sol
@@ -11,13 +11,13 @@ import {VRFTypes} from "../VRFTypes.sol";
  */
 contract BatchVRFCoordinatorV2Plus {
   // solhint-disable-next-line chainlink-solidity/prefix-immutable-variables-with-i
-  IVRFCoordinatorV2Plus public immutable COORDINATOR;
+  IVRFCoordinatorV2PlusFulfill public immutable COORDINATOR;
 
   event ErrorReturned(uint256 indexed requestId, string reason);
   event RawErrorReturned(uint256 indexed requestId, bytes lowLevelData);
 
   constructor(address coordinatorAddr) {
-    COORDINATOR = IVRFCoordinatorV2Plus(coordinatorAddr);
+    COORDINATOR = IVRFCoordinatorV2PlusFulfill(coordinatorAddr);
   }
 
   /**
@@ -28,7 +28,7 @@ contract BatchVRFCoordinatorV2Plus {
   function fulfillRandomWords(VRFTypes.Proof[] memory proofs, VRFTypes.RequestCommitmentV2Plus[] memory rcs) external {
     // solhint-disable-next-line custom-errors
     require(proofs.length == rcs.length, "input array arg lengths mismatch");
-    for (uint256 i = 0; i < proofs.length; i++) {
+    for (uint256 i = 0; i < proofs.length; ++i) {
       try COORDINATOR.fulfillRandomWords(proofs[i], rcs[i], false) returns (uint96 /* payment */) {
         continue;
       } catch Error(string memory reason) {
@@ -59,7 +59,7 @@ contract BatchVRFCoordinatorV2Plus {
   }
 }
 
-interface IVRFCoordinatorV2Plus {
+interface IVRFCoordinatorV2PlusFulfill {
   function fulfillRandomWords(
     VRFTypes.Proof memory proof,
     VRFTypes.RequestCommitmentV2Plus memory rc,

diff --git a/contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol b/contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol
@@ -65,7 +65,7 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
     uint64 nonce;
     uint64 pendingReqCount;
   }
-  // Note a nonce of 0 indicates an the consumer is not assigned to that subscription.
+  // Note a nonce of 0 indicates the consumer is not assigned to that subscription.
   mapping(address => mapping(uint256 => ConsumerConfig)) /* consumerAddress */ /* subId */ /* consumerConfig */
     internal s_consumers;
   mapping(uint256 => SubscriptionConfig) /* subId */ /* subscriptionConfig */ internal s_subscriptionConfigs;
@@ -171,11 +171,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 {
-    address owner = s_subscriptionConfigs[subId].owner;
-    if (owner == address(0)) {
+    address subOwner = s_subscriptionConfigs[subId].owner;
+    if (subOwner == address(0)) {
       revert InvalidSubscription();
     }
-    _cancelSubscriptionHelper(subId, owner);
+    _cancelSubscriptionHelper(subId, subOwner);
   }
 
   /**
@@ -311,17 +311,17 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
     public
     view
     override
-    returns (uint96 balance, uint96 nativeBalance, uint64 reqCount, address owner, address[] memory consumers)
+    returns (uint96 balance, uint96 nativeBalance, uint64 reqCount, address subOwner, address[] memory consumers)
   {
-    owner = s_subscriptionConfigs[subId].owner;
-    if (owner == address(0)) {
+    subOwner = s_subscriptionConfigs[subId].owner;
+    if (subOwner == address(0)) {
       revert InvalidSubscription();
     }
     return (
       s_subscriptions[subId].balance,
       s_subscriptions[subId].nativeBalance,
       s_subscriptions[subId].reqCount,
-      owner,
+      subOwner,
       s_subscriptionConfigs[subId].consumers
     );
   }
@@ -472,12 +472,12 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
   }
 
   function _onlySubOwner(uint256 subId) internal view {
-    address owner = s_subscriptionConfigs[subId].owner;
-    if (owner == address(0)) {
+    address subOwner = s_subscriptionConfigs[subId].owner;
+    if (subOwner == address(0)) {
       revert InvalidSubscription();
     }
-    if (msg.sender != owner) {
-      revert MustBeSubOwner(owner);
+    if (msg.sender != subOwner) {
+      revert MustBeSubOwner(subOwner);
     }
   }
 }
diff --git a/contracts/src/v0.8/vrf/dev/VRFConsumerBaseV2Plus.sol b/contracts/src/v0.8/vrf/dev/VRFConsumerBaseV2Plus.sol
@@ -147,7 +147,7 @@ abstract contract VRFConsumerBaseV2Plus is IVRFMigratableConsumerV2Plus, Confirm
   /**
    * @inheritdoc IVRFMigratableConsumerV2Plus
    */
-  function setCoordinator(address _vrfCoordinator) public override onlyOwnerOrCoordinator {
+  function setCoordinator(address _vrfCoordinator) external override onlyOwnerOrCoordinator {
     if (_vrfCoordinator == address(0)) {
       revert ZeroAddress();
     }

diff --git a/contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol b/contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol
@@ -3,6 +3,7 @@ pragma solidity ^0.8.4;
 
 import {BlockhashStoreInterface} from "../interfaces/BlockhashStoreInterface.sol";
 import {VRF} from "../../vrf/VRF.sol";
+import {VRFTypes} from "../VRFTypes.sol";
 import {VRFConsumerBaseV2Plus, IVRFMigratableConsumerV2Plus} from "./VRFConsumerBaseV2Plus.sol";
 import {ChainSpecificUtil} from "../../ChainSpecificUtil.sol";
 import {SubscriptionAPI} from "./SubscriptionAPI.sol";
@@ -35,21 +36,12 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
   error InvalidLinkWeiPrice(int256 linkWei);
   error LinkDiscountTooHigh(uint32 flatFeeLinkDiscountPPM, uint32 flatFeeNativePPM);
   error InvalidPremiumPercentage(uint8 premiumPercentage, uint8 max);
-  error InsufficientGasForConsumer(uint256 have, uint256 want);
   error NoCorrespondingRequest();
   error IncorrectCommitment();
   error BlockhashNotInStore(uint256 blockNum);
   error PaymentTooLarge();
   error InvalidExtraArgsTag();
   error GasPriceExceeded(uint256 gasPrice, uint256 maxGas);
-  struct RequestCommitment {
-    uint64 blockNum;
-    uint256 subId;
-    uint32 callbackGasLimit;
-    uint32 numWords;
-    address sender;
-    bytes extraArgs;
-  }
 
   struct ProvingKey {
     bool exists; // proving key exists
@@ -376,7 +368,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
 
   function _getRandomnessFromProof(
     Proof memory proof,
-    RequestCommitment memory rc
+    VRFTypes.RequestCommitmentV2Plus memory rc
   ) internal view returns (Output memory) {
     bytes32 keyHash = hashOfKey(proof.pk);
     ProvingKey memory key = s_provingKeys[keyHash];
@@ -425,7 +417,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
 
   function _deliverRandomness(
     uint256 requestId,
-    RequestCommitment memory rc,
+    VRFTypes.RequestCommitmentV2Plus memory rc,
     uint256[] memory randomWords
   ) internal returns (bool success) {
     VRFConsumerBaseV2Plus v;
@@ -452,7 +444,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
    */
   function fulfillRandomWords(
     Proof memory proof,
-    RequestCommitment memory rc,
+    VRFTypes.RequestCommitmentV2Plus memory rc,
     bool onlyPremium
   ) external nonReentrant returns (uint96 payment) {
     uint256 startGas = gasleft();
@@ -508,9 +500,11 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
 
     // stack too deep error
     {
-      // We want to charge users exactly for how much gas they use in their callback.
-      // The gasAfterPaymentCalculation is meant to cover these additional operations where we
-      // decrement the subscription balance and increment the oracles withdrawable balance.
+      // We want to charge users exactly for how much gas they use in their callback with
+      // an additional premium. If onlyPremium is true, only premium is charged without
+      // the gas cost. The gasAfterPaymentCalculation is meant to cover these additional
+      // operations where we decrement the subscription balance and increment the
+      // withdrawable balance.
       bool isFeedStale;
       (payment, isFeedStale) = _calculatePaymentAmount(startGas, gasPrice, nativePayment, onlyPremium);
       if (isFeedStale) {
@@ -741,16 +735,16 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
     if (!_isTargetRegistered(newCoordinator)) {
       revert CoordinatorNotRegistered(newCoordinator);
     }
-    (uint96 balance, uint96 nativeBalance, , address owner, address[] memory consumers) = getSubscription(subId);
+    (uint96 balance, uint96 nativeBalance, , address subOwner, address[] memory consumers) = getSubscription(subId);
     // solhint-disable-next-line custom-errors
-    require(owner == msg.sender, "Not subscription owner");
+    require(subOwner == msg.sender, "Not subscription owner");
     // solhint-disable-next-line custom-errors
     require(!pendingRequestExists(subId), "Pending request exists");
 
     V1MigrationData memory migrationData = V1MigrationData({
       fromVersion: 1,
       subId: subId,
-      subOwner: owner,
+      subOwner: subOwner,
       consumers: consumers,
       linkBalance: balance,
       nativeBalance: nativeBalance

diff --git a/contracts/src/v0.8/vrf/dev/interfaces/IVRFCoordinatorV2Plus.sol b/contracts/src/v0.8/vrf/dev/interfaces/IVRFCoordinatorV2Plus.sol
@@ -9,7 +9,7 @@ import {IVRFSubscriptionV2Plus} from "./IVRFSubscriptionV2Plus.sol";
 interface IVRFCoordinatorV2Plus is IVRFSubscriptionV2Plus {
   /**
    * @notice Request a set of random words.
-   * @param req - a struct containing following fiels for randomness request:
+   * @param req - a struct containing following fields for randomness request:
    * keyHash - Corresponds to a particular oracle job which uses
    * that key for generating the VRF proof. Different keyHash's have different gas price
    * ceilings, so you can select a specific one to bound your maximum per request cost.

diff --git a/contracts/src/v0.8/vrf/dev/interfaces/IVRFMigratableConsumerV2Plus.sol b/contracts/src/v0.8/vrf/dev/interfaces/IVRFMigratableConsumerV2Plus.sol
@@ -8,6 +8,6 @@ interface IVRFMigratableConsumerV2Plus {
   event CoordinatorSet(address vrfCoordinator);
 
   /// @notice Sets the VRF Coordinator address
-  /// @notice This method is should only be callable by the coordinator or contract owner
+  /// @notice This method should only be callable by the coordinator or contract owner
   function setCoordinator(address vrfCoordinator) external;
 }
diff --git a/contracts/src/v0.8/vrf/dev/interfaces/IVRFSubscriptionV2Plus.sol b/contracts/src/v0.8/vrf/dev/interfaces/IVRFSubscriptionV2Plus.sol
@@ -26,7 +26,7 @@ interface IVRFSubscriptionV2Plus {
   function cancelSubscription(uint256 subId, address to) external;
 
   /**
-   * @notice Request subscription owner transfer.
+   * @notice Accept subscription owner transfer.
    * @param subId - ID of the subscription
    * @dev will revert if original owner of subId has
    * not requested that msg.sender become the new owner.
@@ -92,7 +92,7 @@ interface IVRFSubscriptionV2Plus {
   /**
    * @notice Fund a subscription with native.
    * @param subId - ID of the subscription
-   * @notice This method expects msg.value to be greater than 0.
+   * @notice This method expects msg.value to be greater than or equal to 0.
    */
   function fundSubscriptionWithNative(uint256 subId) external payable;
 }
diff --git a/contracts/src/v0.8/vrf/dev/testhelpers/ExposedVRFCoordinatorV2_5.sol b/contracts/src/v0.8/vrf/dev/testhelpers/ExposedVRFCoordinatorV2_5.sol
@@ -2,6 +2,7 @@
 pragma solidity ^0.8.4;
 
 import {VRFCoordinatorV2_5} from "../VRFCoordinatorV2_5.sol";
+import {VRFTypes} from "../../VRFTypes.sol";
 import {EnumerableSet} from "../../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/structs/EnumerableSet.sol";
 
 // solhint-disable-next-line contract-name-camelcase
@@ -25,7 +26,7 @@ contract ExposedVRFCoordinatorV2_5 is VRFCoordinatorV2_5 {
 
   function getRandomnessFromProofExternal(
     Proof calldata proof,
-    RequestCommitment calldata rc
+    VRFTypes.RequestCommitmentV2Plus calldata rc
   ) external view returns (Output memory) {
     return _getRandomnessFromProof(proof, rc);
   }

diff --git a/contracts/test/v0.8/foundry/vrf/VRFV2Plus.t.sol b/contracts/test/v0.8/foundry/vrf/VRFV2Plus.t.sol
@@ -10,6 +10,7 @@ import {SubscriptionAPI} from "../../../../src/v0.8/vrf/dev/SubscriptionAPI.sol"
 import {BlockhashStore} from "../../../../src/v0.8/vrf/dev/BlockhashStore.sol";
 import {VRFV2PlusConsumerExample} from "../../../../src/v0.8/vrf/dev/testhelpers/VRFV2PlusConsumerExample.sol";
 import {VRFV2PlusClient} from "../../../../src/v0.8/vrf/dev/libraries/VRFV2PlusClient.sol";
+import {VRFTypes} from "../../../../src/v0.8/vrf/VRFTypes.sol";
 import {console} from "forge-std/console.sol";
 import {VmSafe} from "forge-std/Vm.sol";
 import {VRFV2PlusLoadTestWithMetrics} from "../../../../src/v0.8/vrf/dev/testhelpers/VRFV2PlusLoadTestWithMetrics.sol";
@@ -373,7 +374,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWordsNative() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       uint256 subId,
       uint256 requestId
     ) = setupSubAndRequestRandomnessNativePayment();
@@ -415,7 +416,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWordsLINK() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       uint256 subId,
       uint256 requestId
     ) = setupSubAndRequestRandomnessLINKPayment();
@@ -460,7 +461,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWordsLINK_FallbackWeiPerUnitLinkUsed() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       ,
       uint256 requestId
     ) = setupSubAndRequestRandomnessLINKPayment();
@@ -480,7 +481,7 @@ contract VRFV2Plus is BaseTest {
 
   function setupSubAndRequestRandomnessLINKPayment()
     internal
-    returns (VRF.Proof memory proof, VRFCoordinatorV2_5.RequestCommitment memory rc, uint256 subId, uint256 requestId)
+    returns (VRF.Proof memory proof, VRFTypes.RequestCommitmentV2Plus memory rc, uint256 subId, uint256 requestId)
   {
     uint32 requestBlock = 20;
     vm.roll(requestBlock);
@@ -556,7 +557,7 @@ contract VRFV2Plus is BaseTest {
       ],
       zInv: 82374292458278672300647114418593830323283909625362447038989596015264004164958
     });
-    rc = VRFCoordinatorV2_5.RequestCommitment({
+    rc = VRFTypes.RequestCommitmentV2Plus({
       blockNum: requestBlock,
       subId: subId,
       callbackGasLimit: 1000000,
@@ -569,7 +570,7 @@ contract VRFV2Plus is BaseTest {
 
   function setupSubAndRequestRandomnessNativePayment()
     internal
-    returns (VRF.Proof memory proof, VRFCoordinatorV2_5.RequestCommitment memory rc, uint256 subId, uint256 requestId)
+    returns (VRF.Proof memory proof, VRFTypes.RequestCommitmentV2Plus memory rc, uint256 subId, uint256 requestId)
   {
     uint32 requestBlock = 10;
     vm.roll(requestBlock);
@@ -646,7 +647,7 @@ contract VRFV2Plus is BaseTest {
       ],
       zInv: 88742453392918610091640193378775723954629905126315835248392650970979000380325
     });
-    rc = VRFCoordinatorV2_5.RequestCommitment({
+    rc = VRFTypes.RequestCommitmentV2Plus({
       blockNum: requestBlock,
       subId: subId,
       callbackGasLimit: CALLBACK_GAS_LIMIT,
@@ -661,7 +662,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWords_NetworkGasPriceExceedsGasLane() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       ,
 
     ) = setupSubAndRequestRandomnessNativePayment();
@@ -678,7 +679,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWords_OnlyPremium_NativePayment() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       uint256 subId,
       uint256 requestId
     ) = setupSubAndRequestRandomnessNativePayment();
@@ -725,7 +726,7 @@ contract VRFV2Plus is BaseTest {
   function testRequestAndFulfillRandomWords_OnlyPremium_LinkPayment() public {
     (
       VRF.Proof memory proof,
-      VRFCoordinatorV2_5.RequestCommitment memory rc,
+      VRFTypes.RequestCommitmentV2Plus memory rc,
       uint256 subId,
       uint256 requestId
     ) = setupSubAndRequestRandomnessLINKPayment();
@@ -878,7 +879,7 @@ contract VRFV2Plus is BaseTest {
       ],
       zInv: 18898957977631212231148068121702167284572066246731769473720131179584458697812
     });
-    VRFCoordinatorV2_5.RequestCommitment memory rc = VRFCoordinatorV2_5.RequestCommitment({
+    VRFTypes.RequestCommitmentV2Plus memory rc = VRFTypes.RequestCommitmentV2Plus({
       blockNum: requestBlock,
       subId: subId,
       callbackGasLimit: CALLBACK_GAS_LIMIT,
@@ -1028,7 +1029,7 @@ contract VRFV2Plus is BaseTest {
       ],
       zInv: 18898957977631212231148068121702167284572066246731769473720131179584458697812
     });
-    VRFCoordinatorV2_5.RequestCommitment memory rc = VRFCoordinatorV2_5.RequestCommitment({
+    VRFTypes.RequestCommitmentV2Plus memory rc = VRFTypes.RequestCommitmentV2Plus({
       blockNum: requestBlock,
       subId: subId,
       callbackGasLimit: CALLBACK_GAS_LIMIT,
@@ -1078,7 +1079,7 @@ contract VRFV2Plus is BaseTest {
       ],
       zInv: 29080001901010358083725892808339807464533563010468652346220922643802059192842
     });
-    rc = VRFCoordinatorV2_5.RequestCommitment({
+    rc = VRFTypes.RequestCommitmentV2Plus({
       blockNum: requestBlock,
       subId: subId,
       callbackGasLimit: CALLBACK_GAS_LIMIT,