CRE-468 Implement metering for write targets #54

patrickhuie19 · 2025-06-24T00:54:08Z

Description

CRE-468

Implement metering for the write targets. Will require chain specific implementations of Target Strategy to be updated for GetEstimateFee and GetTargetStrategy.

A few things need to happen:

Parse spend limits from the capability request if present
If limits are present, perform an estimation of the gas cost. If the estimate is greater than the limit, error out with an InsufficientFunds error
After the transaction has been included in a block, convert the inclusion fee from the native gas unit to the native chain currency
Return this native chain currency fee to the capability response

This PR cannot break current df workflows. I've removed the estimation check against limits to keep things operationally simple until we have a good reason to start supplying those limits.

Requires Dependencies

Resolves Dependencies

Unheilbar · 2025-06-24T01:26:30Z

capabilities/writetarget/write_target.go

+		return nil, 0, fmt.Errorf("invalid gas spend limit format: %s", spendLimit)
+	}
+
+	// Multiply by 10^decimals to convert from ETH to wei


Since it's a generalized component we shouldn't mention chain-specific terms (ETH, wei) here. It's a conversion from native unit to small unit (or something like this)

Mm, I agree. Should this conversion logic also live with the specific target strategy implementer? If so, we need to find a way to plumb that through

capabilities/writetarget/write_target.go

Unheilbar · 2025-06-25T17:49:47Z

There are two options to get Transaction Cost:

