Functions: tracking subscriptions #10613

pinebit · 2023-09-13T09:46:42Z

No description provided.

github-actions · 2023-09-13T09:47:03Z

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

KuphJr · 2023-09-13T16:54:08Z

core/scripts/functions/templates/oracle.toml

@@ -43,6 +43,13 @@ maxSecretsSizesList = [10_240, 20_480, 51_200, 102_400, 307_200, 512_000, 1_048_
  updateFrequencySec = 30
  updateTimeoutSec = 10

+  [pluginConfig.OnchainSubscriptions]
+  blockConfirmations = 1
+  routerAddress = "{{router_contract_address}}"


We have multiple places in the jobspec where there are duplicate definitions of the router contract address. How hard would it be to exclusively use the router address specified under contractID everywhere?
(This is out of scope for this PR, but may be a good backlog ticket as it may add misconfiguration risk)

Agreed, this would be a good improvement.

Add a ticket to tech debt epic please!

KuphJr · 2023-09-13T16:57:12Z

core/services/gateway/handlers/functions/subscriptions.go

+	utils.StartStopOnce
+
+	config             OnchainSubscriptionsConfig
+	subscriptions      map[common.Address]*functions_router.IFunctionsSubscriptionsSubscription


One user could have many subscriptions. Therefore, how are we planning to approach this if we are only mapping each user's address to 1 subscription? Are we only going to keep the subscription with the maximum balance for each user?

Good point, İ need to improve this

We should store all of them.

KuphJr · 2023-09-13T17:08:04Z

core/services/gateway/handlers/functions/subscriptions.go

+			return
+		}
+		if count == 0 {
+			s.lggr.Info("Router has no subscriptions yet")


The current contract implementation reverts if there are zero subscriptions. https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/functions/dev/1_0_0/FunctionsSubscriptions.sol#L269

We will log errors when that happens right?

KuphJr · 2023-09-13T17:16:00Z

core/services/gateway/handlers/functions/subscriptions.go

+	defer s.rwMutex.Unlock()
+	for _, subscription := range subscriptions {
+		if subscription.Owner == utils.ZeroAddress {
+			continue


When would this case happen? Should we log an error here?

If subscription is cancelled I guess. I need to test it better though

Yes, this is for cancelled subs. But we shouldn't continue, we need to still store it as cancelled. Otherwise we'll keep using previous values.

core/services/gateway/handlers/functions/subscriptions.go

KuphJr · 2023-09-13T17:18:35Z

core/services/gateway/handlers/functions/subscriptions.go

+			continue
+		}
+		subscription := subscription
+		s.subscriptions[subscription.Owner] = &subscription


Maybe we should check the balance and only save the subscription with the highest balance here? Although maybe we should also re-work this as we are currently holding the rwMutext here.

Let's save all but have an API to retrieve highest balance. In the future we are also going to look at flags stored within subscriptions.

bolekk · 2023-09-13T23:17:52Z

core/scripts/functions/templates/oracle.toml

@@ -43,6 +43,13 @@ maxSecretsSizesList = [10_240, 20_480, 51_200, 102_400, 307_200, 512_000, 1_048_
  updateFrequencySec = 30
  updateTimeoutSec = 10

+  [pluginConfig.OnchainSubscriptions]
+  blockConfirmations = 1
+  routerAddress = "{{router_contract_address}}"


Add a ticket to tech debt epic please!

bolekk · 2023-09-13T23:19:20Z

core/services/functions/connector_handler.go

@@ -68,6 +70,10 @@ func (h *functionsConnectorHandler) HandleGatewayMessage(ctx context.Context, ga
 		h.lggr.Errorw("request rate-limited", "id", gatewayId, "address", fromAddr)
 		return
 	}
+	if h.subscriptions.GetSubscription(fromAddr) == nil {


This needs to check against sub balance. So we need required balance in the config and probably a more specific API than GetSubscription().

bolekk · 2023-09-13T23:20:29Z

core/services/gateway/handlers/functions/subscriptions.go

+type OnchainSubscriptionsConfig struct {
+	RouterAddress      common.Address `json:"routerAddress"`
+	BlockConfirmations uint           `json:"blockConfirmations"`
+	QueryFrequencySec  uint           `json:"queryFrequencySec"`


To follow allowlist config fields, I'd suggest "UpdateFrequencySec", "UpdateTimeoutSec" and "UpdateRangeSec".

bolekk · 2023-09-13T23:21:19Z

core/services/gateway/handlers/functions/subscriptions.go

+)
+
+type OnchainSubscriptionsConfig struct {
+	RouterAddress      common.Address `json:"routerAddress"`


Allowlist has "ContractAddress" but "Router" is slightly better. So up to you if you prefer consistency or a slightly more specific name.

core/services/gateway/handlers/functions/subscriptions.go

bolekk · 2023-09-13T23:23:23Z

core/services/gateway/handlers/functions/subscriptions.go

+	utils.StartStopOnce
+
+	config             OnchainSubscriptionsConfig
+	subscriptions      map[common.Address]*functions_router.IFunctionsSubscriptionsSubscription


We should store all of them.

bolekk · 2023-09-13T23:25:44Z

core/services/gateway/handlers/functions/subscriptions.go

+
+		count, err := s.getSubscriptionsCount(ctx, blockNumber)
+		if err != nil {
+			s.lggr.Errorw("Error getting subscriptions count", "err", err)


We could cache this and use previous value if fetching fails.

core/services/gateway/handlers/functions/subscriptions.go

bolekk · 2023-09-13T23:28:14Z

core/services/gateway/handlers/functions/subscriptions.go

+	defer s.rwMutex.Unlock()
+	for _, subscription := range subscriptions {
+		if subscription.Owner == utils.ZeroAddress {
+			continue


Yes, this is for cancelled subs. But we shouldn't continue, we need to still store it as cancelled. Otherwise we'll keep using previous values.

bolekk · 2023-09-13T23:29:30Z

core/services/gateway/handlers/functions/subscriptions.go

+			continue
+		}
+		subscription := subscription
+		s.subscriptions[subscription.Owner] = &subscription


Let's save all but have an API to retrieve highest balance. In the future we are also going to look at flags stored within subscriptions.

bolekk · 2023-09-13T23:30:39Z

core/services/functions/connector_handler.go

-func NewFunctionsConnectorHandler(nodeAddress string, signerKey *ecdsa.PrivateKey, storage s4.Storage, allowlist functions.OnchainAllowlist, rateLimiter *hc.RateLimiter, lggr logger.Logger) (*functionsConnectorHandler, error) {
-	if signerKey == nil || storage == nil || allowlist == nil || rateLimiter == nil {
-		return nil, fmt.Errorf("signerKey, storage, allowlist and rateLimiter must be non-nil")
+func NewFunctionsConnectorHandler(nodeAddress string, signerKey *ecdsa.PrivateKey, storage s4.Storage, allowlist functions.OnchainAllowlist, rateLimiter *hc.RateLimiter, subscriptions functions.OnchainSubscriptions, lggr logger.Logger) (*functionsConnectorHandler, error) {


Planning to also add to the Gateway handler?

Do you think it needs to be enabled in GW handler too?

I don't think it's a must. But it should be very easy to add there as optional and the more bad requests we can stop in Gateway, the less unnecessary load we put on the nodes.

KuphJr · 2023-09-14T15:25:14Z

core/services/gateway/handlers/functions/user_subscriptions.go

+			maxBalance = sub.Balance
+		}
+	}
+	fmt.Println(hex.EncodeToString(maxBalance.Bytes()))


KuphJr

One missed log line.

Other that that, my concerns have been addressed, so it LGTM. Bolek should review & make sure his comments have been addressed before we merge.

core/services/functions/connector_handler.go

bolekk · 2023-09-14T20:57:13Z

core/services/functions/connector_handler.go

@@ -68,6 +71,10 @@ func (h *functionsConnectorHandler) HandleGatewayMessage(ctx context.Context, ga
 		h.lggr.Errorw("request rate-limited", "id", gatewayId, "address", fromAddr)
 		return
 	}
+	if balance, err := h.subscriptions.GetMaxUserBalance(fromAddr); err != nil || balance.Cmp(big.NewInt(0)) == 0 {


We want to check against a configured minimum balance, not 0.

core/services/gateway/handlers/functions/subscriptions.go

bolekk · 2023-09-15T16:23:59Z

core/services/functions/connector_handler.go

 	}
+	minBalanceWei := new(big.Int)


It's not Wei but Juels.

bolekk · 2023-09-15T16:24:49Z

core/services/gateway/handlers/functions/handler.functions.go

+	pendingRequests   hc.RequestCache[PendingSecretsRequest]
+	allowlist         OnchainAllowlist
+	subscriptions     OnchainSubscriptions
+	minimumBalanceWei *big.Int


bolekk · 2023-09-15T16:26:46Z

core/services/gateway/handlers/functions/handler.functions.go

+		}
+	}
+	minimumBalanceWei := new(big.Int)
+	big.NewFloat(0).Mul(big.NewFloat(cfg.MinimumSubscriptionBalanceLink), big.NewFloat(1e18)).Int(minimumBalanceWei)


Maybe better to have Juels in config? I'm worried about precision.

changed to what we use in other cases like this: assets.Link.

bolekk · 2023-09-15T16:28:53Z

core/services/gateway/handlers/functions/subscriptions.go

+			return
+		}
+		if count == 0 {
+			s.lggr.Info("Router has no subscriptions yet")


We will log errors when that happens right?

bolekk · 2023-09-17T22:34:29Z

core/services/functions/connector_handler.go

@@ -69,6 +76,11 @@ func (h *functionsConnectorHandler) HandleGatewayMessage(ctx context.Context, ga
 		return
 	}

+	if balance, err := h.subscriptions.GetMaxUserBalance(fromAddr); err != nil || balance.Cmp(h.minBalanceWei) < 0 {
+		h.lggr.Errorw("request is not backed with a funded subscription", "id", gatewayId, "address", fromAddr)


Also log user balance and min required balance.

bolekk · 2023-09-17T22:40:40Z

core/services/gateway/handlers/functions/handler.functions.go

@@ -20,7 +21,11 @@ import (
 type FunctionsHandlerConfig struct {
 	OnchainAllowlistChainID string `json:"onchainAllowlistChainId"`
 	// Not specifying OnchainAllowlist config disables allowlist checks
-	OnchainAllowlist *OnchainAllowlistConfig `json:"onchainAllowlist"`
+	OnchainAllowlist            *OnchainAllowlistConfig `json:"onchainAllowlist"`
+	OnchainSubscriptionsChainID string                  `json:"onchainSubscriptionsChainId"`


I think we can have a single chainID config field for both the allowlist and subscriptions. Maybe rename the existing one to just "ChainId"?

core/services/gateway/handlers/functions/handler.functions.go

bolekk · 2023-09-17T22:42:32Z

core/services/gateway/handlers/functions/handler.functions.go

+	if h.subscriptions != nil {
+		if balance, err := h.subscriptions.GetMaxUserBalance(sender); err != nil || balance.Cmp(h.minimumBalanceWei) < 0 {
+			h.lggr.Debug("received a message from a user having insufficient balance", "sender", msg.Body.Sender, "balance", balance.String())
+			return errors.New("sender has insufficient balance")


Change the message to "insufficient subscription balance: [balance] [unit]"

bolekk · 2023-09-17T22:46:54Z

core/services/gateway/handlers/functions/subscriptions.go

+type OnchainSubscriptions interface {
+	job.ServiceCtx
+
+	// GetMaxUserBalance returns a maximum subscription balance, or error if user has no subscriptions.


Specify unit (juels)

bolekk · 2023-09-18T05:30:26Z

core/scripts/functions/templates/oracle.toml

@@ -35,6 +35,8 @@ requestTimeoutCheckFrequencySec = 10
 requestTimeoutSec = 300
 maxRequestSizesList = [30_720, 51_200, 102_400, 204_800, 512_000, 1_048_576, 2_097_152, 3_145_728, 5_242_880, 10_485_760]
 maxSecretsSizesList = [10_240, 20_480, 51_200, 102_400, 307_200, 512_000, 1_048_576, 2_097_152]
+minimumSubscriptionBalanceLink = 0.1


bolekk · 2023-09-18T05:33:29Z

core/services/functions/connector_handler.go

+	allowlist     functions.OnchainAllowlist
+	rateLimiter   *hc.RateLimiter
+	subscriptions functions.OnchainSubscriptions
+	minBalance    assets.Link


EDIT: ignore

cl-sonarqube-production · 2023-09-18T15:03:18Z

SonarQube Quality Gate

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

See analysis details on SonarQube

Functions: tracking subscriptions

a334b43

pinebit requested review from bolekk, justinkaseman and KuphJr September 13, 2023 09:46

Andrei Smirnov added 3 commits September 13, 2023 12:59

Fixing linter issue

1af8420

Bugfixing

d55b93e

Merge branch 'develop' into functions/subscriptions-tracking

9d352a7

KuphJr reviewed Sep 13, 2023

View reviewed changes

bolekk requested changes Sep 13, 2023

View reviewed changes

bolekk reviewed Sep 13, 2023

View reviewed changes

Andrei Smirnov added 6 commits September 14, 2023 15:22

Merge branch 'develop' into functions/subscriptions-tracking

cdd2a47

Reworking solution

4530867

Merge branch 'develop' into functions/subscriptions-tracking

8206cf1

Fixed tests

0ab8ee1

Fixed linter error

1c2477a

Merge branch 'develop' into functions/subscriptions-tracking

793c2c1

KuphJr reviewed Sep 14, 2023

View reviewed changes

KuphJr previously approved these changes Sep 14, 2023

View reviewed changes

bolekk requested changes Sep 14, 2023

View reviewed changes

Andrei Smirnov added 2 commits September 15, 2023 07:59

Merge branch 'develop' into functions/subscriptions-tracking

77d953c

Addressed PR feedback

c5ae2e5

pinebit dismissed KuphJr’s stale review via c5ae2e5 September 15, 2023 05:01

Andrei Smirnov added 2 commits September 15, 2023 08:24

Added minimum balance config

fa7eb52

Added subscriptions check to gateway

19aadcc

bolekk reviewed Sep 15, 2023

View reviewed changes

bolekk reviewed Sep 18, 2023

View reviewed changes

Andrei Smirnov added 2 commits September 18, 2023 07:43

Merge branch 'develop' into functions/subscriptions-tracking

c54e357

Using assets.Link

a344936

bolekk reviewed Sep 18, 2023

View reviewed changes

Andrei Smirnov added 2 commits September 18, 2023 08:35

Fixed template and naming

13984f9

Reverted renaming

a235b39

bolekk approved these changes Sep 18, 2023

View reviewed changes

pinebit enabled auto-merge September 18, 2023 05:38

Merge branch 'develop' into functions/subscriptions-tracking

ebed8ee

pinebit added this pull request to the merge queue Sep 18, 2023

Merged via the queue into develop with commit 1bb0c1a Sep 18, 2023
105 of 106 checks passed

pinebit deleted the functions/subscriptions-tracking branch September 18, 2023 15:16

snehaagni mentioned this pull request Oct 18, 2023

release/2.6.0 -> master #10997

Merged

Functions: tracking subscriptions #10613

Functions: tracking subscriptions #10613

Conversation

pinebit commented Sep 13, 2023

github-actions bot commented Sep 13, 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

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

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

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

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 Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Sep 18, 2023

KuphJr left a comment •

edited

Loading

bolekk Sep 18, 2023 •

edited

Loading