[Functions] (refactor): Various changes from review #3 (#10056)

* Reapply: s_router is private

* Create libraries folder, move Functions->FunctionsRequest & FunctionsResponse types in

* Revert HasRouter back to Routable, add natspec

* Separate Functions library Buffer import from CBOR

* Functions library, using before all

* (doc): Add more comments to Functions Request library

* Document and clean up FunctionsClient

* Geth wrappers

* Add more ToS allowlist events

* s_router -> i_router

* RouterBase: timelock to uint64, remove doubled Paused getter, if statement revert style

* Remove unused OCR hooks

* make flags index var name more clear & add comment about internal function modifiers

* Regenerate geth wrappers

* Various size reducing measures

* Save

* Use ^v0.8.0 in openzeppelin-solidity/v4.8.0

* merge function base and router

* Router config as typed struct

* (test): Update tests

* Use structs as getter return

* Pull juelsPerGas into helper function

* Add OracleWithdrawAll

* Change multiline comments to single

* Disable timelock propose and set in anticipation of MCM change

* Regenerate geth wrappers

* Add estimated cost to RequestStart

* Fix foundry test

* (test): Amend go integration test for Router constructor

---------

Co-authored-by: Rens Rooimans <[email protected]>

contracts/src/v0.8/functions/dev/1_0_0/FunctionsCoordinator.sol

@@ -9,11 +9,9 @@ import {FunctionsBilling} from "./FunctionsBilling.sol";
 import {OCR2Base} from "./ocr/OCR2Base.sol";
 import {FunctionsResponse} from "./libraries/FunctionsResponse.sol";
 
-/**
- * @title Functions Coordinator contract
- * @notice Contract that nodes of a Decentralized Oracle Network (DON) interact with
- * @dev THIS CONTRACT HAS NOT GONE THROUGH ANY SECURITY REVIEW. DO NOT USE IN PROD.
- */
+// @title Functions Coordinator contract
+// @notice Contract that nodes of a Decentralized Oracle Network (DON) interact with
+// @dev THIS CONTRACT HAS NOT GONE THROUGH ANY SECURITY REVIEW. DO NOT USE IN PROD.
 contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilling {
   using FunctionsResponse for FunctionsResponse.Commitment;
   using FunctionsResponse for FunctionsResponse.FulfillResult;
@@ -50,49 +48,39 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
     address linkToNativeFeed
   ) OCR2Base(true) FunctionsBilling(router, config, linkToNativeFeed) {}
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function getThresholdPublicKey() external view override returns (bytes memory) {
     if (s_thresholdPublicKey.length == 0) {
       revert EmptyPublicKey();
     }
     return s_thresholdPublicKey;
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function setThresholdPublicKey(bytes calldata thresholdPublicKey) external override onlyOwner {
     if (thresholdPublicKey.length == 0) {
       revert EmptyPublicKey();
     }
     s_thresholdPublicKey = thresholdPublicKey;
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function getDONPublicKey() external view override returns (bytes memory) {
     if (s_donPublicKey.length == 0) {
       revert EmptyPublicKey();
     }
     return s_donPublicKey;
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function setDONPublicKey(bytes calldata donPublicKey) external override onlyOwner {
     if (donPublicKey.length == 0) {
       revert EmptyPublicKey();
     }
     s_donPublicKey = donPublicKey;
   }
 
-  /**
-   * @dev check if node is in current transmitter list
-   */
+  // @dev check if node is in current transmitter list
   function _isTransmitter(address node) internal view returns (bool) {
     address[] memory nodes = s_transmitters;
     // Bounded by "maxNumOracles" on OCR2Abstract.sol
@@ -104,9 +92,7 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
     return false;
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function setNodePublicKey(address node, bytes calldata publicKey) external override {
     // Owner can set anything. Transmitters can set only their own key.
     if (!(msg.sender == owner() || (_isTransmitter(msg.sender) && msg.sender == node))) {
@@ -115,9 +101,7 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
     s_nodePublicKeys[node] = publicKey;
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function deleteNodePublicKey(address node) external override {
     // Owner can delete anything. Others can delete only their own key.
     if (msg.sender != owner() && msg.sender != node) {
@@ -126,25 +110,22 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
     delete s_nodePublicKeys[node];
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function getAllNodePublicKeys() external view override returns (address[] memory, bytes[] memory) {
     address[] memory nodes = s_transmitters;
     bytes[] memory keys = new bytes[](nodes.length);
     // Bounded by "maxNumOracles" on OCR2Abstract.sol
     for (uint256 i = 0; i < nodes.length; ++i) {
-      if (s_nodePublicKeys[nodes[i]].length == 0) {
+      bytes memory nodePublicKey = s_nodePublicKeys[nodes[i]];
+      if (nodePublicKey.length == 0) {
         revert EmptyPublicKey();
       }
-      keys[i] = s_nodePublicKeys[nodes[i]];
+      keys[i] = nodePublicKey;
     }
     return (nodes, keys);
   }
 
-  /**
-   * @inheritdoc IFunctionsCoordinator
-   */
+  // @inheritdoc IFunctionsCoordinator
   function sendRequest(
     Request calldata request
   ) external override onlyRouter returns (FunctionsResponse.Commitment memory commitment) {
@@ -188,6 +169,7 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
     return s_transmitters;
   }
 
+  // Report hook called within OCR2Base.sol
   function _report(
     uint256 /*initialGas*/,
     address /*transmitter*/,
@@ -204,6 +186,7 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
       report,
       (bytes32[], bytes[], bytes[], bytes[], bytes[])
     );
+
     if (
       requestIds.length == 0 ||
       requestIds.length != results.length ||
@@ -220,11 +203,19 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
         _fulfillAndBill(requestIds[i], results[i], errors[i], onchainMetadata[i], offchainMetadata[i])
       );
 
+      // Emit on successfully processing the fulfillment
+      // In these two fulfillment results the user has been charged
+      // Otherwise, the DON will re-try
       if (
         result == FunctionsResponse.FulfillResult.USER_SUCCESS || result == FunctionsResponse.FulfillResult.USER_ERROR
       ) {
         emit OracleResponse(requestIds[i], msg.sender);
       }
     }
   }
+
+  // Used in FunctionsBilling.sol
+  function _onlyOwner() internal view override {
+    _validateOwnership();
+  }
 }