GetEstimateFee which simulates tx execution and provides estimated value in native currency. (should be compared with user-defined value)
GetTransactionFee which provides inclusion fee in native currency once we have transaction receipt. (should be returned in capability response
I think both of them should be provided by TargetStrategy interface.
cc @ilija42

…patible estimate check fixed

patrickhuie19 · 2025-06-26T15:12:12Z

There are two options to get Transaction Cost:

1. [GetEstimateFee](https://github.com/smartcontractkit/chainlink-common/blob/5613187324ad638bb25fb5ce65ddc515bc454c25/pkg/types/contract_writer.go#L34C1-L34C139) which simulates tx execution and provides estimated value in native currency. (should be compared with user-defined value)

2. [GetTransactionFee](https://github.com/smartcontractkit/chainlink-common/blob/5613187324ad638bb25fb5ce65ddc515bc454c25/pkg/types/relayer.go#L142) which provides inclusion fee  in native currency once we have transaction receipt. (should be returned in capability response
   I think both of them should be provided by TargetStrategy interface.
   cc @ilija42

After discussing offline, I agree - 4ef7f71

capabilities/writetarget/write_target.go

Unheilbar · 2025-06-26T15:43:54Z

capabilities/writetarget/write_target.go

 	// Wrapper around the ChainWriter to get the transaction status
 	GetTransactionStatus(ctx context.Context, transactionID string) (commontypes.TransactionStatus, error)
+	// Wrapper around the ChainWriter to get the fee esimate
+	GetEstimateFee(ctx context.Context, contract string, method string, args any, toAddress string, meta *commontypes.TxMeta, val *big.Int) (commontypes.EstimateFee, error)


So here signature should be

GetEstimateFee(ctx context.Context, report []byte, reportContext []byte, signatures [][]byte, request capabilities.CapabilityRequest) (commontypes.EstimateFee, error)

since TargetStrategy handles building payload for CW

aren't all of these report []byte, reportContext []byte, signatures [][]byte, part of the capability request?

type SignedReport struct { Report []byte // Report context is appended to the report before signing by libOCR. // It contains config digest + round/epoch/sequence numbers (currently 96 bytes). // Has to be appended to the report before validating signatures. Context []byte // Always exactly F+1 signatures. Signatures [][]byte // Report ID defined in the workflow spec (2 bytes). ID []byte }

Yeah, just checked other arguments are unused by TransmitReport, idk why they are declared there. I guess the only thing is needed is the capability request

I'd recommend we stay consistent for this PR and then migrate all the signatures in one swoop

… in framework

Unheilbar · 2025-06-30T13:57:35Z

capabilities/writetarget/write_target.go

+			}
+			errMsg := c.asEmittedError(ctx, &wt.WriteError{
+				Code:    uint32(TransmissionStateFatal),
+				Summary: "InsufficientFunds",


Summary: "InsufficientFunds",
based on writeTarget.checkGasEstimate there are other possible errors:

fee, err := c.targetStrategy.GetEstimateFee(ctx, report, reportContext, signatures, request) if err != nil { return nil, 0, fmt.Errorf("failed to get gas estimate: %w", err) } // Convert spend limit from chain currency to gas units limitFloat, ok := new(big.Float).SetString(spendLimit) if !ok { return nil, 0, fmt.Errorf("invalid gas spend limit format: %s", spendLimit) }

failed to get gas estimate doesnt match Summary:"InsufficientFunds", also it's retryable, so code should be Failed, not Fatal.
I think all this errors should be handled differently

Unheilbar · 2025-06-30T14:13:28Z

capabilities/writetarget/write_target.go

+	// Get the transaction fee
+	fee, err := c.targetStrategy.GetTransactionFee(ctx, txID)
+	if err != nil {
+		return capabilities.CapabilityResponse{}, fmt.Errorf("failed to get transaction fee: %w", err)


hmm, at this point TX was already sent, we collected the receipt and confirmed successful transmission state, but here we return err since we couldn't get transaction fee (and since receipt is already in DB the only case for that, - we lost db connection). Should we return err in this case? Maybe just log and return estimation value from above?

Yes, for the ST3 use cases, we can do that.

silaslenihan · 2025-06-30T14:48:29Z

capabilities/writetarget/write_target.go

+			return limit.Limit, nil
+		}
+	}
+	return "", fmt.Errorf("no gas spend limit found for chain %s", c.chainInfo.ChainID)


Why is this error necessary? Maybe makes more sense just to be a debug log

getGasSpendLimit is complex enough to demand an error in the response API - its up to the Execute author to handle it properly. Currently, Execute handles it by skipping the check against gas estimation.

silaslenihan · 2025-06-30T15:05:33Z

capabilities/writetarget/write_target.go

+
+	// Compare estimate with limit
+	if fee.Fee.Cmp(limit) > 0 {
+		return nil, 0, fmt.Errorf("estimated gas fee %s exceeds spend limit %s", fee.Fee.String(), limit.String())


Where is this limit specified? Would this be in the workflow? And if so does the workflow do any sort of check to ensure the user actually has those funds?

The engine works with the billing service and a hierarchy of user defined limits to determine what to pass to the capabilities. Once a limit is passed to the capability, we ask that capabilities do their best to estimate the cost of execution upfront and stay within that cost to prevent double spend in the workflow owner account.

silaslenihan · 2025-06-30T15:24:23Z

capabilities/writetarget/write_target.go

 	info.reportInfo.reportID = binary.BigEndian.Uint16(inputs.ID)

+	// Get gas spend limit first
+	spendLimit, err := c.getGasSpendLimit(request)


how is this spend limit enforced when sending a tx?

I don't think it's enforced, it's a rough estimate that doesn't consider that chain state can change/txm can bump tx prority fee.

Not enforced; we can't block NOP keys on sending transactions.

ilija42 · 2025-07-02T16:00:27Z

capabilities/writetarget/write_target.go

+	GetEstimateFee(ctx context.Context, report []byte, reportContext []byte, signatures [][]byte, request capabilities.CapabilityRequest) (commontypes.EstimateFee, error)
+	// GetTransactionFee retrieves the actual transaction fee in native currency from the transaction receipt.
+	// This method should be implemented by chain-specific services and handle the conversion of gas units to native currency.
+	GetTransactionFee(ctx context.Context, transactionID string) (decimal.Decimal, error)


Gas values are always returned as big.Int, I'm assuming this value will be created like this: decimal.New(val, -18)? Could be annoying converting it back to big.Int

ilija42

lgtm, lets just wait for TargetStrategy

WIP

986a9a2

patrickhuie19 force-pushed the feature/CRE-468/EVM_WT_Metering branch from ebb60b4 to 986a9a2 Compare June 24, 2025 00:56

Unheilbar requested changes Jun 24, 2025

View reviewed changes

GetTransactionFee wired through TargetStrategy iface; backwards incom…

4ef7f71

…patible estimate check fixed

patrickhuie19 commented Jun 26, 2025

View reviewed changes

capabilities/writetarget/write_target.go Outdated Show resolved Hide resolved

Unheilbar reviewed Jun 26, 2025

View reviewed changes

patrickhuie19 added 4 commits June 26, 2025 18:01

new signature for estimate fee; unit tests; mocks

c96de38

lint

58bfe4b

Merge branch 'main' into feature/CRE-468/EVM_WT_Metering

8956220

GetEstimateFee continues to return fee + decimals; conversion handled…

d893172

… in framework

patrickhuie19 marked this pull request as ready for review June 30, 2025 13:56

patrickhuie19 requested a review from a team as a code owner June 30, 2025 13:56

Unheilbar reviewed Jun 30, 2025

View reviewed changes

silaslenihan reviewed Jun 30, 2025

View reviewed changes

removing gas estimation checks

35e1e19

patrickhuie19 changed the title ~~WIP~~ CRE-468 Implement metering for write targets Jul 1, 2025

patrickhuie19 requested review from Unheilbar, ilija42 and silaslenihan July 1, 2025 23:23

patrickhuie19 added 4 commits July 1, 2025 19:26

lint

81fa437

lint

8b5a8d4

removing gas estimation from unit tests

18be406

lint

4c237fd

silaslenihan approved these changes Jul 2, 2025

View reviewed changes

ilija42 reviewed Jul 2, 2025

View reviewed changes

Unheilbar mentioned this pull request Jul 2, 2025

update target strategy with biling related calls smartcontractkit/chainlink#18448

Merged

Unheilbar approved these changes Jul 2, 2025

View reviewed changes

ilija42 approved these changes Jul 2, 2025

View reviewed changes

patrickhuie19 merged commit 3f9ae62 into main Jul 2, 2025
22 checks passed

patrickhuie19 deleted the feature/CRE-468/EVM_WT_Metering branch July 2, 2025 18:33

CRE-468 Implement metering for write targets #54

CRE-468 Implement metering for write targets #54

Uh oh!

Conversation

patrickhuie19 commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Requires Dependencies

Resolves Dependencies

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Unheilbar commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhuie19 commented Jun 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilija42 Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilija42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patrickhuie19 commented Jun 24, 2025 •

edited

Loading

Unheilbar commented Jun 25, 2025 •

edited

Loading

ilija42 Jul 2, 2025 •

edited

Loading

ilija42 left a comment •

edited

Loading