FUN-1234 (feat): Chainlink Functions premium fees in USD denomination #12000

justinkaseman · 2024-02-12T18:31:58Z

Changes:

Adds operationFee that pays to the FunctionsCoordinator contract's owner
FunctionsCoordinator now takes a LINK/USD feed in the constructor
A new Commitment & OracleRequest structure that uses the operationFee
The LogPollerWrapper uses the Coordinator's typeAndVersion to select between which OracleRequest to watch for

github-actions · 2024-02-12T18:32:16Z

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

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

contracts/src/v0.8/functions/dev/v1_X/interfaces/IFunctionsBilling.sol

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

KuphJr · 2024-02-15T00:16:51Z

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

+    return (uint256(usdPerUnitLink), s_linkToUsdFeed.decimals());
+  }
+
+  function _getJuelsFromUsd(uint256 amountUsd) private view returns (uint96) {


I need to work through this math by hand to verify.

I doubled checked. I think this is good.

We also have tests that triple and quadruple check this, right?

The tests are in this PR, yes. They should also be reviewed.

contracts/src/v0.8/functions/tests/v1_X/FunctionsBilling.t.sol

KuphJr · 2024-02-15T00:49:01Z

core/services/ocr2/plugins/functions/integration_tests/v1/internal/testutils.go

@@ -186,6 +186,10 @@ func StartNewChainWithContracts(t *testing.T, nClients int) (*bind.TransactOpts,
 	linkEthFeedAddr, _, _, err := mock_v3_aggregator_contract.DeployMockV3AggregatorContract(owner, b, 18, big.NewInt(5_000_000_000_000_000))
 	require.NoError(t, err)



I noticed we don't have additional tests that cover both the previous contract version & new contracts. My guess is this is gonna be flagged for lack of coverage by SonarQube. I think we need to write tests which cover both in order to get this PR merged.

Actually, I think the SonarQube coverage requirement may have been temporarily paused for other reasons? Either way, I'd really prefer we test with both versions of the contract here just to be safe.
I'm also curious if we can simulate a coordinator contract upgrade in our tests.

There is a test with an upgrade, though it doesn't go from version to version, because previously we only held on to one wrapper.

I think it's a good idea to change this test to actually move through breaking changes

I took an attempt with this, but the types got messy.
I'm opening a new ticket to ensure that this happens, even though it won't be in this PR.

okay. In the mean time, we should properly test this process on staging.

If you can, please preserve existing tests without changes and only add new ones. We can remove obsolete ones later, once prod is fully migrated.

core/services/relay/evm/functions/logpoller_wrapper.go

KuphJr · 2024-02-15T01:01:03Z

core/services/relay/evm/functions/logpoller_wrapper.go

+
+func (l *logPollerWrapper) logsToResponses(coordinator coordinator, responseLogs []logpoller.Log) ([]evmRelayTypes.OracleResponse, error) {
+	var responses []evmRelayTypes.OracleResponse
+


Why not set the parsingContract here first based on the version, then remove the 2 duplicated code blocks below?

This is because we don't have a robust way to handle the two different types of wrappers here. Go is very strict about typing. We need a wrapper of wrappers of sorts. Something we will add to our tech debt and tackle soon.

core/services/relay/evm/functions/logpoller_wrapper.go

contracts/scripts/native_solc_compile_all_functions

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

contracts/src/v0.8/functions/dev/v1_X/interfaces/IFunctionsBilling.sol

contracts/src/v0.8/functions/dev/v1_X/libraries/FunctionsResponse.sol

KuphJr · 2024-02-19T23:21:36Z

core/services/relay/evm/functions/logpoller_wrapper_test.go

+		return nil, err
+	}
+	return nameEncoded, nil
+}


Thank you very much for all the additional tests above!

…12066) * fix: test race * Revert "(test): Run LogPollerWrapperTest v1/v2 tests sequentially" This reverts commit 1ba2c06. * chore: refactor logpoller_wrapper by injecting a coordinator dep * fix: remove duplicates abitype definitions * fix: lint sonarqube reliability issue * fix: race conditions due to share variables --------- Co-authored-by: Justin Kaseman <[email protected]>

KuphJr · 2024-02-19T23:31:18Z

core/services/relay/evm/functions/logpoller_wrapper.go

