Implement arkfee package #64
Conversation
WalkthroughIntegrated arkfee-based fee estimation into settlement and coin-selection: added arkfee imports and converters, refactored Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Ark as arkClient
participant FeeEst as arkfee.Estimator
participant CoinSel as CoinSelect
participant Types as Types (Utxo/Receiver/Vtxo)
Note over Ark,FeeEst: Create arkfee.Estimator from server FeeInfo.IntentFees
Caller->>Ark: settle(ctx, receivers[])
Ark->>Types: Receiver.ToArkFeeOutput() / Utxo.ToArkFeeInput() / Vtxo.ToArkFeeInput()
Ark->>FeeEst: NewEstimator(FeeInfo.IntentFees)
Ark->>CoinSel: CoinSelect(boardingUtxos, vtxos, outputs[], feeEstimator)
CoinSel->>FeeEst: EvalOutput(each output)
FeeEst-->>CoinSel: output fee estimates
CoinSel->>FeeEst: EvalOffchainInput / EvalOnchainInput(per input)
FeeEst-->>CoinSel: input fee estimates
CoinSel->>FeeEst: EvalOutput(change)
FeeEst-->>CoinSel: change fee
CoinSel-->>Ark: selected UTXOs, VTXOs, adjusted outputs[]
Ark-->>Caller: proceed with adjusted receivers / tx id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/utils/utils.go (2)
63-78: Bug:breakcauses remaining boarding UTXOs to be lost.When
selectedAmount >= amount, the current boarding UTXO is appended tonotSelectedBoarding, but thenbreakexits the loop and all subsequent boarding UTXOs are lost. This affects the dust-handling logic at lines 126-136 which relies onnotSelectedBoardinghaving available inputs.Apply this diff to properly collect all unselected boarding UTXOs:
for _, boardingUtxo := range boardingUtxos { if selectedAmount >= amount { notSelectedBoarding = append(notSelectedBoarding, boardingUtxo) - break + continue } selectedBoarding = append(selectedBoarding, boardingUtxo)
80-95: Bug:breakcauses remaining VTXOs to be lost.Same issue as with boarding UTXOs. When
selectedAmount >= amount, remaining VTXOs after the current one are not added tonotSelected, which affects the dust-handling logic at lines 115-125.Apply this diff to properly collect all unselected VTXOs:
for _, vtxo := range vtxos { if selectedAmount >= amount { notSelected = append(notSelected, vtxo) - break + continue } selected = append(selected, vtxo)
📜 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 (5)
client.go(7 hunks)client/client.go(2 hunks)go.mod(5 hunks)internal/utils/utils.go(5 hunks)types/types.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
internal/utils/utils.goclient.go
🧬 Code graph analysis (3)
types/types.go (1)
store/sql/sqlc/queries/models.go (1)
Utxo(22-35)
internal/utils/utils.go (2)
types/types.go (2)
Receiver(260-263)Utxo(229-240)client/client.go (1)
TapscriptsVtxo(99-102)
client.go (3)
types/types.go (2)
Receiver(260-263)Utxo(229-240)client/client.go (1)
TapscriptsVtxo(99-102)internal/utils/utils.go (1)
CoinSelect(28-143)
⏰ 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: Sdk integration tests
- GitHub Check: Sdk unit tests
🔇 Additional comments (8)
types/types.go (2)
242-250: LGTM - Correct mapping for boarding UTXO to ArkFee input.The conversion correctly uses
SpendableAtforExpiryand setsTypetoInputTypeBoarding, which aligns with the semantics of boarding UTXOs. The hardcodedWeight: 0appears intentional given the same pattern inTapscriptsVtxo.ToArkFeeInput()inclient/client.go.
265-274: LGTM - Clean output type detection.The method correctly determines output type based on whether the receiver address is onchain, providing proper ArkFee output classification.
client/client.go (1)
104-112: LGTM - Proper differentiation from Utxo.ToArkFeeInput().This correctly uses
InputTypeVtxoandExpiresAtfor VTXOs, contrasting withtypes.Utxo.ToArkFeeInput()which usesInputTypeBoardingandSpendableAtfor boarding UTXOs. The semantic distinction is appropriate.client.go (4)
720-748: LGTM - Clean fee estimation integration.The CollaborativeExit correctly fetches fee info from the server, creates the fee estimator, and passes it through the coin selection flow. The outputs returned from
selectFundsproperly include any change, which is then passed tojoinBatchWithRetry.
2076-2094: Approve "send all" logic with fee deduction.The logic correctly handles the case where a single receiver with amount ≤ 0 means "send all available funds". It iterates through all inputs, sums their values, and deducts the fee for each input, resulting in the maximum sendable amount.
2154-2165: LGTM - Fee estimator construction.The fee estimator is correctly instantiated using the offchain input/output fee expressions from the server's fee info.
750-756: Verify whetherSettle()with nil receivers is intentional or a bug.
Settle()callssettle(ctx, nil, opts...)passing nil receivers. If this results inselectFundscomputingamount=0, the settlement functionality would break. Determine ifbuildSettlementexplicitly handles the nil receivers case (e.g., collecting all available VTXOs to self), or if this is actually an unintended behavior that should send funds to self instead.go.mod (1)
8-8: Verify the ark-lib commit contains the arkfee package with expected exports.The ark-lib dependency at line 8 is updated to a specific commit from Dec 8, 2025. This codebase imports and uses
arkfeefromgithub.com/arkade-os/arkd/pkg/ark-lib/arkfeethroughout (Estimator, Input, Output, InputTypeBoarding, InputTypeVtxo, OutputTypeVtxo, OutputTypeOnchain, EvalOutput). Confirm this commit includes all required arkfee exports before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client.go (1)
2160-2170: Verify server response structure to prevent nil pointer dereference.Similar to line 727, this code accesses
info.Fees.IntentFeeswithout nil checks. Ensure consistent error handling across bothCollaborativeExitandsettlefunctions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
client.go
⏰ 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). (1)
- GitHub Check: Sdk unit tests
🔇 Additional comments (3)
client.go (3)
17-17: LGTM!The arkfee package import supports the fee estimation integration described in the PR objectives.
317-319: No action needed. SendOffChain correctly passesnilfor thefeeEstimatorparameter. This function handles offchain-to-offchain payments (vtxo transfers only) and explicitly excludes boarding utxos, making fee estimation unnecessary. Fee estimation is only required inCollaborativeExitandsettlebecause they interact with onchain UTXOs, which SendOffChain does not.Likely an incorrect or invalid review comment.
721-732: The code is safe from nil pointer dereference; no changes needed.
IntentFeesis a struct value field (not a pointer) withintypes.FeeInfo, so it's always allocated when aFeeInfois created. TheparseFeesfunction ensuresIntentFeeInfois always populated in the returnedFeeInfostruct. Direct access toinfo.Fees.IntentFees.OffchainInputandOffchainOutputcannot cause a nil dereference.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client.go (1)
2032-2113: Sweep‑all path in selectFunds still double-counts input fees and can make CoinSelect impossible to satisfy.In
selectFunds:
When
len(outputs) == 0, you create a single offchain receiver withAmount: 0.When
len(outputs) == 1 && outputs[0].Amount <= 0(the sweep‑all case used bySettle()), you:outputs[0].Amount += utxo.Amount fees, _ := feeEstimator.EvalOnchainInput(...) outputs[0].Amount -= uint64(fees.ToSatoshis()) outputs[0].Amount += vtxo.Amount fees, _ := feeEstimator.EvalOffchainInput(...) outputs[0].Amount -= uint64(fees.ToSatoshis())i.e. you pre‑subtract per‑input fees for all candidate boarding UTXOs and VTXOs.
You then call
utils.CoinSelect(..., outputs, a.Dust, opts.WithExpirySorting, feeEstimator)whereCoinSelectitself:
- Adds output fees to
amount, and- Adds input fees to
amountagain for each selected input.The net effect for sweep‑all is:
outputs[0].Amountstarts at total funds minus all candidate input fees.CoinSelectthen adds fees again for each selected input and also adds output fees.If the estimator charges a positive fee for the output, you end up requiring:
sum(selected inputs) >= sum(all inputs) + outputFeewhich is impossible when all available inputs are selected;
CoinSelectwill return"not enough funds to cover amount"even though the actual balance is sufficient. This is essentially the same issue previously flagged about “fee calculation before coin selection” in the sweep‑all path.To fix this, you’ll want to avoid double‑charging input fees. Two possible directions:
- Option A (recommended): Don’t mutate
outputs[0].Amountbased on per‑input fees at all. LetCoinSelectsee the raw sweep output (e.g., zero or a nominal target), select inputs withfeeEstimatorfully applied, and then post‑compute the effective payout from the selected inputs and resultingchangeAmount.- Option B: If you keep the pre‑sweep logic, call
CoinSelectwithfeeEstimator == nilin this specific branch so that input fees are only accounted for once (in your pre‑sweep calculation), and adjust how you treat the output fee.Either way, the current combination of pre‑subtracting per‑input fees and passing the same estimator into
CoinSelectmakes the sweep‑all case incorrect and, for realistic positive output fees, unsatisfiable.internal/utils/utils.go (1)
110-118: Fix possible uint64 underflows when subtracting fee from change.
changeisuint64, and you subtract fees from it in three places:
- Change-output fee:
change -= uint64(fees.ToSatoshis())- Extra offchain input when
change < dust:change += notSelected[0].Amount change -= uint64(fees.ToSatoshis())- Extra boarding UTXO in the same branch:
change += notSelectedBoarding[0].Amount change -= uint64(fees.ToSatoshis())If
fees.ToSatoshis()exceeds the currentchange(e.g., very small change or pathological fee config), these subtractions will underflow and wrap to a very large value, corrupting the selection result.Guard the subtraction with a bounds check and decide on a safe behavior (clamp to zero, or return an explicit error). For example:
- change -= uint64(fees.ToSatoshis()) + feeAmt := uint64(fees.ToSatoshis()) + if feeAmt > change { + // Either: + // return nil, nil, 0, fmt.Errorf("change output fee %d exceeds change %d", feeAmt, change) + // or clamp: + change = 0 + } else { + change -= feeAmt + }Apply the same pattern in the two branches that adjust
changeafter adding an extra input.Also applies to: 120-131, 133-142
🧹 Nitpick comments (3)
types/types.go (2)
10-17: Public FeeInfo change to arkfee.Config is a breaking API surface change.Switching
FeeInfo.IntentFeestoarkfee.Configis a sensible consolidation but breaks existing callers that referenced the old struct fields. Ensure this is accompanied by a documented SDK version bump and migration notes for downstream users (or a compatibility shim, if needed).Also applies to: 55-58
235-239: Tighten ArkFee conversion helpers to avoid silent failures.
Utxo.ToArkFeeInput()andReceiver.ToArkFeeOutput()look correct in terms of mapping amounts and pkScript bytes, butToArkFeeOutput()swallowsToTxOuterrors and returns a zero-valuearkfee.Output. If this helper is ever used with unvalidated receivers, fee estimation could silently operate on{Amount:0, Script:""}.Consider either:
- Returning
(arkfee.Output, error)fromToArkFeeOutput(), or- At least logging/annotating the error at call sites where bad receivers should be impossible.
This will make future misuse easier to diagnose.
Also applies to: 254-263
store/file/config_store.go (1)
76-85: Config-store intent fee mapping matches new arkfee.Config; consider upgrade semantics.Persisting
intent_fees.offchain_input/outputandonchain_input/outputfromdata.Fees.IntentFees.{Intent*Program}is consistent with the new arkfee-based config and with the decode logic instore/file/types.go.If older
state.jsonfiles contain legacy values that are not valid arkfee programs, be prepared either to document that users must re-run initialization or to add a small migration layer.
📜 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 (9)
client.go(7 hunks)client/client.go(2 hunks)client/grpc/client.go(2 hunks)client/rest/client.go(2 hunks)go.mod(5 hunks)internal/utils/utils.go(5 hunks)store/file/config_store.go(1 hunks)store/file/types.go(2 hunks)types/types.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/client.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
internal/utils/utils.goclient.go
🧬 Code graph analysis (7)
internal/utils/utils.go (3)
types/types.go (2)
Receiver(249-252)Utxo(222-233)client/client.go (1)
TapscriptsVtxo(99-102)explorer/service.go (1)
Output(80-84)
types/types.go (2)
explorer/service.go (2)
Utxo(105-111)Output(80-84)store/sql/sqlc/queries/models.go (1)
Utxo(22-35)
client/grpc/client.go (3)
types/types.go (2)
FeeInfo(55-58)Config(26-47)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
FeeInfo(569-575)FeeInfo(588-588)FeeInfo(603-605)client/rest/service/model_fee_info.go (1)
FeeInfo(21-24)
store/file/types.go (1)
types/types.go (2)
Config(26-47)OnchainOutput(305-312)
store/file/config_store.go (1)
types/types.go (1)
OnchainOutput(305-312)
client/rest/client.go (4)
types/types.go (2)
FeeInfo(55-58)Config(26-47)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
FeeInfo(569-575)FeeInfo(588-588)FeeInfo(603-605)client/rest/service/model_fee_info.go (1)
FeeInfo(21-24)store/service.go (1)
Config(35-40)
client.go (4)
types/types.go (2)
Receiver(249-252)Utxo(222-233)types.go (1)
CoinSelectOptions(134-145)client/client.go (1)
TapscriptsVtxo(99-102)internal/utils/utils.go (1)
CoinSelect(28-149)
🔇 Additional comments (7)
go.mod (1)
3-4: Dependency bumps align with arkfee integration; verify toolchain & module graph.The updated Go version and ark-lib/protobuf/CEL-related dependencies look consistent with the new arkfee-based fee handling. Please make sure your CI/build runs
go mod tidyand compiles/tests this module with the configured Go toolchain so that any transitive incompatibilities surface early.Also applies to: 8-9, 26-27, 31-37, 64-65, 109-110, 133-134
store/file/types.go (1)
8-12: ArkFee config reconstruction from file-store looks consistent.
decode()now buildstypes.FeeInfowith anarkfee.Configconstructed from the storedintent_feesfields, which matches whatconfig_store.AddDatawrites. This keeps file persistence aligned with the new fee model.Just ensure arkfee’s
Configparsing/validation accepts these string fields as-is (especially for environments upgrading from older configs).Also applies to: 88-97, 150-158
client/grpc/client.go (1)
12-25: gRPC fee parsing → arkfee.Config wiring looks correct; handle empty-config case.
parseFeesnow:
- Parses
TxFeeRatefrom the string field, and- Maps
fees.IntentFee.{OffchainInput,OffchainOutput,OnchainInput,OnchainOutput}into the fourIntent*Programfields ofarkfee.Config.This is the expected shape for later
arkfee.New(info.Fees.IntentFees)calls. WhenIntentFeeis absent, the generated getters will yield empty strings; make surearkfee.Neweither rejects that with a clear error (as you already propagate) or that the server always populates a valid config.Also applies to: 515-541
client/rest/client.go (1)
13-20: REST fee parsing mirrors gRPC mapping into arkfee.Config.
parseFeescorrectly:
- Parses
TxFeeRatewhen present, and- Builds
arkfee.Configfromfees.IntentFee.{OffchainInput,OffchainOutput,OnchainInput,OnchainOutput}.This keeps REST and gRPC in sync for fee configuration. As with the gRPC path, please confirm that
ark_service.FeeInfo.GetIntentFee()behaves sanely when the field is absent, and that passing the resulting (possibly empty) config intoarkfee.Newis either supported or yields a clear error.Also applies to: 530-552
internal/utils/utils.go (1)
28-35: Fee-aware CoinSelect integration is structurally sound.Extending
CoinSelectto acceptoutputs []types.Receiverand a*arkfee.Estimatorand then:
- Adding output fees up front,
- Adding input fees as inputs are selected,
fits well with the new arkfee-based fee model and keeps call sites clean. Callers passing
nilforfeeEstimatoralso preserve the old behavior.Also applies to: 40-57, 76-84, 95-101
client.go (2)
16-18: CollaborativeExit’s feeEstimator wiring is coherent with the new arkfee flow.Fetching
info := a.client.GetInfo(ctx), constructingfeeEstimator := arkfee.New(info.Fees.IntentFees), and routing throughselectFundsgives you a single source of truth for fee programs across CollaborativeExit and Settle. The pre-check onbalance < amountis harmless, though note thatselectFunds/arkfee will still be the ultimate arbiter once fees are factored in.Also applies to: 677-745
2116-2179: Settle/receivers validation + feeEstimator integration look good; leverage selectFunds fix above.The updated
settle:
- Validates each provided receiver as an Ark address for this signer and enforces
receiver.Amount >= a.Dust.- Fetches
info := a.client.GetInfo(ctx)and constructsfeeEstimator := arkfee.New(info.Fees.IntentFees).- Calls
selectFundsto obtain boarding UTXOs, vtxos, and final outputs, then passes those intojoinBatchWithRetry.Once the sweep‑all behavior in
selectFundsis corrected, this flow will give you a single, arkfee-driven path for both explicit receivers and full-balance settlements without duplicating fee logic here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/utils/utils.go (2)
107-115: Potential underflow when subtracting change output fee.If
uint64(fees.ToSatoshis())exceedschange, this will cause an unsigned integer underflow. Add a check to prevent this:if feeEstimator != nil { - fees, err := feeEstimator.EvalOffchainOutput(arkfee.Output{ + fees, err := feeEstimator.EvalOffchainOutput(arkfee.Output{ Amount: change, }) if err != nil { return nil, nil, 0, err } - change -= uint64(fees.ToSatoshis()) + feeAmount := uint64(fees.ToSatoshis()) + if feeAmount > change { + return nil, nil, 0, fmt.Errorf("change output fee (%d) exceeds change amount (%d)", feeAmount, change) + } + change -= feeAmount }
117-143: Potential underflow when subtracting input fees from change.Similar to the change output fee issue, subtracting input fees from
changeat lines 127 and 138 can underflow if the fee exceeds the change amount. Consider adding checks:if change < dust { if len(notSelected) > 0 { selected = append(selected, notSelected[0]) change += notSelected[0].Amount if feeEstimator != nil { fees, err := feeEstimator.EvalOffchainInput(notSelected[0].ToArkFeeInput()) if err != nil { return nil, nil, 0, err } - change -= uint64(fees.ToSatoshis()) + feeAmount := uint64(fees.ToSatoshis()) + if feeAmount > change { + return nil, nil, 0, fmt.Errorf("input fee (%d) exceeds available change (%d)", feeAmount, change) + } + change -= feeAmount } } else if len(notSelectedBoarding) > 0 { selectedBoarding = append(selectedBoarding, notSelectedBoarding[0]) change += notSelectedBoarding[0].Amount if feeEstimator != nil { fees, err := feeEstimator.EvalOnchainInput(notSelectedBoarding[0].ToArkFeeInput()) if err != nil { return nil, nil, 0, err } - change -= uint64(fees.ToSatoshis()) + feeAmount := uint64(fees.ToSatoshis()) + if feeAmount > change { + return nil, nil, 0, fmt.Errorf("boarding input fee (%d) exceeds available change (%d)", feeAmount, change) + } + change -= feeAmount } } else { change = 0 } }client.go (1)
2004-2035: Critical: Fee calculation occurs before coin selection in sweep-all case.When sweeping all funds (
amount <= 0), the code calculates fees for ALL boarding UTXOs and vtxos (lines 2011-2027) before callingCoinSelectat line 2030. However,CoinSelectmay not select all inputs—it selects based on the target amount. This creates a mismatch:
- Fees are calculated for all available inputs
CoinSelectselects only a subset of inputs to cover the target amount- The actual fees paid will differ from the pre-calculated fees
outputs[0].Amountwill be incorrectThis breaks the sweep-all functionality.
Recommended fix:
For the sweep-all case, either:
- Modify
CoinSelectto have a sweep-all mode that selects all inputs whenfeeEstimatoris provided and amount is computed from all inputs- Or refactor to call
CoinSelectfirst (with a large enough target), then calculate the actual amount based on selected inputs and their fees
🧹 Nitpick comments (1)
client.go (1)
2047-2111: LGTM! The settle function refactoring is well-structured.The function correctly:
- Validates receiver addresses and amounts
- Creates a fee estimator from server info
- Uses
selectFundsto perform fee-aware coin selection- Passes adjusted outputs to the batch joining logic
Optional: Update comment for clarity
- // coinselect boarding utxos and vtxos + // Select inputs and compute adjusted outputs including fees and change boardingUtxos, vtxos, outputs, err := a.selectFunds(This better reflects that
selectFundsdoes more than just coin selection—it also handles fee calculation and change output creation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
client.goclient/grpc/client.gogo.modinternal/utils/utils.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
internal/utils/utils.goclient.go
🧬 Code graph analysis (3)
client/grpc/client.go (3)
types/types.go (2)
FeeInfo(55-58)Config(26-47)client/rest/service/model_fee_info.go (1)
FeeInfo(21-24)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
FeeInfo(569-575)FeeInfo(588-588)FeeInfo(603-605)
internal/utils/utils.go (3)
types/types.go (2)
Receiver(249-252)Utxo(222-233)client/client.go (1)
TapscriptsVtxo(99-102)explorer/service.go (1)
Output(80-84)
client.go (4)
types/types.go (2)
Receiver(249-252)Utxo(222-233)types.go (1)
CoinSelectOptions(134-145)client/client.go (1)
TapscriptsVtxo(99-102)internal/utils/utils.go (1)
CoinSelect(28-146)
⏰ 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: Sdk integration tests
- GitHub Check: Sdk unit tests
🔇 Additional comments (8)
go.mod (2)
8-8: Ark-lib version update is appropriate for arkfee integration.The ark-lib dependency update properly supports the arkfee fee estimation implementation used throughout the codebase (client.go, types.go, and related modules). The CEL-related dependencies appearing in go.mod are transitive dependencies from ark-lib, not direct additions to this project.
133-134: These are indirect dependencies (transitive, not directly imported in the codebase). No compatibility issues found—go mod verifypasses successfully with no breaking imports or version conflicts.internal/utils/utils.go (3)
28-54: LGTM! Output fee calculation logic is correct.The function signature update and output fee calculation implementation look good:
- Properly differentiates between onchain and offchain outputs
- Correctly accumulates fees into the total amount required
- Handles errors appropriately
67-82: LGTM! Boarding UTXO input fee handling is correct.The onchain input fee calculation for boarding UTXOs is properly implemented:
- Evaluates fee using
EvalOnchainInput- Adds fee to the target amount, correctly accounting for the cost of using each input
- Handles errors appropriately
84-99: LGTM! Vtxo input fee handling is correct.The offchain input fee calculation for vtxos is properly implemented and consistent with the boarding UTXO handling.
client.go (2)
330-332: LGTM! SendOffChain correctly passes nil for feeEstimator.For pure off-chain transfers, not using fee estimation appears intentional and maintains backward compatibility.
733-757: LGTM! CollaborativeExit fee estimation integration is correct.The implementation properly:
- Retrieves fee configuration from the server
- Creates a fee estimator
- Passes it to
selectFundsfor accurate coin selection- Uses the adjusted outputs for batch joining
client/grpc/client.go (1)
12-12: No action required. The arkfee library compatibility and field mapping inparseFeesare correct and consistent with the arkfee.Config struct definition in the specified version.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @go.mod:
- Line 32: go.mod still lists unused CEL-related indirect modules (cel.dev/expr,
antlr4-go/antlr, google/cel-go, stoewer/go-strcase); remove those module lines
from go.mod and run `go mod tidy` to purge transitive entries, then run the
provided verification commands (search for "cel.dev", "google/cel-go", "antlr",
"go-strcase", and CEL API patterns) to confirm no code imports or references
remain; update the PR with the cleaned go.mod/go.sum and mention that `go mod
tidy` was run.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod
⏰ 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: Sdk integration tests
- GitHub Check: Sdk unit tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/transaction_test.go (1)
519-519: Consider polling with timeout instead of fixed sleep.The increased sleep duration from 1 to 3 seconds suggests the test was experiencing timing-related flakiness. While this may improve reliability, a fixed sleep is brittle and can unnecessarily slow down tests in fast environments or still fail in slower CI pipelines.
A more robust approach would be to poll
GetTransactionHistorywith a reasonable timeout (e.g., 5 seconds) and a short retry interval (e.g., 100-200ms), returning as soon as the transaction appears.♻️ Example polling implementation
- time.Sleep(3 * time.Second) - - history, err = restoredAlice.GetTransactionHistory(ctx) - require.NoError(t, err) - require.True(t, slices.ContainsFunc(history, func(tx types.Transaction) bool { - return tx.TransactionKey.String() == txid - })) + // Poll for the transaction to appear in history + require.Eventually(t, func() bool { + history, err := restoredAlice.GetTransactionHistory(ctx) + if err != nil { + return false + } + return slices.ContainsFunc(history, func(tx types.Transaction) bool { + return tx.TransactionKey.String() == txid + }) + }, 5*time.Second, 200*time.Millisecond, "transaction should appear in history after auto-finalization")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/transaction_test.go
⏰ 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: Sdk unit tests
- GitHub Check: Sdk integration tests
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @client/grpc/client.go:
- Around line 219-231: The EstimateIntentFee method in grpcClient returns -1 on
error; change it to return the zero value for int64 (0) when an error occurs.
Locate func (a *grpcClient) EstimateIntentFee(ctx context.Context, proof,
message string) and replace the return statement in the error branch (currently
"return -1, err") with "return 0, err" so callers receive the idiomatic zero
value alongside the error.
In @client/rest/client.go:
- Around line 130-144: The function restClient.EstimateIntentFee currently
returns -1 on error; change the error return to return the zero value 0 instead
(i.e., return 0, err) so callers rely on the error rather than a sentinel value;
update the error branch in EstimateIntentFee to return 0 instead of -1 and leave
the successful return as resp.GetFee(), nil.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (8)
api-spec/openapi/swagger/ark/v1/service.openapi.jsonclient/client.goclient/grpc/client.goclient/rest/client.goclient/rest/service/.openapi-generator/FILESclient/rest/service/api_ark_service.goclient/rest/service/model_estimate_intent_fee_request.goclient/rest/service/model_estimate_intent_fee_response.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
client/client.go
🧬 Code graph analysis (3)
client/rest/service/model_estimate_intent_fee_request.go (1)
api-spec/protobuf/gen/ark/v1/service.pb.go (3)
EstimateIntentFeeRequest(339-344)EstimateIntentFeeRequest(357-357)EstimateIntentFeeRequest(372-374)
client/rest/client.go (5)
client/rest/service/model_estimate_intent_fee_request.go (1)
EstimateIntentFeeRequest(21-23)api-spec/protobuf/gen/ark/v1/service.pb.go (3)
EstimateIntentFeeRequest(339-344)EstimateIntentFeeRequest(357-357)EstimateIntentFeeRequest(372-374)api-spec/protobuf/gen/ark/v1/types.pb.go (6)
Intent(441-447)Intent(460-460)Intent(475-477)FeeInfo(569-575)FeeInfo(588-588)FeeInfo(603-605)types/types.go (2)
FeeInfo(55-58)Config(26-47)client/rest/service/model_fee_info.go (1)
FeeInfo(21-24)
client/rest/service/model_estimate_intent_fee_response.go (1)
api-spec/protobuf/gen/ark/v1/service.pb.go (3)
EstimateIntentFeeResponse(383-388)EstimateIntentFeeResponse(401-401)EstimateIntentFeeResponse(416-418)
⏰ 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: Sdk unit tests
- GitHub Check: Sdk integration tests
🔇 Additional comments (10)
api-spec/openapi/swagger/ark/v1/service.openapi.json (2)
90-130: LGTM!The new EstimateIntentFee endpoint follows the established API patterns and includes proper request/response definitions with standard error handling.
633-651: LGTM!The schema definitions are well-structured. The use of
int64for the fee field is appropriate for representing satoshi amounts.client/rest/service/.openapi-generator/FILES (1)
12-13: LGTM!The new model files align with the EstimateIntentFeeRequest and EstimateIntentFeeResponse schemas added to the OpenAPI specification.
client/client.go (2)
31-31: LGTM!The new interface method follows the established pattern of other intent-related methods and appropriately returns
int64for the fee amount in satoshis.
105-118: Verify ifWeight: 0is correct—requires checking the arkfee package specification.The
ToArkFeeInputmethod setsWeightto a hardcoded 0. TheVtxostruct contains no weight field to source this value from, and there are no comments explaining whether this is intentional. This requires verification against the externalarkfeepackage (fromgithub.com/arkade-os/arkd/pkg/ark-lib/arkfee) to confirm:
- Whether
OffchainInput.Weightis actually used in fee calculations- If 0 is an acceptable/valid value for offchain input weight estimation
- Whether weight should be dynamically calculated from input characteristics
client/grpc/client.go (1)
532-558: LGTM!The refactoring correctly migrates from
types.IntentFeeInfotoarkfee.Config, mapping the intent fee fields to their corresponding program properties. This aligns with the PR's goal of integrating arkfee-based fee estimation.client/rest/client.go (1)
546-568: LGTM!The
parseFeesimplementation correctly migrates toarkfee.Configand is consistent with the gRPC client implementation, ensuring uniform fee handling across transport layers.client/rest/service/model_estimate_intent_fee_request.go (1)
1-126: LGTM! Standard OpenAPI-generated model code.This file introduces the
EstimateIntentFeeRequestmodel with proper constructors, accessors, JSON serialization, and nullable wrapper support. The implementation follows established patterns and is consistent with the protobuf definitions.client/rest/service/api_ark_service.go (1)
265-384: LGTM! Properly structured API endpoint implementation.The new
ArkServiceEstimateIntentFeeendpoint follows the established patterns in this file, including proper request validation, error handling, and response decoding. The implementation is consistent with other ArkService endpoints.client/rest/service/model_estimate_intent_fee_response.go (1)
1-126: LGTM! Standard OpenAPI-generated response model.This file introduces the
EstimateIntentFeeResponsemodel with aFeefield and complete accessor/serialization support. The implementation follows the same patterns as other generated models and properly handles optional fields.
arkade-os/arkd#860
Summary by CodeRabbit
New Features
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.