Conversation
Co-authored-by: Chris Li <chris@avaprotocol.org>
PR Review: Value-Capture Fee Estimation and BillingThis is a well-structured refactor replacing the old monitoring/execution rate model with a cleaner three-component fee model (execution_fee + COGS + value_fee). The tiered value-capture design and documentation are clear. Below are issues ranging from correctness bugs to minor nits. Bugs / Correctness1. TOCTOU race in credit gating (executor.go) The credit check reads the outstanding balance and then lets execution proceed with no lock held between them. On a multi-goroutine executor, two tasks for the same owner can both pass the check before either records a fee. For V1 where fees are not recorded automatically this is moot in practice, but the check is technically racy and will matter once V2 recording lands. Consider per-owner advisory locking or at minimum document the known gap. 2. FeeLedger.RecordValueFee idempotency check is not atomic (fee_ledger.go) The code does a GetKey check and then a separate BatchWrite. Two concurrent calls with the same ExecutionID can both see existingRecord == nil and both write, doubling the balance. BadgerDB supports transactions; the idempotency check and the batch write should be a single transaction. 3. buildValueFee / buildCOGSFromSteps misclassify LoopNode as always on-chain A loop can contain only off-chain operations (custom code, contract reads). Classifying NODE_TYPE_LOOP as on-chain unconditionally inflates COGS entries and applies value-capture fees to pure off-chain loops. The same issue exists in isOnChainNode in fee_estimator.go. 4. resolveRunnerAndWalletCreation now errors on CodeAt failure (breaking behavior change) Previously the estimator logged a warning and assumed the wallet exists (safe fallback). Now it returns an error and the entire EstimateFees call fails. If the RPC is briefly degraded, fee estimation becomes completely unavailable. The old approach was more resilient; consider restoring the fallback with a warning entry in the response. Precision / Financial Arithmetic5. ConvertUSDToWei uses big.NewFloat(usdAmount) floating-point precision loss big.NewFloat inherits precision from the float64 mantissa (53 bits). For amounts like 0.02 this introduces a small but non-zero rounding error in the Wei result. The test uses a plus-or-minus 1 wei tolerance which hints the author noticed this. Consider new(big.Float).SetPrec(256) or represent fee amounts as integer micro-dollars to avoid the issue. Dead Code6. DiscountRules / discountRules field retained but unused FeeEstimator.discountRules is set in both constructors but never read. EstimateFees returns an empty Discounts slice unconditionally. Either remove discountRules and getDefaultDiscountRules() or add a TODO comment to make the intent clear. 7. Duplicate godoc on wrapWithReimbursement (builder.go) The function now has two comment blocks attached to it, the original one-liner and the new expanded block. Remove the original line. Minor Issues8. buildExecutionFee allocates on every call buildExecutionFee(n.config.FeeRates) is called per execution and allocates a new FeeRates struct each time via convertFeeRatesConfig. Since FeeRatesConfig is immutable after startup, convert once at initialization and cache. 9. fee_ledger_test.go uses a custom setupTestDB instead of testutil.TestMustDB() The rest of the codebase uses testutil.TestMustDB() for test storage. The local helper diverges from the convention and won't benefit from future improvements to the shared helper. 10. FeeRecord.Tier is a raw string (stringly-typed) The test sets Tier: "EXECUTION_TIER_1" as a raw string. This will silently diverge from the proto enum's string representation if the enum name ever changes. Use avsproto.ExecutionTier_EXECUTION_TIER_1.String() or store the int32 value. 11. Credit-limit Wei conversion is ETH-price-dependent CreditLimitUSD is converted to Wei via ConvertUSDToWei at check time, so a sharp drop in ETH price lowers the Wei threshold and could unexpectedly block users. This may be intentional, but deserves a comment since the behavior is non-obvious. Positive Highlights
|
… docker auto-build Fee system fixes: - Add mutex to FeeLedger.RecordValueFee to prevent double-recording on concurrent calls - Classify loop nodes based on inner runner type (contract_write/eth_transfer = on-chain, custom_code/contract_read/rest_api/graphql = off-chain) instead of treating all loops as on-chain - Restore CodeAt fallback in fee estimator — log warning and assume wallet exists on RPC failure instead of failing the entire EstimateFees call - Document known TOCTOU race in executor credit gating (acceptable for V1) - Use testutil.TestMustDB() in fee_ledger_test.go to match codebase convention - Use proto enum .String() for FeeRecord.Tier instead of raw string literal - Remove duplicate godoc on wrapWithReimbursement Dev docker workflow: - Auto-build avaprotocol/avs-dev:latest on staging push when protobuf/avs.proto changes - Use blacksmith-4vcpu-ubuntu-2404 runner - Add concurrency group to cancel in-flight builds - Auto-builds use amd64-only (fast) for CI; manual dispatch unchanged
Summary
Replaces the old monitoring/execution rate model with a three-component fee system and adds a value fee ledger for post-paid billing.
Fee Model (three components per execution)
Fee Estimation API (
EstimateFees)eth_estimateGasagainst the factoryFee Billing (execution path)
wrapWithReimbursementextended to include execution fee transfer alongside gas reimbursement, atomically in a singleexecuteBatchWithValuescallbuildExecutionFee,buildValueFee,buildCOGSFromStepshelpers for post-execution fee assembly from real gas receiptsValue Fee Ledger (
FeeLedger)FeeRecordaudit trail per executionCheckCreditLimitfor pre-execution credit gating (documented TOCTOU gap, acceptable for V1)Loop Node Classification
Dev Docker Workflow
avaprotocol/avs-dev:lateston staging push whenprotobuf/avs.protochangesblacksmith-4vcpu-ubuntu-2404)Config
FeeRatesConfigin aggregator YAML with per-tier percentages and credit limit*float64) to distinguish missing from explicit zeroCreditLimitUSDfor pre-execution gating (converted to WEI at check time)Changed Files
protobuf/avs.proto/avs.pb.go— Fee, NodeCOGS, ValueFee, FeeDiscount, EstimateFees messagescore/taskengine/fee_estimator.go— Fee estimation with COGS, value classification, loop-aware on-chain detectioncore/taskengine/fee_ledger.go— Value fee ledger with mutex-protected idempotent recordingcore/taskengine/executor.go— Credit gating, fee breakdown assemblycore/taskengine/engine.go— Fee estimation API endpoint, fee breakdown in execution recordspkg/erc4337/preset/builder.go— Atomic execution fee + gas reimbursement collectioncore/config/config.go— FeeRatesConfig with credit limit.github/workflows/publish-dev-docker.yml— Auto-build on proto changes, Blacksmith runnerdocs/FEE_ESTIMATION.md— Fee model documentation