+func init() {
+	err := initAbiTypes()
+	if err != nil {
+		panic(err)


Why are we introducing a panic? This does not seem like the best practice.

Actually, I am not quite sure I understand why this function is here at all...

We can probably run initAbiTypes() inside NewLogPollerWrapper() and also not panic.

Yeah I don't love the panic either, my initial reasons were:
The init() function can not return an error, so if anything fails this is the only way. The benefits of initialising this here is that it will run after variables are declare and before anything else is executed (this is specially important because these are pkg variables that can be reference from outside). Another pro: It will only run once compare to re-declaring the types on each request.

We can probably run initAbiTypes() inside NewLogPollerWrapper() and also not panic.

I thought about this but, wouldn't we leave the door open to someone referencing these pkg variables before they are initialised? 🤔

I think there is an alternative that could work too without panic and is defining a structure like:

type validAbiTypes struct{ uint32Type abi.Type, ... }

that contain all the valid types as part of logpuller_wrapper, this could be initialised in the NewLogPollerWrapper and injected down to the V1 and V2 implementations of the Coordinator.

will explore these two last options and have something for today

KuphJr · 2024-02-19T23:34:53Z

core/services/relay/evm/functions/logpoller_wrapper.go

-	if errActive != nil || errProposed != nil {
-		l.lggr.Errorw("LogPollerWrapper: Failed to register filters", "errorActive", errActive, "errorProposed", errProposed)
+	var activeCoordinator Coordinator
+	switch {


This section looks cleaner. Thanks!

KuphJr · 2024-02-19T23:36:10Z

core/services/relay/evm/functions/logpoller_wrapper_test.go

@@ -60,28 +55,42 @@ func newSubscriber(expectedCalls int) *subscriber {
 	return sub
 }

-func addr(t *testing.T, lastByte string) []byte {


Given that passing t was not actually the issue which caused the race condition, can we revert this change?

KuphJr · 2024-02-19T23:36:56Z

core/services/relay/evm/functions/logpoller_wrapper_test.go

+	return contractAddr, nil
+}
+
+type setupResponse struct {


Nice! Good way to clean this up.

bolekk

A bunch of minor comments and questions.
One small bug (cc @agparadiso please verify)

bolekk · 2024-02-20T01:20:30Z

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

      msg.sender,
-      commitment
+      FunctionsResponse.Commitment({


Can we add a comment here explaining that we re-pack everything into an older Commitment struct compatible with the Router?

Good suggestion, I agree that it'll make it more clear

bolekk · 2024-02-20T01:21:59Z

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

-      s_feePool += commitment.donFee;
+      s_feePool += commitment.donFeeJuels;
+      // Pay the operation fee to the Coordinator owner
+      s_withdrawableTokens[_owner()] += commitment.operationFeeJuels;


Is there already a proposal for concrete values of all 3 fees in prod? Can we discuss that in the Config sheet maybe?

It's in another PR in tooling. It will be added to the Config sheet once finalized.

bolekk · 2024-02-20T01:23:04Z

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

      emit RequestBilled({
        requestId: requestId,
        juelsPerGas: juelsPerGas,
        l1FeeShareWei: l1FeeShareWei,
        callbackCostJuels: callbackCostJuels,
-        totalCostJuels: gasOverheadJuels + callbackCostJuels + commitment.donFee + commitment.adminFee
+        donFeeJuels: commitment.donFeeJuels,


Will this affect Atlas or other data collection? Any other event changes that we are coordinating (or will) with other teams?)

It will not. I already double checked that Atlas only uses Router events.

bolekk · 2024-02-20T01:27:28Z

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol

-    commitment = FunctionsResponse.Commitment({
-      adminFee: request.adminFee,
+    commitment = FunctionsResponse.CommitmentWithOperationFee({
+      adminFeeJuels: request.adminFee,


Since we are deprecating adminFee (right?), could we save all this effort with a new Commitment struct by putting the new operationFee into adminFee field? Still passing a zero to the router so everything matches there. Not the most elegant approach but maybe a lot simpler?

I hadn't considered this. I'd be very curious to know if this is possible as it would be great to eliminate the changes to the log_poller_wrapper.

bolekk · 2024-02-20T01:33:09Z

core/services/ocr2/plugins/functions/integration_tests/v1/internal/testutils.go

@@ -186,6 +186,10 @@ func StartNewChainWithContracts(t *testing.T, nClients int) (*bind.TransactOpts,
 	linkEthFeedAddr, _, _, err := mock_v3_aggregator_contract.DeployMockV3AggregatorContract(owner, b, 18, big.NewInt(5_000_000_000_000_000))
 	require.NoError(t, err)



If you can, please preserve existing tests without changes and only add new ones. We can remove obsolete ones later, once prod is fully migrated.

bolekk · 2024-02-20T06:00:09Z

core/services/relay/evm/functions/logpoller_wrapper.go

 		if err != nil {
-			l.lggr.Error("LatestEvents: creating a contract instance for parsing failed")


Keep this log line (with an appropriate message).

bolekk · 2024-02-20T06:00:24Z

core/services/relay/evm/functions/logpoller_wrapper.go

+		resultsReq = append(resultsReq, requests...)
+		responses, err := coordinator.LogsToResponses(responseLogs)
+		if err != nil {
+			return nil, nil, err


Same here - log something with a "LatestEvents" prefix

bolekk · 2024-02-20T06:04:31Z

core/services/relay/evm/functions/logpoller_wrapper.go

 		l.lggr.Error("LogPollerWrapper: cannot update activeCoordinator to zero address")
 		return
 	}

-	if activeCoordinator == l.activeCoordinator && proposedCoordinator == l.proposedCoordinator {
+	if (l.activeCoordinator != nil && l.activeCoordinator.Address() == activeCoordinatorAddress) &&
+		(l.proposedCoordinator != nil && l.proposedCoordinator.Address() == proposedCoordinatorAddress) {


If proposedCoordinator == nil we never get inside this block but often we should. @agparadiso

ok changed to:

if (l.activeCoordinator != nil && l.activeCoordinator.Address() == activeCoordinatorAddress) && (l.proposedCoordinator == nil || l.proposedCoordinator.Address() == proposedCoordinatorAddress) { l.lggr.Debug("LogPollerWrapper: no changes to routes") return }

that would contemplate both scenarios

bolekk · 2024-02-20T06:07:02Z

core/services/relay/evm/functions/coordinator_v2.go

+
+	return c.logPoller.RegisterFilter(
+		logpoller.Filter{
+			Name: logpoller.FilterName("FunctionsLogPollerWrapper", c.address.String(), "-v", "2"),


Move string to a const used by both files. Also you could encode "-v2" in the prefix.

bolekk · 2024-02-20T06:07:44Z

core/services/relay/evm/functions/logpoller_wrapper.go

+		return
+	}
+
+	var proposedCoordinator Coordinator


Can we add a helper method to avoid code duplication?

cl-sonarqube-production · 2024-02-20T18:07:19Z

Quality Gate passed

Issues
0 New issues
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
82.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

justinkaseman · 2024-02-21T17:21:12Z

Closing in favor of #12104

justinkaseman requested review from a team as code owners February 12, 2024 18:31

justinkaseman temporarily deployed to sdlc February 12, 2024 18:32 — with GitHub Actions Inactive

justinkaseman force-pushed the FUN-1234 branch from 6dbe81c to 32f1943 Compare February 12, 2024 18:35

justinkaseman temporarily deployed to sdlc February 12, 2024 18:35 — with GitHub Actions Inactive

justinkaseman force-pushed the FUN-1234 branch from 32f1943 to 81b7d6d Compare February 12, 2024 18:38

justinkaseman temporarily deployed to sdlc February 12, 2024 18:38 — with GitHub Actions Inactive

justinkaseman temporarily deployed to sdlc February 12, 2024 20:19 — with GitHub Actions Inactive

RensR reviewed Feb 13, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol Outdated Show resolved Hide resolved

RensR reviewed Feb 13, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/interfaces/IFunctionsBilling.sol Outdated Show resolved Hide resolved

KuphJr reviewed Feb 15, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol Outdated Show resolved Hide resolved

KuphJr reviewed Feb 15, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol Outdated Show resolved Hide resolved

KuphJr reviewed Feb 15, 2024

View reviewed changes

justinkaseman temporarily deployed to sdlc February 15, 2024 19:09 — with GitHub Actions Inactive

agparadiso reviewed Feb 15, 2024

View reviewed changes

core/services/relay/evm/functions/logpoller_wrapper.go Outdated Show resolved Hide resolved

justinkaseman temporarily deployed to sdlc February 15, 2024 20:40 — with GitHub Actions Inactive

justinkaseman temporarily deployed to sdlc February 15, 2024 21:28 — with GitHub Actions Inactive

justinkaseman force-pushed the FUN-1234 branch 2 times, most recently from c59aa80 to fa6410a Compare February 16, 2024 00:46

justinkaseman had a problem deploying to sdlc February 16, 2024 00:46 — with GitHub Actions Failure

justinkaseman temporarily deployed to sdlc February 16, 2024 02:07 — with GitHub Actions Inactive

justinkaseman temporarily deployed to sdlc February 17, 2024 01:43 — with GitHub Actions Inactive

RensR reviewed Feb 19, 2024

View reviewed changes

contracts/scripts/native_solc_compile_all_functions Outdated Show resolved Hide resolved

RensR reviewed Feb 19, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/FunctionsBilling.sol Outdated Show resolved Hide resolved

RensR reviewed Feb 19, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/interfaces/IFunctionsBilling.sol Outdated Show resolved Hide resolved

RensR reviewed Feb 19, 2024

View reviewed changes

contracts/src/v0.8/functions/dev/v1_X/libraries/FunctionsResponse.sol Outdated Show resolved Hide resolved

smartcontractkit deleted a comment from justinkaseman Feb 19, 2024

FUN-1234 (feat): Chainlink Functions premium fees in USD denomination

b2927a3

justinkaseman temporarily deployed to sdlc February 19, 2024 21:00 — with GitHub Actions Inactive

justinkaseman temporarily deployed to sdlc February 19, 2024 21:21 — with GitHub Actions Inactive

justinkaseman temporarily deployed to sdlc February 19, 2024 21:40 — with GitHub Actions Inactive

justinkaseman force-pushed the FUN-1234 branch from 2e6532c to f5afdb8 Compare February 19, 2024 22:05

justinkaseman temporarily deployed to sdlc February 19, 2024 22:05 — with GitHub Actions Inactive

KuphJr previously approved these changes Feb 19, 2024

View reviewed changes

justinkaseman dismissed KuphJr’s stale review via 2adfc1b February 19, 2024 23:26

justinkaseman temporarily deployed to sdlc February 19, 2024 23:26 — with GitHub Actions Inactive

KuphJr reviewed Feb 19, 2024

View reviewed changes

bolekk reviewed Feb 20, 2024

View reviewed changes

chore: internal abitypes to avoid pkg variables and init function

753122a

agparadiso temporarily deployed to sdlc February 20, 2024 12:11 — with GitHub Actions Inactive

chore: reuse abi.Arguments to remove duplication

688b5f4

agparadiso temporarily deployed to sdlc February 20, 2024 14:55 — with GitHub Actions Inactive

chore: rollback func addr() change

bcc39f2

agparadiso temporarily deployed to sdlc February 20, 2024 15:14 — with GitHub Actions Inactive

chore: improve logs

a1d93bc

agparadiso temporarily deployed to sdlc February 20, 2024 15:35 — with GitHub Actions Inactive

fix: no changes if proposed is nil

361893f

agparadiso temporarily deployed to sdlc February 20, 2024 15:46 — with GitHub Actions Inactive

chore: extract building of versioned coordinator to a helper function

4991b13

agparadiso temporarily deployed to sdlc February 20, 2024 16:26 — with GitHub Actions Inactive

chore: simplify Coordinator interface

2c47244

agparadiso temporarily deployed to sdlc February 20, 2024 16:46 — with GitHub Actions Inactive

chore: extract FilterName to a const

6ace734

agparadiso temporarily deployed to sdlc February 20, 2024 17:52 — with GitHub Actions Inactive

justinkaseman closed this Feb 21, 2024

		@@ -186,6 +186,10 @@ func StartNewChainWithContracts(t testing.T, nClients int) (bind.TransactOpts,
		linkEthFeedAddr, _, _, err := mock_v3_aggregator_contract.DeployMockV3AggregatorContract(owner, b, 18, big.NewInt(5_000_000_000_000_000))
		require.NoError(t, err)


		func (l *logPollerWrapper) logsToResponses(coordinator coordinator, responseLogs []logpoller.Log) ([]evmRelayTypes.OracleResponse, error) {
		var responses []evmRelayTypes.OracleResponse

		if err != nil {
		l.lggr.Error("LatestEvents: creating a contract instance for parsing failed")

FUN-1234 (feat): Chainlink Functions premium fees in USD denomination #12000

FUN-1234 (feat): Chainlink Functions premium fees in USD denomination #12000

Conversation

justinkaseman commented Feb 12, 2024 • edited Loading

github-actions bot commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkaseman Feb 19, 2024 • 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

justinkaseman Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KuphJr Feb 15, 2024 • 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

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

bolekk left a 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 Feb 20, 2024 • 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

KuphJr Feb 20, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Feb 20, 2024

Quality Gate passed

justinkaseman commented Feb 21, 2024

justinkaseman commented Feb 12, 2024 •

edited

Loading

justinkaseman Feb 19, 2024 •

edited

Loading

justinkaseman Feb 16, 2024 •

edited

Loading

KuphJr Feb 15, 2024 •

edited

Loading

justinkaseman Feb 20, 2024 •

edited

Loading

KuphJr Feb 20, 2024 •

edited

Loading