[Functions] Various changes from review #2 #10014

justinkaseman · 2023-07-31T23:36:44Z

No description provided.

…ansfer

github-actions · 2023-07-31T23:37:13Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-07-31T23:37:14Z

I see that you haven't updated any README files. Would it make sense to do so?

github-actions · 2023-08-02T02:01:57Z

Go solidity wrappers are out-of-date, regenerate them via the make go-solidity-wrappers command

justinkaseman · 2023-08-02T02:41:12Z

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

    // NOTE: Optionally, compute additional fee here
-    return IFunctionsRouter(address(_getRouter())).getAdminFee();
+    (adminFee, , ) = IFunctionsRouter(address(_getRouter())).getConfig();


Morgan's PR will take over this change

justinkaseman · 2023-08-02T02:41:47Z

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

@@ -67,29 +67,10 @@ abstract contract FunctionsClient is IFunctionsClient {
  /**
   * @inheritdoc IFunctionsClient


Leaning out FunctionsClient

justinkaseman · 2023-08-02T02:42:02Z

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

@@ -365,7 +365,7 @@ abstract contract FunctionsBilling is HasRouter, IFunctionsBilling {

    // The Functions Router will perform the callback to the client contract
    IFunctionsRouter router = IFunctionsRouter(address(_getRouter()));
-    (uint8 result, uint96 callbackCostJuels) = router.fulfill(
+    (FulfillResult resultCode, uint96 callbackCostJuels) = router.fulfill(


Return result code directly rather than uint8

justinkaseman · 2023-08-02T02:42:44Z

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

@@ -226,52 +223,43 @@ contract FunctionsRouter is RouterBase, IFunctionsRouter, FunctionsSubscriptions
    bytes memory response,
    bytes memory err,
    uint96 juelsPerGas,
-    uint96 costWithoutFulfillment,
+    uint96 costWithoutCallback,


More accurate name

KuphJr · 2023-08-02T02:55:33Z

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

@@ -7,8 +7,6 @@ import {ITypeAndVersion} from "../../../shared/interfaces/ITypeAndVersion.sol";
 import {IOwnableRouter} from "./interfaces/IOwnableRouter.sol";

 abstract contract HasRouter is ITypeAndVersion, IConfigurable {
-  bytes32 internal s_configHash;


We don't care about the config hash anymore?

It provided a safety check where you couldn't double apply the same config, but that's not really a problem. Querying the hash also isn't really use-able information.

So it got removed to reduce gas costs & reduce contract size

KuphJr

Overall, I like the changes. It simplifies a lot.

I want someone else to look these contracts over since it is pretty late for me, however I know Steve and Rens are planning to review the refactored contracts tomorrow, so I'll approve such that we can get this merged.

pinebit · 2023-08-02T04:49:18Z

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

@@ -218,20 +198,19 @@ abstract contract RouterBase is IRouterBase, Pausable, ITypeAndVersion, Confirme
      address from = s_route[id];
      address to = s_proposedContractSet.to[i];
      s_route[id] = to;
-      emit ContractUpdated(id, from, to);
+      emit ContractUpdated({
+        proposedContractSetId: id,


Why arguments for this event are prefixed "proposed"?

justinkaseman · 2023-08-02T05:41:22Z

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

@@ -49,14 +45,14 @@ abstract contract FunctionsSubscriptions is IFunctionsSubscriptions, ERC677Recei

  error TooManyConsumers();
  error InsufficientBalance();
-  error InvalidConsumer(uint64 subscriptionId, address consumer);
+  error InvalidConsumer();


Losing error args to help with contract size. They aren't super useful anyway.

justinkaseman · 2023-08-02T05:42:02Z

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

  /**
   * @inheritdoc IFunctionsSubscriptions
   */
-  function getFlags(uint64 subscriptionId) external view returns (bytes32) {
-    return _getFlags(subscriptionId);
+  function getFlags(uint64 subscriptionId) public view returns (bytes32) {


Saves us a couple bytes

github-actions · 2023-08-02T05:42:11Z

Go solidity wrappers are out-of-date, regenerate them via the make go-solidity-wrappers command

justinkaseman · 2023-08-02T05:42:55Z

contracts/src/v0.8/functions/dev/1_0_0/accessControl/interfaces/ITermsOfServiceAllowList.sol

-   * @return Hash of the message data packed as "\x19Ethereum Signed Message\n" + len(msg) + msg
-   */
-  function getEthSignedMessageHash(bytes32 messageHash) external pure returns (bytes32);
+  function getMessage(address acceptor, address recipient) external pure returns (bytes32);


Simplifying these

cl-sonarqube-production · 2023-08-02T05:48:18Z

SonarQube Quality Gate

0.0% Coverage on New Code (is less than 80%)

See analysis details on SonarQube

github-actions · 2023-08-03T15:53:40Z

Go solidity wrappers are out-of-date, regenerate them via the make go-solidity-wrappers command

justinkaseman added 28 commits July 30, 2023 11:00

Move FulfillResultCodes to interfaces folder

af60c85

Rename Function.sol setArgs -> addArgs

f8a82e8

Rename Routable s_config_hash -> s_configHash

e886428

Make Routable updateConfig public for re-use in constructor

4ee6324

Move comment out of IConfigurable

80d8598

Make Routable s_router private with an internal getter

9700595

Rename constant to ROUTER_ID

66936e5

Rename RouterBase s_config_hash -> s_configHash

b8d2049

Rename Routable.sol -> HasRouter.sol

98e91ef

Add IOwnableRouter interface

8a428ed

FunctionsSubscriptions MustBeSubOwner -> MustBeSubscriptionOwner

0be5963

Make RouterBase variable private

c804b4d

RouterBase togglePause -> pause/unpause

8f5a72b

Use full semantic version in typeAndVersion

0b55614

ToS Allow List rename proof -> signature

d2de7bb

Change acceptTermsOfService arguments from signature to raw r,s,v

12bee85

Prettier format 💅

ec00ae0

Remove RouterBase versioning

47ad2e2

Move RequestTimedOut event to Router

534aab1

Rename requestSubscriptionOwnerTransfer -> proposeSubscriptionOwnerTr…

d4a94aa

…ansfer

Merge branch 'develop' into jk/functions_various

4da3dd1

(test): Update tests for changes

65f08a8

(test): Add v0 gas test

c9a47f9

Regenerate geth wrappers

ef299cc

Use FulfillResult within FunctionsRouter

83cc5cd

FunctionsRouter getConfig getter

99f5a98

Strip down FunctionsClient.sol

c3ff782

Rename GAS_FLAX_INDEX -> MAX_CALLBACK_GAS_LIMIT_FLAGS_INDEX

788f96d

justinkaseman requested a review from connorwstein as a code owner August 2, 2023 01:56

Regenerate geth wrappers

ab3b3a3

justinkaseman force-pushed the jk/functions_various2 branch from 5a6c39d to ab3b3a3 Compare August 2, 2023 02:12

justinkaseman commented Aug 2, 2023

View reviewed changes

KuphJr reviewed Aug 2, 2023

View reviewed changes

KuphJr previously approved these changes Aug 2, 2023

View reviewed changes

pinebit reviewed Aug 2, 2023

View reviewed changes

Merge branch 'develop' into jk/functions_various2

6ee81c0

justinkaseman dismissed KuphJr’s stale review via 6ee81c0 August 2, 2023 05:34

justinkaseman requested review from pinebit and KuphJr August 2, 2023 05:35

Prettier & gethwrappers

22546a0

justinkaseman commented Aug 2, 2023

View reviewed changes

(test): amend foundry test

ffdaad1

justinkaseman enabled auto-merge August 2, 2023 05:46

pinebit approved these changes Aug 2, 2023

View reviewed changes

justinkaseman added this pull request to the merge queue Aug 2, 2023

Merged via the queue into develop with commit 600365a Aug 2, 2023
95 checks passed

justinkaseman deleted the jk/functions_various2 branch August 2, 2023 06:37

dmitry-diginex mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 dmitry-diginex/chainlink#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Functions] Various changes from review #2 #10014

[Functions] Various changes from review #2 #10014

justinkaseman commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

github-actions bot commented Aug 2, 2023

justinkaseman Aug 2, 2023

justinkaseman Aug 2, 2023

justinkaseman Aug 2, 2023

justinkaseman Aug 2, 2023

KuphJr Aug 2, 2023

justinkaseman Aug 2, 2023 •

edited

Loading

KuphJr left a comment •

edited

Loading

pinebit Aug 2, 2023

justinkaseman Aug 2, 2023

justinkaseman Aug 2, 2023

justinkaseman Aug 2, 2023

github-actions bot commented Aug 2, 2023

justinkaseman Aug 2, 2023

cl-sonarqube-production bot commented Aug 2, 2023

github-actions bot commented Aug 3, 2023

		@@ -67,29 +67,10 @@ abstract contract FunctionsClient is IFunctionsClient {
		/**
		* @inheritdoc IFunctionsClient

[Functions] Various changes from review #2 #10014

[Functions] Various changes from review #2 #10014

Conversation

justinkaseman commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

github-actions bot commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkaseman Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

KuphJr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 2, 2023

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Aug 2, 2023

github-actions bot commented Aug 3, 2023

justinkaseman Aug 2, 2023 •

edited

Loading

KuphJr left a comment •

edited

Loading