Introduce fee system using CEL formulas #860
Conversation
WalkthroughAdds a CEL-based dynamic fee estimator and integrates it into the service/config, replacing a static on-chain fee. Introduces an RPC POST /v1/batch/estimateFee, arkfee package with CEL environments/programs, a FeeManager port and ArkFee manager implementation, updated tests, and config/env variables to supply fee programs. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant gRPC as gRPC Handler
participant Parser as Intent Parser
participant Service as Core Service
participant FeeMgr as FeeManager (arkFeeManager)
participant Estimator as arkfee.Estimator
participant CEL as CEL Env/Evaluator
Client->>gRPC: EstimateFee(EstimateFeeRequest)
gRPC->>Parser: parseEstimateFeeIntent(intent)
Parser-->>gRPC: (proof, EstimateFeeMessage)
gRPC->>Service: EstimateFee(ctx, proof, message)
Service->>FeeMgr: GetIntentFees(ctx, boardingInputs, vtxoInputs, onchainOutputs, offchainOutputs)
FeeMgr->>Estimator: Eval(offchainInputs, onchainInputs, offchainOutputs, onchainOutputs)
Estimator->>CEL: Compile/Evaluate programs with variables
CEL-->>Estimator: FeeAmount per item
Estimator->>FeeMgr: Total FeeAmount
FeeMgr-->>Service: fee (satoshis)
Service-->>gRPC: EstimateFeeResponse(fee)
gRPC-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
pkg/ark-cli/main.go (2)
543-546: Consider removing the unusedcomputeExpirationvariable.The
computeExpirationvariable is read from the flag on Line 543 but is no longer passed toCollaborativeExiton Line 546. This suggests it may be unused in this code path.Apply this diff to remove the unused variable from this path:
- computeExpiration := ctx.Bool(enableExpiryCoinselectFlag.Name) if len(onchainReceivers) > 0 { txid, err := arkSdkClient.CollaborativeExit( ctx.Context, onchainReceivers[0].To, onchainReceivers[0].Amount,Note: If
computeExpirationis still needed for theSendOffChaincall on Line 554, the variable assignment should be moved after theif len(onchainReceivers) > 0block.
199-199: Consider removing the unusedexpiryDetailsFlagfrom the balance command.The
expiryDetailsFlagis still listed in the balance command's flags, but the flag value is no longer passed to theBalancemethod (Line 382). This suggests the flag may no longer be needed.Apply this diff if the flag is no longer used:
balanceCommand = cli.Command{ Name: "balance", Usage: "Shows onchain and offchain Ark wallet balance", Action: func(ctx *cli.Context) error { return balance(ctx) }, - Flags: []cli.Flag{expiryDetailsFlag}, }internal/interface/grpc/handlers/parser.go (1)
51-67: Estimate-fee intent parsing correctly mirrors register flow
parseEstimateFeeIntentreusesparseIntentProofTx, validates message presence, and delegates JSON/type validation toEstimateFeeMessage.Decode, which keeps the parsing path consistent with other intent types. You might optionally specialize the error message to"invalid estimate-fee intent message"for easier debugging, but the current behavior is functionally fine.pkg/ark-lib/arkfee/estimator_test.go (1)
76-86: Avoid strict float equality for weighted fee expectations
require.EqualonFeeAmountfor values like56.3depends on exact float representation of the CEL evaluation matching the literal, which can be brittle.Consider using a delta-based assertion instead, e.g.:
- require.Equal(t, testCase.expected, result) + require.InDelta(t, float64(testCase.expected), float64(result), 1e-9)and similarly for other non-integer expectations. This keeps the tests robust against minor floating‑point rounding differences while still tightly checking correctness.
Also applies to: 192-245
pkg/ark-lib/intent/message.go (1)
12-17: EstimateFeeMessage reuse of RegisterMessage structure with distinct type check looks soundDefining
EstimateFeeMessageon top ofRegisterMessageand validatingm.Type == IntentMessageTypeEstimateFeeinDecodecleanly reuses the existing message shape while preventing cross-use ofregisterpayloads on the estimate-fee path. As with the other messages, callers just need to ensureBaseMessage.Typeis set correctly beforeEncode.Also applies to: 62-82
api-spec/openapi/swagger/ark/v1/service.openapi.json (1)
90-130: EstimateFee OpenAPI path and schemas align with the new RPC; consider documenting required intent typeThe
/v1/batch/estimateFeeendpoint and itsEstimateFeeRequest/EstimateFeeResponseschemas are consistent with the existing intent-based APIs and correctly describe the fee as anint64in satoshis.To make client usage clearer, consider extending the description to note that the embedded
Intent’s message must carry{"type": "estimate-fee", ...}(matchingIntentMessageTypeEstimateFee), even though the payload shape mirrors the register intent.Also applies to: 633-651
pkg/ark-lib/arkfee/estimator.go (1)
79-99: Consider validating that individual fees are non-negative.The
Evalfunction aggregates fees from all inputs and outputs. If a CEL program returns a negative value, it could result in unexpected behavior (fees reducing the total or going negative). Depending on business requirements, you may want to add validation.If negative fees should not be allowed:
func (e *Estimator) Eval(inputs []Input, outputs []Output) (FeeAmount, error) { fee := FeeAmount(0) for _, input := range inputs { inputFee, err := e.EvalInput(input) if err != nil { return 0, err } + if inputFee < 0 { + return 0, fmt.Errorf("negative input fee not allowed: %v", inputFee) + } fee += inputFee } for _, output := range outputs { outputFee, err := e.EvalOutput(output) if err != nil { return 0, err } + if outputFee < 0 { + return 0, fmt.Errorf("negative output fee not allowed: %v", outputFee) + } fee += outputFee } return fee, nil }pkg/ark-lib/arkfee/types.go (1)
10-14: Consider adding validation for FeeAmount edge cases.The
ToSatoshis()method directly castsmath.Ceil(float64(f))toint64without checking for:
- Negative fee amounts (which would be invalid)
- Values exceeding
math.MaxInt64(causing overflow)func (f FeeAmount) ToSatoshis() int64 { + if f < 0 { + return 0 + } + ceiling := math.Ceil(float64(f)) + if ceiling > float64(math.MaxInt64) { + return math.MaxInt64 + } - return int64(math.Ceil(float64(f))) + return int64(ceiling) }internal/core/application/service.go (1)
1570-1574: Address the Weight calculation TODOs.The
Weightfield is hardcoded to0in multiple places with TODO comments. This field represents the "weighted liquidity lockup ratio" and is part of the fee calculation inputs. Leaving it at 0 may result in incorrect fee estimates if the CEL programs reference this variable.Do you want me to help implement the Weight calculation, or should this be tracked in a separate issue? The logic would depend on how you define the liquidity lockup ratio (e.g., based on vtxo age, amount, or exit delay).
Also applies to: 1692-1698
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sumpkg/ark-cli/go.sumis excluded by!**/*.sumpkg/ark-lib/go.sumis excluded by!**/*.sumpkg/errors/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
api-spec/openapi/swagger/ark/v1/service.openapi.json(2 hunks)api-spec/protobuf/ark/v1/service.proto(2 hunks)envs/arkd.light.env(1 hunks)go.mod(7 hunks)internal/config/config.go(9 hunks)internal/core/application/service.go(12 hunks)internal/core/application/types.go(1 hunks)internal/interface/grpc/handlers/arkservice.go(1 hunks)internal/interface/grpc/handlers/parser.go(1 hunks)internal/test/e2e/e2e_test.go(18 hunks)pkg/ark-cli/go.mod(4 hunks)pkg/ark-cli/main.go(4 hunks)pkg/ark-lib/arkfee/celenv/functions.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_input.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_output.go(1 hunks)pkg/ark-lib/arkfee/celenv/variables.go(1 hunks)pkg/ark-lib/arkfee/estimator.go(1 hunks)pkg/ark-lib/arkfee/estimator_test.go(1 hunks)pkg/ark-lib/arkfee/types.go(1 hunks)pkg/ark-lib/go.mod(2 hunks)pkg/ark-lib/intent/message.go(2 hunks)pkg/ark-lib/intent/message_test.go(1 hunks)pkg/ark-lib/intent/testdata/message_fixtures.json(1 hunks)pkg/errors/errors.go(1 hunks)pkg/errors/go.mod(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/errors/go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/core/application/service.go
🧬 Code graph analysis (7)
pkg/ark-lib/intent/message_test.go (1)
pkg/ark-lib/intent/message.go (2)
IntentMessageTypeEstimateFee(16-16)EstimateFeeMessage(62-62)
internal/interface/grpc/handlers/arkservice.go (2)
api-spec/protobuf/gen/ark/v1/service.pb.go (6)
EstimateFeeRequest(339-344)EstimateFeeRequest(357-357)EstimateFeeRequest(372-374)EstimateFeeResponse(383-388)EstimateFeeResponse(401-401)EstimateFeeResponse(416-418)pkg/errors/errors.go (1)
Error(39-46)
internal/core/application/types.go (3)
pkg/ark-lib/intent/proof.go (1)
Proof(39-41)pkg/ark-lib/intent/message.go (1)
EstimateFeeMessage(62-62)pkg/errors/errors.go (1)
Error(39-46)
pkg/ark-cli/main.go (2)
internal/core/application/admin.go (1)
Balance(484-487)api-spec/protobuf/gen/ark/v1/wallet.pb.go (3)
Balance(653-659)Balance(672-672)Balance(687-689)
pkg/ark-lib/arkfee/estimator.go (3)
pkg/ark-lib/arkfee/celenv/intent_input.go (1)
IntentInputEnv(7-7)pkg/ark-lib/arkfee/celenv/intent_output.go (1)
IntentOutputEnv(7-7)pkg/ark-lib/arkfee/types.go (4)
Input(25-31)FeeAmount(10-10)Output(55-58)OutputType(48-48)
internal/interface/grpc/handlers/parser.go (5)
internal/infrastructure/db/postgres/sqlc/queries/models.go (1)
Intent(33-38)internal/infrastructure/db/sqlite/sqlc/queries/models.go (1)
Intent(31-36)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)pkg/ark-lib/intent/proof.go (1)
Proof(39-41)pkg/ark-lib/intent/message.go (1)
EstimateFeeMessage(62-62)
pkg/ark-lib/arkfee/estimator_test.go (2)
pkg/ark-lib/arkfee/types.go (5)
Input(25-31)FeeAmount(10-10)InputTypeRecoverable(19-19)Output(55-58)OutputTypeOnchain(52-52)pkg/ark-lib/arkfee/estimator.go (1)
New(22-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: integration tests
- GitHub Check: unit tests
🔇 Additional comments (33)
pkg/errors/errors.go (1)
337-337: LGTM!The new error code follows the established pattern and uses an appropriate gRPC code (Internal) for fee evaluation failures.
api-spec/protobuf/ark/v1/service.proto (2)
27-35: LGTM!The new EstimateFee RPC endpoint follows the established patterns in the service definition and provides clear documentation.
186-191: LGTM!The EstimateFee message definitions are well-structured and consistent with other request/response pairs in the file. The use of int64 for the fee field is appropriate for representing satoshi amounts.
pkg/ark-cli/main.go (3)
207-214: LGTM!The formatting change improves readability by breaking the flags into multiple lines.
382-382: LGTM!The Balance method signature has been simplified to remove the compute-expiry parameter, aligning with the SDK changes.
424-426: LGTM!The CollaborativeExit method signature has been simplified to remove the compute-expiry parameter.
envs/arkd.light.env (1)
14-15: LGTM!The CEL fee program expressions are syntactically correct and demonstrate a practical fee structure:
- Input fees: waive fees for notes and recoverable inputs, charge 1% otherwise
- Output fees: charge a flat 200 sats for on-chain outputs, waive for off-chain
pkg/ark-lib/intent/testdata/message_fixtures.json (1)
40-50: LGTM!The new test fixture for the estimate-fee message type is well-structured and follows the existing pattern. The fields mirror those of the register message, which is appropriate since fee estimation requires the same information.
pkg/ark-lib/intent/message_test.go (1)
74-78: LGTM!The new case for
IntentMessageTypeEstimateFeefollows the established pattern and correctly handles the decoding of estimate fee messages.pkg/ark-lib/arkfee/celenv/intent_output.go (1)
1-22: LGTM!The CEL environment setup for intent output evaluation is correctly structured and follows best practices. The panic in
init()is appropriate as it catches configuration errors early during startup.internal/core/application/types.go (1)
22-31: EstimateFee API shape is consistent with existing Service methodsThe new
EstimateFee(ctx, proof, message) (int64, errors.Error)mirrors the existing intent APIs and cleanly exposes fee estimation without side effects. No issues from an interface/usage standpoint.pkg/ark-lib/arkfee/celenv/intent_input.go (1)
1-25: IntentInputEnv initialization matches expected CEL env patternGlobal
IntentInputEnvsetup with input variables and helper functions ininit()is straightforward and consistent with how a shared CEL environment should be configured. Panicking on misconfiguration at startup is reasonable here.internal/interface/grpc/handlers/arkservice.go (1)
104-118: EstimateFee handler wiring is consistent and minimalThe handler cleanly validates the request via
parseEstimateFeeIntent, maps parse errors toInvalidArgument, and delegates fee computation tosvc.EstimateFee, returning the rawfeein the response. This matches the existing patterns for other intent-based RPCs.pkg/ark-lib/arkfee/estimator_test.go (1)
11-266: Good coverage of estimator behaviors across input, output, and aggregate flowsThe fixture-based tests for
EvalInput,EvalOutput, andEvalexercise the key CEL programs (expiry-based, type-based, weight-based, and fixed-fee) and verify both zero-fee and non-zero-fee scenarios. The structure makes it easy to extend with more cases later.pkg/ark-cli/go.mod (1)
9-15: All dependency updates verified—go mod tidy and CLI build are cleanThe bumped
ark-lib/go-sdkversions and new CEL-related/genproto/protobuf dependencies are correct and well-resolved:
go mod tidycompletes without changes- CLI builds successfully against all updated versions
- Module graph is consistent and all transitive dependencies resolve cleanly
The changes are solid as-is.
internal/test/e2e/e2e_test.go (2)
82-82: LGTM - Consistent API signature updates.The
Balance()calls are updated throughout to match the new simplified API signature. The removal of the boolean parameter aligns with the SDK changes mentioned in the AI summary.Also applies to: 90-90, 135-135, 140-140, 179-179, 186-186
379-379: LGTM - CollaborativeExit API updates.The
CollaborativeExit()calls are consistently updated to the new 3-parameter signature, removing the on-chain-inputs boolean control. These changes align with the broader API surface updates in this PR.Also applies to: 424-424, 463-463, 1901-1901
pkg/ark-lib/go.mod (1)
18-19: LGTM - CEL dependencies added for arkfee package.The new dependencies support the CEL-based fee estimation system:
cel.dev/exprandgithub.com/google/cel-gofor CEL expression evaluationantlr4-go/antlras an indirect dependency for CEL parsingstoewer/go-strcasefor string case conversion in CEL- Updated protobuf/genproto dependencies required by CEL
Also applies to: 28-28, 33-36, 42-42
pkg/ark-lib/arkfee/celenv/functions.go (2)
12-21: LGTM -now()function implementation.Clean implementation returning Unix timestamp in seconds as a double, suitable for time-based fee calculations in CEL expressions.
24-53: LGTM -clamp_doubleandclamp_double_int_intoverloads.The implementations correctly handle type conversion and clamping logic. Error handling for conversion failures is appropriate.
Also applies to: 83-116
pkg/ark-lib/arkfee/estimator.go (3)
16-42: LGTM - Estimator construction and program parsing.The constructor correctly handles empty programs gracefully and validates CEL expressions at initialization time rather than at evaluation time, which is the right approach for fail-fast behavior.
44-60: LGTM - EvalInput implementation.The nil-program guard clause returning zero fee is appropriate for optional fee programs. Type conversion from CEL result to float64 is correct.
115-126: LGTM - CEL program parsing with type validation.Enforcing
cel.DoubleTypeas the return type at parse time ensures consistent fee values across all programs.internal/config/config.go (5)
127-130: LGTM - New fee configuration fields.The addition of
IntentInputFeeProgram,IntentOutputFeeProgram, and the privatefeeestimator field follows the existing config patterns. The estimator is appropriately kept private as it's an implementation detail.
213-214: LGTM - Environment variable configuration for fee programs.The new
ARKD_INTENT_INPUT_FEE_PROGRAMandARKD_INTENT_OUTPUT_FEE_PROGRAMenvironment variables follow the existing naming conventions. Default empty strings allow the system to run without fees configured.Also applies to: 251-252, 292-293, 404-405
648-654: LGTM - Fee estimator initialization.The
feeEstimator()method correctly creates the estimator from config values and wraps errors with context. This follows the same pattern as other service initialization methods in this file.
571-573: LGTM - Fee estimator validation placement.Initializing the fee estimator early in the validation chain is correct - it ensures invalid CEL programs are caught at startup rather than at runtime.
841-841: LGTM - Fee estimator passed to application service.The estimator is correctly wired into the application layer, replacing the previous
OnchainOutputFeeapproach mentioned in the summary.pkg/ark-lib/arkfee/celenv/variables.go (1)
1-23: LGTM!The CEL variable definitions are well-structured with:
- Clear constant names that match the variable identifiers
- Appropriate type choices (DoubleType for numeric values, StringType for enum-like types)
- Helpful documentation strings for each variable
internal/core/application/service.go (4)
73-73: LGTM!The integration of the
feeEstimatorfield into the service struct and constructor is clean and follows the existing pattern for dependency injection.Also applies to: 119-119, 242-242
2060-2063: LGTM!The
GetInfomethod now correctly exposes the CEL fee programs instead of a static fee value, enabling clients to understand the fee model.
2290-2392: LGTM! EstimateFee method correctly provides fee estimates.The
EstimateFeemethod appropriately:
- Validates time ranges
- Looks up vtxos to determine input types (boarding vs vtxo/recoverable/note)
- Constructs arkFeeInputs and arkFeeOutputs
- Evaluates fees using the estimator
The method intentionally skips signature verification and detailed validation since it's providing an estimate, not registering an actual intent. The logic correctly mirrors the fee calculation in
RegisterIntent.
1869-1895: ThecomputeIntentFeeshelper function is properly implemented.The function is defined in
internal/core/application/utils.go(line 456) and correctly extracts fees from the intent proof by calculatingsumOfInputs - sumOfOutputs. It validates that each input has a witness UTXO and returns an error if the sum of inputs is less than the sum of outputs, which prevents negative fees.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/ark-lib/arkfee/README.md (3)
63-96: Document error handling and edge cases in the Usage section.The usage examples show the happy path but omit error handling guidance. Please document:
- What happens when a CEL program is malformed or invalid?
- How should callers handle
errreturns fromEvalInput,EvalOutput, andEval?- Are there any constraints on program complexity, variable values, or maximum amounts?
131-131: Add details aboutFeeAmounttype andToSatoshis()conversion.The return type section mentions converting
FeeAmountto satoshis usingToSatoshis(), but doesn't document:
- The structure or fields of the
FeeAmounttype- Whether
ToSatoshis()performs rounding, truncation, or other transformations- Whether the result is an integer or floating-point value
24-25: Clarify conditional availability ofexpiryandbirthvariables.These variables are marked as "only available if input has an expiry/birth time." Please clarify:
- What value or error is returned if a program references these variables when they're absent?
- Should programs defensively check for their presence before use?
- Are there common patterns or best practices for handling optional variables in CEL?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/ark-lib/arkfee/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
pkg/ark-lib/arkfee/README.md
[grammar] ~124-~124: Ensure spelling is correct
Context: ...? 0.0 : 200.0 **Percentage fee for onchain outputs:**cel outputType == 'onchai...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit tests
- GitHub Check: integration tests
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/test/e2e/e2e_test.go (1)
758-779:refillusesfor range int(delta), which is invalid and doesn’t do what you want
for range int(delta)cannot range over anintand will not compile; even if it did, it wouldn’t clearly express “call faucet N times”. Givendeltais a positive float64 difference in BTC units, you likely want to loopint(delta)times and callnigiri fauceton each iteration.Apply something like this to make the intent explicit and compilable:
- for range int(delta) { - if _, err := runCommand("nigiri", "faucet", address.Address); err != nil { - return err - } - } + for i := 0; i < int(delta); i++ { + if _, err := runCommand("nigiri", "faucet", address.Address); err != nil { + return err + } + }If
deltashould instead represent BTC amount per faucet call rather than “count”, consider renaming or adjusting the math accordingly, but the currentfor range int(delta)is definitely wrong.
🧹 Nitpick comments (3)
internal/test/e2e/utils_test.go (2)
137-198: Consider de-duplicatingrunCommandWithEnvwithrunCommandThe logic in
runCommandWithEnvis almost identical torunCommand, differing only in how the environment is set onexec.Cmd. This works, but it duplicates a fairly involved pattern (pipes, goroutines, error aggregation).You could reduce maintenance surface by extracting a common helper (e.g.
runPreparedCommand(cmd *exec.Cmd)) and have bothrunCommandandrunCommandWithEnvcall it, or by adding an optional env overlay parameter torunCommanditself.
576-612:restartArkdWithNewConfigmay be flaky and leaves arkd in modified stateThe helper does what the new fee tests need, but two things are worth tightening:
Readiness waiting: it relies on fixed
time.Sleepcalls afterdocker container stopanddocker compose up, then immediately POSTs/wallet/unlock. On slower machines or under load this can easily race. Reusing the existingwaitUntilReadylogic (or polling/status) after unlock would make this much more robust.Global config side effect: it permanently restarts
arkdwith the provided fee programs and never restores the previous configuration, so any tests running after the caller will see the modified fee model. That’s OK as long asTestFeeis last and future tests are aware, but it’s fragile. Consider either:
- accepting a “restore” env and calling
restartArkdWithNewConfigagain in adeferat the call site, or- providing a symmetric helper that restarts with default (empty) fee programs.
internal/test/e2e/e2e_test.go (1)
3821-3982:TestFeeexercise is solid, but consider config restoration and broader coverageThe test does a good job of:
- Restarting
arkdwith a specific input-fee program.- Verifying that two consecutive batch settlements apply a 1% fee each time (
21000 → 20790 → 20582).- Asserting that offchain balances and onchain states move as expected.
A few improvements to consider:
- Restore arkd config after the test: As written,
restartArkdWithNewConfigleaves the CEL fee programs enabled for any tests that might run later. To keep the e2e environment deterministic, it would be safer to restore defaults in adefer, for example:func TestFee(t *testing.T) { - env := arkdEnv{ + env := arkdEnv{ intentInputFeeProgram: "inputType == 'note' || inputType == 'recoverable' ? 0.0 : amount*0.01", intentOutputFeeProgram: "outputType == 'onchain' ? 200.0 : 0.0", } - err := restartArkdWithNewConfig(env) + err := restartArkdWithNewConfig(env) require.NoError(t, err) + + // Optionally restore defaults (empty programs) after this test. + t.Cleanup(func() { + _ = restartArkdWithNewConfig(arkdEnv{}) + })
- Test the “free for note/recoverable” and output-fee branches: Right now you only cover the percentage fee on boarding/refresh inputs. Adding a small subtest that:
- onboards via
noteand checks that no input fee is charged, and/or- performs a collaborative exit and checks that the configured 200-sat onchain output fee is applied,
would give full coverage of the program shown in the PR description.These are not blockers for the current PR but would harden the tests against future changes to the fee-evaluation logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
docker-compose.regtest.yml(1 hunks)go.mod(7 hunks)internal/test/e2e/e2e_test.go(19 hunks)internal/test/e2e/utils_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-08T11:50:25.008Z
Learnt from: altafan
Repo: arkade-os/arkd PR: 733
File: cmd/arkd/commands.go:601-606
Timestamp: 2025-10-08T11:50:25.008Z
Learning: In the market hour configuration (cmd/arkd/commands.go and related files), when roundMinParticipantsCount or roundMaxParticipantsCount is set to 0, the system uses default values from environment variables ARKD_ROUN_MIN/MAX_PARTICIPANTS_COUNT. Therefore, it's acceptable that these values cannot be explicitly cleared back to 0 after being set to a non-zero value.
Applied to files:
docker-compose.regtest.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration tests
- GitHub Check: unit tests
- GitHub Check: Build and Scan
🔇 Additional comments (2)
docker-compose.regtest.yml (1)
95-96: New fee-program env vars are wired correctly into arkd service
ARKD_INTENT_OUTPUT_FEE_PROGRAMandARKD_INTENT_INPUT_FEE_PROGRAMare exposed as docker-compose envs with sane empty defaults and match the names used by the e2e restart helper. This is consistent with the CEL fee-model design and keeps existingARKD_ONCHAIN_OUTPUT_FEEbehavior intact.internal/test/e2e/e2e_test.go (1)
82-89: Balance/CollaborativeExit call-site updates are consistent with the new SDK APIAll updated usages of
Balance(...)andCollaborativeExit(...)across the e2e suite consistently follow the new signatures and preserve the original intent of the assertions (checking offchain/onchain split and exit amounts). No legacy callers with the old boolean flag signatures remain in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/ark-cli/go.mod (1)
17-17: Update CEL dependencies to latest stable versions (duplicate feedback).This PR continues to use cel.dev/expr v0.24.0 and antlr4-go v4.13.0, which were flagged in the past review as outdated. Please update to:
- cel.dev/expr v0.24.0 → v0.26.1 (latest stable as of Dec 2025)
- antlr4-go v4.13.0 → v4.13.1 (latest stable)
Apply updates across all go.mod files (both pkg/ark-cli/go.mod and root go.mod).
Also applies to: 21-21
go.mod (1)
72-72: Update CEL dependencies to latest stable versions (duplicate feedback).The root go.mod continues to use outdated CEL dependencies that were flagged in the previous review. Please update:
- cel.dev/expr v0.24.0 → v0.26.1 (latest stable as of Dec 2025)
- antlr4-go v4.13.0 → v4.13.1 (latest stable)
Since this is the root module, update it first, then ensure pkg/ark-cli/go.mod reflects the same versions.
Also applies to: 81-81
pkg/ark-lib/arkfee/README.md (1)
21-28: Clarify theweightvariable semantics (range and effect).Current text “Weighted liquidity lockup ratio of a vtxo” is still ambiguous. It would help to specify:
- Expected range (e.g. 0.0–1.0 or arbitrary >=0 scalar),
- Whether it’s unitless and how it typically scales fees (e.g. multiplier vs. share in a weighted average),
- A short numeric example showing its impact in a sample program.
This will make it easier for operators to design correct fee formulas.
🧹 Nitpick comments (3)
internal/core/application/service.go (2)
1485-1700: RegisterIntent fee evaluation flow is sound; consider a few robustness tweaks.Positives:
- Input classification into
boardingvs. (vtxo / recoverable / note) and output classification intovtxovs.onchainmap cleanly onto the documented CELinputType/outputTypevariants.- Using
FeeAmount.ToSatoshis()and enforcingfees < expectedFeesSatsbefore accepting an intent provides a clear, conservative threshold (rounding up expected fees).A few refinements to consider:
- Input type precedence for notes vs. recoverables
inputType := arkfee.InputTypeVtxo if vtxo.Swept { inputType = arkfee.InputTypeRecoverable } else if vtxo.IsNote() { inputType = arkfee.InputTypeNote }This assumes
SweptandIsNote()are mutually exclusive. If a note can also be swept, it will be classified asrecoverableinstead ofnote. If that’s not intended, invert the order or use an explicitswitchto make precedence clear.
- Non‑negative value assumptions when casting to
uint64Both for boarding inputs and outputs you do:
Amount: uint64(psbtInput.WitnessUtxo.Value), ... Amount: amount // where amount := uint64(output.Value)If a malformed PSBT ever carries a negative
Value, this will silently wrap to a hugeuint64. You already reject malformed proofs elsewhere, but adding explicit guards here (e.g.if Value < 0 return INVALID_PSBT_INPUT) would make the fee path more self‑contained and safer.
- Weight is always zero for now
You currently append:
Weight: 0, // TODO compute weightfor both boarding and vtxo inputs. That’s fine as a first step, but worth documenting explicitly (in code or README) that
weightis not yet meaningful so operators don’t rely on it in programs prematurely.
- Optional: expose per‑input/output breakdown in metadata
INTENT_INSUFFICIENT_FEE’s metadata type hasInputExpectedFees/OutputExpectedFeesmaps in addition to totals. If you later extendEstimatorto return per‑element fees, populating those maps here would make debugging under‑funded intents easier.Overall, the integration is correct; these points are mainly about edge‑case clarity and future‑proofing.
Also applies to: 1745-1845, 1869-1895
2290-2392: EstimateFee mirrors RegisterIntent well; consider DRYing and tightening validation.The new
EstimateFeemethod correctly:
- Reuses the same
ValidAt/ExpireAtchecks asRegisterIntent.- Builds
arkfee.Input/arkfee.Outputwith the same boarding vs vtxo vs recoverable vs note and onchain vs offchain classifications.- Calls
s.feeEstimator.Evaland returns the ceiling in satoshis.A few suggestions:
- Share input/output construction with RegisterIntent
The input/output →
arkfee.Input/arkfee.Outputlogic is now duplicated betweenRegisterIntentandEstimateFee. Extracting helpers (e.g.buildArkFeeInputs/buildArkFeeOutputs) would reduce drift risk when adding new input/output types or fields.
- Same negative‑value guard as registration path
Here too you cast
output.ValueandWitnessUtxo.Valuetouint64. Adding simpleValue < 0checks before the cast (returning anINVALID_INTENT_PSBT/INVALID_PSBT_INPUTerror) would protect the estimator in case a client supplies a malformed PSBT.
- Clarify behavior for unknown inputs
When
GetVtxosreturns no rows, you treat the input asboardingand still charge viaInputTypeBoarding. That’s reasonable for estimation, but a brief comment would help future readers understand that this is “best effort” estimation without requiring the vtxo to exist yet.Functionally, the method is consistent with the registration flow.
internal/config/config.go (1)
26-27: Config wiring for CEL fee programs is coherent.
- New
IntentInputFeeProgram/IntentOutputFeeProgramfields, env keys, and defaults are consistent, and with theARKD_prefix viper will pick upARKD_INTENT_INPUT_FEE_PROGRAM/ARKD_INTENT_OUTPUT_FEE_PROGRAMas intended.LoadConfigcorrectly plumbs these into theConfiginstance, so they are available before service initialization.One small robustness improvement: you may want to
strings.TrimSpacethe program strings before passing them downstream, to avoid surprising parse errors from accidental leading/trailing whitespace in env vars.Also applies to: 118-132, 159-217, 219-255, 294-296, 344-409
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpkg/ark-cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.mod(7 hunks)internal/config/config.go(9 hunks)internal/core/application/service.go(13 hunks)pkg/ark-cli/go.mod(4 hunks)pkg/ark-lib/arkfee/README.md(1 hunks)pkg/ark-lib/arkfee/celenv/functions.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_input.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_output.go(1 hunks)pkg/ark-lib/arkfee/types.go(1 hunks)pkg/ark-lib/go.mod(2 hunks)pkg/errors/go.mod(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/errors/go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/ark-lib/arkfee/celenv/intent_input.go
- pkg/ark-lib/go.mod
- pkg/ark-lib/arkfee/celenv/functions.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/core/application/service.go
🧬 Code graph analysis (3)
pkg/ark-lib/arkfee/types.go (1)
pkg/ark-lib/arkfee/celenv/variables.go (6)
AmountVariableName(8-8)InputTypeVariableName(12-12)WeightVariableName(11-11)ExpiryVariableName(9-9)BirthVariableName(10-10)OutputTypeVariableName(13-13)
internal/core/application/service.go (10)
pkg/ark-lib/arkfee/estimator.go (2)
Estimator(16-19)New(22-42)pkg/ark-lib/arkfee/types.go (7)
Input(25-31)InputTypeVtxo(20-20)InputTypeRecoverable(19-19)InputTypeNote(22-22)Output(55-58)OutputTypeVtxo(51-51)OutputTypeOnchain(52-52)internal/core/ports/tx_builder.go (1)
Input(25-28)pkg/ark-lib/intent/proof.go (3)
Input(44-48)New(134-158)Proof(39-41)api-spec/protobuf/gen/ark/v1/types.pb.go (6)
Input(76-82)Input(95-95)Input(110-112)Outpoint(24-30)Outpoint(43-43)Outpoint(58-60)internal/core/application/admin.go (1)
Receiver(519-523)pkg/errors/errors.go (7)
INVALID_INTENT_PROOF(292-296)InvalidIntentProofMetadata(173-176)INTENT_INSUFFICIENT_FEE(325-329)IntentInsufficientFeeMetadata(195-200)INVALID_INTENT_TIMERANGE(279-283)IntentTimeRangeMetadata(163-167)PsbtMetadata(111-113)pkg/ark-lib/intent/message.go (1)
EstimateFeeMessage(62-62)internal/core/application/types.go (2)
Outpoint(180-180)VOut(134-134)internal/core/domain/vtxo.go (1)
Outpoint(15-18)
internal/config/config.go (1)
pkg/ark-lib/arkfee/estimator.go (2)
Estimator(16-19)New(22-42)
🪛 LanguageTool
pkg/ark-lib/arkfee/README.md
[duplication] ~8-~8: Possible typo: you repeated a word.
Context: ...nput Program**: Evaluated for each input in an intent - Intent Output Program: Evaluated ...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: Build and Scan
- GitHub Check: integration tests
🔇 Additional comments (5)
go.mod (1)
221-222: Verify protobuf and genproto version alignment.The protobuf and genproto version updates use recent pseudo-versions (2025-12-02). Confirm that these versions are compatible with the CEL dependencies and that no API breakage occurs during compilation and testing.
Also applies to: 252-252, 272-272
pkg/ark-lib/arkfee/celenv/intent_output.go (1)
1-21: IntentOutputEnv wiring looks consistent with input env.Env construction with
amount,outputType, andnowFunctionmatches the expected CEL surface and failing fast via panic on init is appropriate here.pkg/ark-lib/arkfee/types.go (1)
10-65: FeeAmount and CEL arg marshalling look correct.
FeeAmount.ToSatoshis()usingmath.Ceilis appropriate to avoid under‑charging when programs return fractional fees.Input/OutputtoArgs correctly exposeamount,inputType/outputType,weight, and optional timestamps using the shared celenv variable names.No behavioral issues spotted here.
internal/core/application/service.go (1)
2431-2447: New cache cleanup in startRound looks correct.The added cleanup:
- Deletes tree signing sessions for the previous round ID.
- Deletes cached boarding input signatures keyed by the previous
CommitmentTxid.This is gated on
existingRound != niland empty‑string checks, so it’s safe and should prevent stale data from leaking into subsequent rounds.internal/config/config.go (1)
574-576: feeEstimator lifecycle and error surfacing look good; align with service‑side nil handling.
Validate()now callsc.feeEstimator()early, so invalid CEL programs fail fast with a clear “failed to create fee estimator” error before any DB/wallet wiring.feeEstimator()simply delegates toarkfee.New(c.IntentInputFeeProgram, c.IntentOutputFeeProgram)and stores the result inc.fee, which is then injected intoapplication.NewServiceinappService().Once you enforce a non‑nil estimator in
NewService(as suggested inservice.go), this configuration path guaranteesc.feeis always initialized for normal node startup, and misconfigured fee programs are rejected at config‑validation time.No functional issues here; just ensure any direct uses of
application.NewServiceoutside the config path are updated to pass a valid estimator as well.Also applies to: 651-657, 831-845
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/ark-lib/arkfee/README.md (1)
29-29: Clarify theweightvariable documentation.The description "Weighted liquidity lockup ratio of a vtxo" remains vague. Per prior feedback, please specify the valid range (e.g., 0.0–1.0), explain how it modifies fee calculations, describe edge cases (e.g., weight=0 means no contribution, weight=1.0 means full contribution), and include a concrete example showing how a vtxo with weight 0.25 affects a fee computation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/ark-lib/arkfee/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
pkg/ark-lib/arkfee/README.md
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ...nput Program**: Evaluated for each input in an intent - Intent Output Program: Evaluated ...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.18.1)
pkg/ark-lib/arkfee/README.md
6-6: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/ark-lib/arkfee/estimator.go (1)
23-33: Consider safer type assertion.While the type assertion on line 32 is protected by the
DoubleTypecheck inparse(), it could still panic if there's an unexpected bug. Consider using a type assertion with ok check for defensive programming.Apply this diff for safer error handling:
func (p *program) Eval(args map[string]any) (FeeAmount, error) { result, _, err := p.Program.Eval(args) if err != nil { return 0, err } native, err := result.ConvertToNative(reflect.TypeOf(float64(0))) if err != nil { return 0, err } - return FeeAmount(native.(float64)), nil + f64, ok := native.(float64) + if !ok { + return 0, fmt.Errorf("unexpected type %T, expected float64", native) + } + return FeeAmount(f64), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/ark-lib/arkfee/celenv/intent_offchain_input.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_onchain_input.go(1 hunks)pkg/ark-lib/arkfee/celenv/intent_output.go(1 hunks)pkg/ark-lib/arkfee/celenv/variables.go(1 hunks)pkg/ark-lib/arkfee/estimator.go(1 hunks)pkg/ark-lib/arkfee/estimator_test.go(1 hunks)pkg/ark-lib/arkfee/testdata/invalid.json(1 hunks)pkg/ark-lib/arkfee/testdata/valid.json(1 hunks)pkg/ark-lib/arkfee/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ark-lib/arkfee/celenv/variables.go
- pkg/ark-lib/arkfee/types.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/ark-lib/arkfee/estimator_test.go (3)
pkg/ark-lib/arkfee/estimator.go (2)
Config(11-16)New(44-73)internal/config/config.go (1)
Config(61-145)pkg/ark-lib/arkfee/types.go (5)
OffchainInput(24-30)FeeAmount(10-10)OnchainInput(47-49)Output(57-60)VtxoType(16-16)
pkg/ark-lib/arkfee/estimator.go (5)
pkg/ark-lib/arkfee/types.go (4)
FeeAmount(10-10)OffchainInput(24-30)OnchainInput(47-49)Output(57-60)internal/config/config.go (1)
Config(61-145)pkg/ark-lib/arkfee/celenv/intent_offchain_input.go (1)
IntentOffchainInputEnv(7-7)pkg/ark-lib/arkfee/celenv/intent_onchain_input.go (1)
IntentOnchainInputEnv(7-7)pkg/ark-lib/arkfee/celenv/intent_output.go (1)
IntentOutputEnv(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: integration tests
- GitHub Check: unit tests
🔇 Additional comments (11)
pkg/ark-lib/arkfee/celenv/intent_onchain_input.go (1)
9-20: LGTM!The initialization pattern is appropriate. Panicking in
init()for critical environment setup failures is acceptable since the application cannot function without valid CEL environments.pkg/ark-lib/arkfee/celenv/intent_output.go (1)
9-20: LGTM!Consistent initialization pattern with appropriate variables for output evaluation (amount, outputScript, now function).
pkg/ark-lib/arkfee/celenv/intent_offchain_input.go (1)
9-23: LGTM!The offchain input environment appropriately includes temporal fields (expiry, birth) and input metadata (type, weight) needed for time-based and type-conditional fee calculations.
pkg/ark-lib/arkfee/testdata/invalid.json (1)
1-112: LGTM!Comprehensive negative test coverage including syntax errors, type mismatches, undefined variables, and cross-environment variable usage. This will help ensure robust error handling during CEL program compilation.
pkg/ark-lib/arkfee/estimator_test.go (4)
13-30: LGTM!Comprehensive testing of invalid configurations with proper error assertions.
32-165: LGTM!Thorough test coverage for individual evaluation methods, including edge cases where no program is configured.
167-210: LGTM!Well-structured end-to-end evaluation tests exercising all four program types together.
212-345: LGTM!Clean test data structures and conversion utilities using Go's embed feature for test fixtures.
pkg/ark-lib/arkfee/estimator.go (3)
43-73: LGTM!The constructor properly handles optional programs by only parsing non-empty configuration strings, with appropriate error propagation.
75-109: LGTM!Individual evaluation methods correctly handle the case where programs are not configured (returning zero fees) and properly delegate to the program wrapper.
145-160: LGTM!The
parsehelper properly validates that CEL programs returndoubletype, which provides type safety for the fee calculations and protects the type assertion inprogram.Eval.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/ark-lib/arkfee/README.md (1)
6-6: Fix bare URL in markdown.Use proper markdown link syntax instead of a bare URL.
-https://github.com/google/cel-spec/blob/master/doc/langdef.md#list-of-standard-definitions +[CEL language specification](https://github.com/google/cel-spec/blob/master/doc/langdef.md#list-of-standard-definitions)internal/core/application/service.go (1)
73-75: Still need a guard against nil feeEstimator in service construction.
servicenow dereferencess.feeEstimatorin multiple places (RegisterIntent,EstimateFee,GetInfo) assuming it is non‑nil, butNewServicestill acceptsfeeEstimator *arkfee.Estimatorwithout enforcing this invariant. Tests or alternative wiring that bypassconfig.Validate()can still passniland panic at runtime.Recommend enforcing non‑nil in
NewServiceand optionally asserting inside call sites:func NewService( @@ - feeEstimator *arkfee.Estimator, + feeEstimator *arkfee.Estimator, ) (Service, error) { + if feeEstimator == nil { + return nil, fmt.Errorf("fee estimator must not be nil") + } @@ - vtxoNoCsvValidationCutoffTime: vtxoNoCsvValidationCutoffTime, - feeEstimator: feeEstimator, + vtxoNoCsvValidationCutoffTime: vtxoNoCsvValidationCutoffTime, + feeEstimator: feeEstimator,Also applies to: 119-120, 199-243
🧹 Nitpick comments (9)
pkg/ark-lib/arkfee/README.md (1)
31-31: Clarify theweightvariable documentation.The description "Weighted liquidity lockup ratio of a vtxo" lacks critical context. Expand this to specify the valid range (e.g., 0.0–1.0?), how it modifies the fee calculation, and its behavioral semantics (e.g., 0 = no lockup → no contribution, 1 = fully locked → full contribution). A short example showing how a vtxo with weight 0.25 affects a fee computation would be helpful.
internal/core/application/types.go (1)
95-105: Changing IntentFeeInfo on‑chain fields to string is consistent with exposing CEL programs.Switching
OnchainInput/OnchainOutputtostringmatches how these values are later populated fromarkfee.Estimator.Configand serialized via gRPC. Just ensure all callers expect a program string rather than a numeric fee value.If any downstream consumers still treat these fields as numeric amounts, they should be updated or renamed to avoid confusion (e.g.,
OnchainInputProgram).internal/core/application/service.go (3)
1440-1714: Fee accounting in RegisterIntent is consistent but could use minor robustness.Positives:
- You accumulate
expectedFeesfrom:
- On‑chain boarding inputs via
EvalOnchainInput.- Off‑chain inputs (existing vtxos) via
EvalOffchainInputwithVtxoType{Vtxo,Recoverable,Note}.- Outputs via
EvalOnchainOutput/EvalOffchainOutput.- You then compute actual proof fees via
proof.Fees()and enforcefees >= expectedFees.ToSatoshis(), returning a structuredINTENT_INSUFFICIENT_FEEon failure.- Missing
WitnessUtxoon any proof input will be caught early byProof.Fees().Two minor suggestions:
- Guard against nil feeEstimator just in case. Even with constructor checks, an optional defensive:
if s.feeEstimator == nil { return "", errors.INTERNAL_ERROR.New("fee estimator unavailable") }near the top of
RegisterIntentwould turn a panic into a clear error.
- Consider logging or exposing per‑component fee breakdown. Right now only the total is enforced; if you later need debugging, returning or logging the per‑category fees (on/off‑chain, in/out) can be very helpful.
Also applies to: 1898-1913
1768-1871: Output fee evaluation matches message semantics and existing amount checks.
- You build a reusable
arkfee.Outputfrom eachTxOutand branch onslices.Contains(message.OnchainOutputIndexes, outputIndex)to choose betweenEvalOnchainOutputandEvalOffchainOutput.- Existing amount and script validations (UTXO/VTXO min/max, address extraction, sub‑dust checks) remain intact and occur before fee accumulation, so invalid outputs are rejected before fees matter.
- Adding
outputto error metadata makes mis‑configured CEL programs diagnosable.This is a clean integration of the estimator into the existing registration flow.
If you find yourself needing this split in multiple places, consider a small helper that returns
(arkfee.Output, isOnchain bool)to DRY up the logic betweenRegisterIntentandEstimateFee.
2310-2422: EstimateFee implementation is consistent with RegisterIntent and side‑effect free.
- Time‑range checks (
ValidAt/ExpireAt) mirrorRegisterIntent, so callers see the same validity semantics.- Inputs:
- For each proof input, you look up a vtxo; if found, you build an
OffchainInputwith typeVtxo/Recoverable/Note.- If no vtxo is found or there’s an error, you fall back to an
OnchainInputusingWitnessUtxo.Value.- Outputs:
- You classify outputs as on‑chain vs off‑chain with the same
OnchainOutputIndexesset used in registration.- You then call
s.feeEstimator.Eval(...)with the four slices and return the total in satoshis.This matches the registration‑time accounting but without DB/cache mutations and looks safe as an estimate‑only path.
You might eventually want to reuse the input/output collection logic between
RegisterIntentandEstimateFee(e.g. helper that returns the four arkfee slices plus the raw receivers), to avoid subtle drift if one path is updated in the future.internal/config/config.go (2)
442-618: feeEstimator initialization in Validate surfaces CEL errors early.
Validatenow callsc.feeEstimator()before constructing other services, so malformed CEL programs cause startup to fail fast with a clear"failed to create fee estimator"error.feeEstimator()builds the estimator directly fromc.Intent*Programfields, relying onarkfee.Newto only parse non‑empty programs, which matches the intent of “optional fee rules”.One minor suggestion: log the offending program(s) at debug level when
arkfee.Newfails, to ease diagnosis in production.You can verify behavior with a small CEL typo in one of the
INTENT_*_FEE_PROGRAMenv vars and ensureValidatereturns a clear error mentioning the estimator creation.Also applies to: 661-672
820-861: Passing c.fee into NewService is correct but assumes prior initialization.
appService()forwardsc.feetoapplication.NewService, which matches the new constructor signature. Given thatValidate()already callsc.feeEstimator(), this is fine for normal startup.As a defensive improvement, consider:
- Returning an explicit error if
c.feeis stillnilhere (e.g., ifappService()is called withoutValidate()in some tests).internal/test/e2e/e2e_test.go (2)
3909-3911: Clarify fee calculation with inline comments.The hardcoded expected values (20790, 20582) are correct but would benefit from inline calculation comments to make the test more maintainable and self-documenting.
Apply this diff to add clarity:
// 21000 - 1% of 21000 = 20790 + // Formula: 21000 * 0.99 = 20790 require.Equal(t, 20790, int(aliceFirstVtxo.Amount)) require.Equal(t, 20790, int(bobFirstVtxo.Amount)) // ... // 20790 - 1% of 20790 = 20582 + // Formula: 20790 * 0.99 = 20582.1, truncated to 20582 require.Equal(t, 20582, int(aliceSecondVtxo.Amount)) require.Equal(t, 20582, int(bobSecondVtxo.Amount))Note: The second calculation shows truncation (20790 * 0.99 = 20582.1 → 20582), which is expected behavior for satoshi amounts but worth documenting.
Also applies to: 3963-3965
3824-3831: Document the fee program semantics for test maintainability.The CEL fee formulas are central to the test but their semantics could be clearer for future maintainers.
Consider adding a comment block explaining the fee structure:
+ // Fee structure being tested: + // - Offchain inputs: Free for 'note' or 'recoverable' types, otherwise 1% of amount + // - Onchain inputs: 1% of amount + // - Offchain outputs: Free + // - Onchain outputs: Flat 200 satoshis env := arkdEnv{ - // for input: free in case of recoverable or note, 1% of the amount otherwise - // for output: 200 satoshis for onchain output, 0 for vtxo output intentOffchainInputFeeProgram: "inputType == 'note' || inputType == 'recoverable' ? 0.0 : amount*0.01", intentOnchainInputFeeProgram: "0.01 * amount", intentOffchainOutputFeeProgram: "0.0", intentOnchainOutputFeeProgram: "200.0", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpkg/ark-cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
docker-compose.regtest.yml(1 hunks)go.mod(7 hunks)internal/config/config.go(9 hunks)internal/core/application/service.go(14 hunks)internal/core/application/types.go(2 hunks)internal/core/application/utils.go(0 hunks)internal/interface/grpc/handlers/parser.go(2 hunks)internal/test/e2e/e2e_test.go(19 hunks)internal/test/e2e/utils_test.go(3 hunks)pkg/ark-cli/go.mod(4 hunks)pkg/ark-lib/arkfee/README.md(1 hunks)pkg/ark-lib/intent/proof.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/core/application/utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/test/e2e/utils_test.go
- go.mod
- docker-compose.regtest.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/core/application/service.go
🧬 Code graph analysis (2)
internal/interface/grpc/handlers/parser.go (3)
api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)pkg/ark-lib/intent/message.go (1)
EstimateFeeMessage(62-62)pkg/ark-lib/arkfee/types.go (1)
OnchainInput(47-49)
internal/config/config.go (1)
pkg/ark-lib/arkfee/estimator.go (3)
Estimator(35-41)Config(11-16)New(44-73)
🪛 LanguageTool
pkg/ark-lib/arkfee/README.md
[grammar] ~24-~24: Ensure spelling is correct
Context: ...ntOffchainInputEnv Used for evaluating offchain input (vtxo) fee calculations. Availabl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~36-~36: Ensure spelling is correct
Context: ...entOnchainInputEnv Used for evaluating onchain input (boarding) fee calculations. Avai...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
pkg/ark-lib/arkfee/README.md
6-6: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: integration tests
- GitHub Check: Build and Scan
🔇 Additional comments (14)
pkg/ark-lib/arkfee/README.md (1)
64-117: LGTM!The documentation is comprehensive, well-structured, and provides clear usage patterns with concrete examples across all four program categories. The CEL environment documentation, example programs, and code snippets make it easy for users to understand and implement custom fee logic.
pkg/ark-cli/go.mod (3)
10-11: Verify updated ark-lib and go-sdk versions contain expected CEL implementation.The direct dependencies have been updated to newer pseudo-versions. Please confirm that these versions include the fee estimation logic and CEL parsing/evaluation changes that should accompany the new CEL-based fee system.
92-95: Verify proto-related dependency versions are aligned.The protobuf-related indirect dependencies have been updated to newer versions. Confirm these versions are compatible with Go 1.25.5 and properly aligned with the gRPC API changes introduced by the fee system (EstimateFee RPC).
17-17: CEL dependencies are secure with no known vulnerabilities, though two can be updated to newer versions.All four CEL-related dependencies are stable and free of public security advisories:
cel.dev/expr v0.24.0— secure, no CVEsgoogle/cel-go v0.26.1— latest released version, secureantlr4-go/antlr/v4 v4.13.0— secure; newer patch v4.13.2 available (non-critical)stoewer/go-strcase v1.2.0— secure; newer v1.3.x versions available (non-critical)The versions in use are safe to ship. Consider upgrading antlr and go-strcase to their latest patches in a future update for currency, but no immediate action is required.
internal/interface/grpc/handlers/parser.go (2)
51-67: parseEstimateFeeIntent correctly mirrors register-intent parsing.The new helper follows the same validation and decode pattern as
parseRegisterIntent, including error text and nil checks, which keeps parsing behavior consistent across intent types. Looks good.
291-303: FeeInfo toProto uses raw on‑chain fee programs consistently.Passing
OnchainInputandOnchainOutputthrough as strings (like the off‑chain fields) matches the newIntentFeeInfotypes and seems consistent with exposing CEL programs rather than numeric fees. No issues here.internal/core/application/types.go (1)
22-31: Service interface extension for EstimateFee looks coherent.The new
EstimateFeemethod matches the implementation signature inservice.go(context,intent.Proof,intent.EstimateFeeMessage, returnsint64, errors.Error), so interface and concrete type stay aligned.pkg/ark-lib/intent/proof.go (1)
160-179: Proof.Fees implementation is logically sound and defensive.
- Uses all PSBT inputs (including index 0) but safely, since the first input’s witness UTXO is set to value 0 in
buildToSignTx.- Requires
WitnessUtxoon every input and reports which index is missing, which is helpful when proofs are malformed.- Sums unsigned‑tx outputs and rejects negative fees (
sumInputs < sumOutputs), preventing nonsensical results.This aligns with how proofs are constructed in this package and should give a reliable fee figure for
RegisterIntent.internal/core/application/service.go (2)
2062-2085: GetInfo now exposes CEL fee programs correctly.Populating
ServiceInfo.Fees.IntentFees.*froms.feeEstimator.Config.Intent*Programfields is coherent with the config/arkfee wiring and gives clients introspection into the active fee model. As long asfeeEstimatoris guaranteed non‑nil (see earlier comment), this is straightforward.
2461-2477: Round cleanup additions help prevent stale cache entries.The extra cleanup of tree signing sessions and boarding‑input signatures when restarting rounds:
- Deletes tree signing sessions keyed by
existingRound.Id.- Clears boarding input signatures keyed by
existingRound.CommitmentTxid.This should reduce the risk of old data leaking into new rounds. Behavior looks correct given the surrounding reset logic.
internal/config/config.go (2)
26-27: Config wire‑up for arkfee estimator looks structurally correct.
- New
Configfields (IntentOffchainInputProgram,IntentOnchainInputProgram, etc.) plusfee *arkfee.Estimatoralign witharkfee.Config.- Env variables
ARKD_INTENT_*_FEE_PROGRAMmap cleanly to those fields, and you’ve introduced matching constants.EnablePprofremains orthogonal and doesn’t interfere.This is a sensible way to surface CEL programs via environment.
Also applies to: 126-147, 217-221
223-261: Defaults and LoadConfig wiring preserve backward compatibility.
- Fee‑program defaults are empty strings, so in the absence of env overrides the estimator will compile with “no extra fees” rather than failing.
LoadConfigpulls the fee‑program env vars into the new Config fields while leaving existing behavior intact.EnablePprofcontinues to be read from its env var with a false default.No behavioral regressions apparent here.
Also applies to: 295-305, 352-419
internal/test/e2e/e2e_test.go (2)
82-82: LGTM! API signature updates are consistent.The
Balance()call signature changes throughout the test suite are consistent with the updated SDK API that removed the boolean parameter. All call sites properly updated.Also applies to: 90-90, 135-135, 140-140, 179-179, 186-186, 202-202, 229-229, 255-255, 269-269, 289-289, 313-313, 345-345, 363-363, 368-368, 385-385, 391-391, 407-407, 413-413, 429-429, 435-435, 2058-2058, 2702-2702
379-379: LGTM! API signature updates are consistent.The
CollaborativeExit()call signature changes are consistent with the updated SDK API that removed the boolean parameter.Also applies to: 424-424, 463-463, 1901-1901
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/ark-lib/arkfee/estimator.go (3)
23-32: Avoid potential panic onConvertToNativeresult
native.(float64)will panic if CEL ever returns a non‑float64despite the staticDoubleTypecheck. It’s low‑probability but easy to harden by checking the type assertion and including the program text in the error for easier debugging.Example refactor:
func (p *program) Eval(args map[string]any) (FeeAmount, error) { - result, _, err := p.Program.Eval(args) - if err != nil { - return 0, err - } - native, err := result.ConvertToNative(reflect.TypeOf(float64(0))) - if err != nil { - return 0, err - } - return FeeAmount(native.(float64)), nil + result, _, err := p.Program.Eval(args) + if err != nil { + return 0, fmt.Errorf("evaluate fee program %q: %w", p.txt, err) + } + native, err := result.ConvertToNative(reflect.TypeOf(float64(0))) + if err != nil { + return 0, fmt.Errorf("convert result of fee program %q to float64: %w", p.txt, err) + } + f, ok := native.(float64) + if !ok { + return 0, fmt.Errorf("fee program %q returned non-float64 result of type %T", p.txt, native) + } + return FeeAmount(f), nil }
43-73: Avoid returning a partially initializedEstimatoron errorBecause
Newuses named returns, a compile/parse failure returns a non‑nilEstimatoralongside a non‑nilerr. That’s fine if all call sites reliably ignore the estimator whenerr != nil, but it’s easy to make this safer/clearer by returningnilon failure and avoiding named returns.For example:
-func New(config Config) (estimator *Estimator, err error) { - estimator = &Estimator{Config: config} +func New(config Config) (*Estimator, error) { + estimator := &Estimator{Config: config} @@ - if err != nil { - return - } + if err != nil { + return nil, err + } @@ - if err != nil { - return - } + if err != nil { + return nil, err + } @@ - if err != nil { - return - } + if err != nil { + return nil, err + } @@ - if err != nil { - return - } + if err != nil { + return nil, err + } } - - return + return estimator, nil }
75-109: Fix minor typos in exported method commentsThe comments for the
Eval*methods have a small typo (“evalutes”) and are user‑facing on the exported API. Consider correcting them for polish, e.g.:-// EvalOffchainInput evalutes the fee for a given vtxo input +// EvalOffchainInput evaluates the fee for a given offchain (vtxo) input @@ -// EvalOnchainInput evalutes the fee for a given boarding input +// EvalOnchainInput evaluates the fee for a given onchain (boarding) input @@ -// EvalOffchainOutput evalutes the fee for a given vtxo output +// EvalOffchainOutput evaluates the fee for a given offchain (vtxo) output @@ -// EvalOnchainOutput evalutes the fee for a given collaborative exit output +// EvalOnchainOutput evaluates the fee for a given collaborative exit (onchain) output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/ark-lib/arkfee/estimator.go(1 hunks)pkg/ark-lib/arkfee/testdata/valid.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
pkg/ark-lib/arkfee/testdata/valid.json
🧬 Code graph analysis (1)
pkg/ark-lib/arkfee/estimator.go (4)
pkg/ark-lib/arkfee/types.go (4)
FeeAmount(10-10)OffchainInput(24-30)OnchainInput(47-49)Output(57-60)pkg/ark-lib/arkfee/celenv/intent_offchain_input.go (1)
IntentOffchainInputEnv(7-7)pkg/ark-lib/arkfee/celenv/intent_onchain_input.go (1)
IntentOnchainInputEnv(7-7)pkg/ark-lib/arkfee/celenv/intent_output.go (1)
IntentOutputEnv(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: integration tests
- GitHub Check: Build and Scan
🔇 Additional comments (6)
pkg/ark-lib/arkfee/estimator.go (2)
111-150: AggregateEvalnow correctly accounts for all four intent dimensionsThe aggregate
Evalmethod now iterates over offchain/onchain inputs and outputs, delegating to the per‑item evaluators and propagating errors. This fixes the previously noted omission of onchain outputs and aligns with the test expectations intestdata/valid.json(e.g., mixed and onchain‑only cases).Logic and error handling here look solid.
153-168: Type enforcement inparseis appropriateValidating
ast.OutputType() == cel.DoubleTypebefore constructing the program is a good guardrail and matches theFeeAmountsemantics. This should prevent misconfigured fee programs (e.g., boolean/string) from slipping into runtime evaluation.Implementation looks good as‑is.
pkg/ark-lib/arkfee/testdata/valid.json (4)
2-83: Offchain input fixtures cover time, type, and weight dimensions wellThe
evalOffchainInputgroup nicely exercises expiry‑based conditions, input type branching, and weighted percentage fees. The expected values (e.g., 5000 vs 0, 56.3, 100) match the CEL programs’ logic.
84-125: Onchain input fixtures validate both fixed and proportional feesThe
evalOnchainInputcases correctly validate constant andamount * 0.01programs, with expectations (100, 500) matching the formulas. This gives good coverage of typical onchain input fee models.
126-203: Output fixtures symmetrically cover offchain/onchain scenariosThe
evalOffchainOutputandevalOnchainOutputsections mirror each other with fixed, script‑size‑based, and percentage fees. The script length example ("0020deadbeefeeeeeeffff"→ 22 bytes → 0.22 fee) looks correct and ensuressize(script)handling is exercised.
204-281: Aggregateevalexpectations correctly include all fee componentsThe aggregate
evalcases now match the estimator’s full behavior:
- “fixed fee” mixed case: 100 (offchain in) + 50 (onchain in) + 100 (offchain out) + 150 (onchain out) = 400.
- “free for vtxo input”: only one charge from the non‑empty script output → 100.
- “onchain only”: 1% of 10000 = 100 plus 15% of 20000 = 3000 → 3100.
These expectations are consistent with the corrected estimator (including onchain outputs).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/config/config.go (1)
216-221: Fee program env var names vs PR/docs — ensure they’re aligned and fully documentedHere you expose four env keys:
INTENT_OFFCHAIN_INPUT_FEE_PROGRAMINTENT_ONCHAIN_INPUT_FEE_PROGRAMINTENT_OFFCHAIN_OUTPUT_FEE_PROGRAMINTENT_ONCHAIN_OUTPUT_FEE_PROGRAMwhich become
ARKD_...env vars via the prefix, plus the newENABLE_PPROFand the cutoff date key.The PR objectives / description mention only
ARKD_INTENT_INPUT_FEE_PROGRAMandARKD_INTENT_OUTPUT_FEE_PROGRAM. If the final design is to have four separate knobs (offchain/onchain × input/output), it would be good to:
- Update any README / config docs to list all four env vars explicitly and drop/clarify the older two‑var naming, and
- Maybe add a short comment here or in the README explaining the mapping (offchain vs onchain) so operators don’t have to infer it from code.
I can help draft the doc / README snippet if you’d like.
Also applies to: 295-297, 405-407
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/config/config.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: integration tests
- GitHub Check: unit tests
🔇 Additional comments (2)
internal/config/config.go (2)
126-127: EnablePprof config wiring looks consistentThe
EnablePproffield, env key, default, andLoadConfigmapping all follow the existing config patterns (env‑backed bool with a sensible default). Assuming the rest of the codebase actually consumesConfig.EnablePprof, this wiring looks correct and non‑controversial.Also applies to: 221-221, 256-256, 296-296, 406-406
26-27: Fee programs are optional and safely handled when unsetThe integration is actually more robust than the original concern suggested.
arkfee.Newexplicitly guards each program field withif len(config.IntentXxxProgram) > 0before parsing, so empty programs are gracefully skipped without error. This is documented in the arkfee code comment: "parses the intent input and output programs if not empty."Since there are no
viper.SetDefaultcalls for the fourINTENT_*_FEE_PROGRAMenv vars, unset programs default to empty strings, whicharkfee.Newhandles by leaving those estimator fields asnil. This is the intended design—all four programs are optional, not required. No breaking change or startup failure will occur if operators don't configure them.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/application/service.go (1)
1726-1820: Fix incorrect output categorization logic.Line 1819 appends every output to
offchainOutputs, including those already categorized as onchain outputs on line 1789. This means onchain outputs are included in bothonchainOutputsandoffchainOutputs, which will result in incorrect fee calculation.Apply this diff to fix the logic:
for outputIndex, output := range proof.UnsignedTx.TxOut { amount := uint64(output.Value) rcv := domain.Receiver{ Amount: amount, } isOnchainOutput := slices.Contains(message.OnchainOutputIndexes, outputIndex) if isOnchainOutput { // ... validation and setting rcv.OnchainAddress ... onchainOutputs = append(onchainOutputs, *output) } else { // ... validation and setting rcv.PubKey ... + offchainOutputs = append(offchainOutputs, *output) } receivers = append(receivers, rcv) - offchainOutputs = append(offchainOutputs, *output) }
🧹 Nitpick comments (2)
internal/infrastructure/feemanager/arkfee.go (2)
18-25: Consider storing the estimator as a pointer to avoid copying.The constructor dereferences the estimator pointer (line 24:
Estimator: *estimator), which embeds the entire estimator by value. Ifarkfee.Estimatoris large or contains non-copyable fields (mutexes, channels), this could lead to inefficiency or subtle bugs.Consider changing the struct to embed a pointer:
type arkFeeManager struct { - arkfee.Estimator + *arkfee.Estimator } func NewArkFeeManager(config arkfee.Config) (ports.FeeManager, error) { estimator, err := arkfee.New(config) if err != nil { return nil, err } - return arkFeeManager{Estimator: *estimator}, nil + return &arkFeeManager{Estimator: estimator}, nil }
60-72: Eliminate duplicate code.The functions
toArkFeeOffchainOutputandtoArkFeeOnchainOutputare identical. This duplication can be removed by using a single helper function.Apply this refactor:
-func toArkFeeOffchainOutput(output wire.TxOut) arkfee.Output { +func toArkFeeOutput(output wire.TxOut) arkfee.Output { return arkfee.Output{ Amount: uint64(output.Value), Script: hex.EncodeToString(output.PkScript), } } -func toArkFeeOnchainOutput(output wire.TxOut) arkfee.Output { - return arkfee.Output{ - Amount: uint64(output.Value), - Script: hex.EncodeToString(output.PkScript), - } -}Then update the calls in
GetIntentFees:for _, output := range offchainOutputs { - arkfeeOffchainOutputs = append(arkfeeOffchainOutputs, toArkFeeOffchainOutput(output)) + arkfeeOffchainOutputs = append(arkfeeOffchainOutputs, toArkFeeOutput(output)) } arkfeeOnchainOutputs := make([]arkfee.Output, 0, len(onchainOutputs)) for _, output := range onchainOutputs { - arkfeeOnchainOutputs = append(arkfeeOnchainOutputs, toArkFeeOnchainOutput(output)) + arkfeeOnchainOutputs = append(arkfeeOnchainOutputs, toArkFeeOutput(output)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/config/config.go(9 hunks)internal/core/application/service.go(11 hunks)internal/core/ports/fee.go(1 hunks)internal/infrastructure/feemanager/arkfee.go(1 hunks)pkg/ark-lib/arkfee/estimator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ark-lib/arkfee/estimator.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/core/application/service.go (5)
internal/core/ports/fee.go (1)
FeeManager(10-16)internal/core/application/types.go (1)
IntentFeeInfo(100-105)pkg/errors/errors.go (4)
INVALID_INTENT_PROOF(292-296)InvalidIntentProofMetadata(173-176)INTENT_INSUFFICIENT_FEE(325-329)IntentInsufficientFeeMetadata(195-200)pkg/ark-lib/intent/proof.go (2)
New(134-158)Proof(39-41)internal/core/domain/offchain_tx.go (1)
Tx(30-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (11)
internal/core/ports/fee.go (1)
10-16: LGTM!The
FeeManagerinterface is well-designed with a clear contract for computing fees based on boarding inputs, VTXO inputs, and onchain/offchain outputs.internal/infrastructure/feemanager/arkfee.go (1)
27-58: LGTM!The
GetIntentFeesimplementation correctly converts internal types to ArkFee types and invokes the estimator. Error handling is properly propagated.internal/core/application/service.go (5)
71-73: LGTM!The service struct now uses
feeManagerandintentFeeInfoinstead of a static fee value, enabling dynamic fee calculation based on CEL formulas.
1851-1884: Fee validation logic is well-structured.The dynamic fee calculation using
feeManager.GetIntentFeesproperly replaces the static fee check. Error handling and metadata are appropriately detailed.Note: This logic depends on the correct output categorization flagged in the previous comment (lines 1726-1820).
2269-2361: LGTM!The new
EstimateFeemethod provides a clean interface for fee estimation without intent registration. Time validation, input/output categorization, and error handling are all correctly implemented.
2041-2043: LGTM!The
GetInfomethod now correctly exposes the CEL fee programs viaintentFeeInfo, allowing clients to understand the fee structure.
2400-2417: Good defensive coding.The cleanup logic now includes proper nil/empty checks before deleting tree signing sessions and boarding input signatures, preventing potential cache operations with invalid keys.
internal/config/config.go (4)
127-134: LGTM!The Config struct now includes the four Intent fee program fields and a
feemanager, enabling dynamic CEL-based fee configuration.
654-665: LGTM!The
feeManager()method correctly constructs anArkFeeManagerusing the CEL program configuration and provides clear error handling.
577-579: LGTM!Validating the fee manager during config validation ensures that any CEL syntax errors or configuration issues are caught early.
839-857: LGTM!The application service is correctly instantiated with the fee manager and fee info, completing the integration of CEL-based dynamic fee estimation.
This PR is introducing the fee system specified by #639
ark-lib/arkfeepackage is exporting all the structs and types parsing and evaluating CEL programs.Add new env variables letting to specify the fee model for intent input
ARKD_INTENT_INPUT_FEE_PROGRAM, and intent output :ARKD_INTENT_OUTPUT_FEE_PROGRAM.the CEL envs are speicifed in arkfee/README.md
Example of valid CEL programs:
@Kukks @altafan please review
it closes #857
it closes #856
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.