Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds persistent per-round collected fees: DB schema, repo, domain, application logic, gRPC admin endpoint to query fees by time range, and tests (unit, integration, e2e). Also extracts helpers to compute boarding input and collected fees and wires fees through finalization and events. Changes
Sequence Diagram(s)sequenceDiagram
participant Finalization as Finalizer
participant Domain as Domain Model
participant Repo as Repository
participant DB as Database
Finalization->>Finalization: calculateBoardingInputAmount(ptx)
Finalization->>Finalization: calculateCollectedFees(round, boardingAmount)
Finalization->>Domain: EndFinalization(forfeitTxs, txn, collectedFees)
Domain->>Domain: Emit RoundFinalized event (Fees = collectedFees)
Domain->>Repo: UpsertRound(round with CollectedFees)
Repo->>DB: INSERT/UPDATE round.fees
DB-->>Repo: OK
sequenceDiagram
participant Client as gRPC Client
participant Handler as Admin Handler
participant Service as AdminService
participant Repo as RoundRepository
participant DB as Database
Client->>Handler: GetCollectedFees(after, before)
Handler->>Handler: validate after/before
Handler->>Service: GetCollectedFees(ctx, after, before)
Service->>Repo: GetCollectedFees(ctx, after, before)
Repo->>DB: SELECT COALESCE(SUM(fees),0) WHERE ended=true AND failed=false AND timestamp BETWEEN after AND before
DB-->>Repo: sum
Repo-->>Service: uint64 total
Service-->>Handler: uint64 total
Handler-->>Client: GetCollectedFeesResponse{CollectedFees}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
internal/core/domain/round_test.go (1)
475-488: Add one non-zero collected-fees assertion case.These tests now compile with the new signature, but they don’t verify the new behavior. Add a case with non-zero fees and assert it is reflected in
RoundFinalizedand round state.Suggested test enhancement
- events, err = round.EndFinalization(forfeitTxs, finalCommitmentTx, 0) + const collectedFees uint64 = 1234 + events, err = round.EndFinalization(forfeitTxs, finalCommitmentTx, collectedFees) require.NoError(t, err) ... event, ok := events[0].(domain.RoundFinalized) require.True(t, ok) + require.Equal(t, collectedFees, event.CollectedFees) + require.Equal(t, collectedFees, round.CollectedFees)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/domain/round_test.go` around lines 475 - 488, Add a new subtest that calls round.EndFinalization with a non-zero collected fees value and assert the value appears on the emitted domain.RoundFinalized event and on the round state; specifically, invoke round.EndFinalization(forfeitTxs, finalCommitmentTx, nonZeroFees) in a separate test case, require no error, cast events[0] to domain.RoundFinalized and assert event.CollectedFees equals nonZeroFees and event.Timestamp equals round.EndingTimestamp, and also assert round.CollectedFees (or the corresponding round field) reflects nonZeroFees along with the existing IsStarted/IsEnded/IsFailed checks so the new signature behavior is verified.internal/infrastructure/db/service_test.go (1)
711-733: Add a regression case forended=true+failed=truerounds.Current failure fixture is never finalized, so it does not catch the case where a round is finalized and then marked failed. That edge case should stay excluded from collected-fee totals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/service_test.go` around lines 711 - 733, Create a regression test that covers the “ended=true and failed=true” edge case by constructing a round that is first finalized and then marked failed (e.g., build a domain.NewRoundFromEvents sequence containing domain.RoundStarted, domain.RoundFinalized with Timestamp X, then domain.RoundFailed with Timestamp > X), set failedRound.CollectedFees to a non-zero value and call repo.AddOrUpdateRound(ctx, *failedRound), and then assert this round is excluded from collected-fee totals; update the existing fixture that currently only includes an unfinished failed round (the failedRound variable and its AddOrUpdateRound call) to use this finalized-then-failed event sequence so the test verifies rounds with ended=true && failed=true are ignored.internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (1)
2185-2198: The cast is explicit but unguarded.domain.Round.CollectedFees(uint64) is converted atinternal/infrastructure/db/sqlite/round_repo.go:94andinternal/infrastructure/db/postgres/round_repo.go:94using direct casts (int64(round.CollectedFees)) with no bounds validation. While the maximum Bitcoin supply (~2.1×10^15 satoshis) falls well withinmath.MaxInt64(~9.2×10^18), adding explicit bounds checking would strengthen the implementation as defensive programming against future changes or unexpected values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go` around lines 2185 - 2198, domain.Round.CollectedFees (uint64) is being directly cast to int64 when populating UpsertRoundParams.CollectedFees in round_repo.go (see the cast at internal/infrastructure/db/sqlite/round_repo.go:94 and internal/infrastructure/db/postgres/round_repo.go:94); add an explicit bounds check against math.MaxInt64 before casting and handle the out-of-range case (e.g., return an error from the repository function or otherwise surface/log the condition) instead of unguarded casting so you never overflow the int64 field in UpsertRoundParams; reference domain.Round.CollectedFees and UpsertRoundParams.CollectedFees when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/alert.go`:
- Around line 121-125: In calculateCollectedFees, don't add boardingInputAmount
per intent and avoid uint64 underflow/overflow: move adding boardingInputAmount
outside the loop (add it once to the total), and inside the loop compute
per-intent fee as max(0, intent.TotalInputAmount() - intent.TotalOutputAmount())
to prevent unsigned wrap; when accumulating, guard additions against uint64
overflow (e.g., check before adding or use math/bits.Add64) so collectedFees
never wraps. Ensure you reference calculateCollectedFees, round.Intents,
intent.TotalInputAmount() and intent.TotalOutputAmount() when making the
changes.
In `@internal/infrastructure/db/badger/ark_repo.go`:
- Around line 130-133: The current query always applies StartingTimestamp >
after (via badgerhold.Where(...).And("StartingTimestamp").Gt(after)), but per
API contract passing after=0 should be treated as “unset”; update the code that
builds the BadgerHold query (the query variable in ark_repo.go where
badgerhold.Where("Stage.Ended") is used) to only append the
.And("StartingTimestamp").Gt(after) clause when after > 0 (similar to the
existing before > 0 check), so that after==0 does not filter by
StartingTimestamp.
- Around line 127-144: GetCollectedFees sums Round.CollectedFees but
addOrUpdateRound currently omits CollectedFees when building the rnd value
stored in the DB, so reloaded rounds have zeroed fees; update addOrUpdateRound
to preserve/persist CollectedFees (copy round.CollectedFees into rnd before the
upsert or ensure the persistence mapping includes the CollectedFees field) so
that GetCollectedFees reads the correct persisted value, and keep the rest of
the upsert logic in addOrUpdateRound unchanged.
In
`@internal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sql`:
- Line 1: Update the migration to enforce a non-negative invariant on the new
column by adding a CHECK constraint for collected_fees; keep the existing NOT
NULL DEFAULT 0 but append a CHECK (collected_fees >= 0) or add an explicit
constraint (e.g., ADD CONSTRAINT round_collected_fees_nonnegative CHECK
(collected_fees >= 0)) to the ALTER TABLE statement that adds collected_fees so
the database rejects negative values at the boundary.
In `@internal/infrastructure/db/postgres/round_repo.go`:
- Line 94: The CollectedFees fields are being cast between uint64 and int64
without bounds checks (e.g., round.CollectedFees → CollectedFees int64 and DB
int64 → uint64), which can overflow/underflow; before casting in the
marshal/save path (where round.CollectedFees is converted to int64) ensure value
≤ math.MaxInt64 and return an error if larger, and in the unmarshal/load path
(where DB CollectedFees int64 is converted to uint64) ensure the int64 is
non-negative before converting and return/report an error if negative. Update
all occurrences referenced (the current cast at round.CollectedFees and the
similar conversions around the other ranges mentioned) to perform these explicit
checks and propagate an error instead of silently wrapping.
In `@internal/infrastructure/db/postgres/sqlc/query.sql`:
- Around line 443-445: The aggregation query currently filters only on ended =
true so it still includes rounds that were later marked failed; update the WHERE
clause in the SQL (the block referencing ended, starting_timestamp, `@after` and
`@before`) to explicitly exclude failed rounds by adding a condition such as AND
failed = FALSE (or AND NOT failed / AND is_failed = FALSE depending on the
actual column name) so only non-failed, ended rounds are included in the revenue
totals.
In
`@internal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sql`:
- Line 1: The new column on table "round" (collected_fees) must forbid negative
values at the schema level: update the migration so the ADD COLUMN defines
collected_fees as INTEGER NOT NULL DEFAULT 0 with a CHECK constraint enforcing
collected_fees >= 0; if the SQLite version in use does not support adding a
CHECK via ALTER TABLE, implement the safe table-recreate pattern (create new
table with the CHECK, copy rows, drop old table, rename) to ensure the
non-negative invariant is applied.
In `@internal/infrastructure/db/sqlite/round_repo.go`:
- Line 94: The code silently coerces round.CollectedFees between uint64 and
int64 (the CollectedFees assignment and corresponding db read code), risking
overflow/wrap and a default branch that returns 0, nil for unexpected types;
change the write path to validate that round.CollectedFees fits into int64
before casting and return an explicit error if it does not, and change the
read/unmarshal path (the switch/scan handling the CollectedFees column) to
explicitly handle expected types (int64, int32, uint64, string numeric) with
range checks converting to uint64, and remove the catch-all default that returns
0, nil—instead return a descriptive error for unexpected types or out-of-range
values; apply the same hardening to the other occurrences referenced around the
CollectedFees handling (the other two blocks noted).
In `@internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go`:
- Around line 368-373: The SQLite query returns interface{} because the SUM
result isn't explicitly cast; update the SQLite SQL for the queries referenced
by SelectCollectedFees and SelectExpiringLiquidityAmount to cast the aggregated
COALESCE(SUM(...), 0) to an integer (e.g., CAST(COALESCE(SUM(collected_fees), 0)
AS INTEGER)) so SQLC will infer int64, then regenerate SQLC artifacts; target
the SQL string/constants that define selectCollectedFees and the equivalent
selectExpiringLiquidityAmount so their generated Go functions
(SelectCollectedFees, SelectExpiringLiquidityAmount) return int64 instead of
interface{}.
In `@internal/infrastructure/db/sqlite/sqlc/query.sql`:
- Around line 442-447: The query SelectCollectedFees currently always applies
starting_timestamp > sqlc.arg('after'), so after=0 is not treated as "no lower
bound"; update the WHERE clause to short-circuit when after <= 0 by adding a
condition like (sqlc.arg('after') <= 0 OR starting_timestamp >
sqlc.arg('after')) alongside the existing before check so that passing after=0
disables the lower-bound filter.
---
Nitpick comments:
In `@internal/core/domain/round_test.go`:
- Around line 475-488: Add a new subtest that calls round.EndFinalization with a
non-zero collected fees value and assert the value appears on the emitted
domain.RoundFinalized event and on the round state; specifically, invoke
round.EndFinalization(forfeitTxs, finalCommitmentTx, nonZeroFees) in a separate
test case, require no error, cast events[0] to domain.RoundFinalized and assert
event.CollectedFees equals nonZeroFees and event.Timestamp equals
round.EndingTimestamp, and also assert round.CollectedFees (or the corresponding
round field) reflects nonZeroFees along with the existing
IsStarted/IsEnded/IsFailed checks so the new signature behavior is verified.
In `@internal/infrastructure/db/service_test.go`:
- Around line 711-733: Create a regression test that covers the “ended=true and
failed=true” edge case by constructing a round that is first finalized and then
marked failed (e.g., build a domain.NewRoundFromEvents sequence containing
domain.RoundStarted, domain.RoundFinalized with Timestamp X, then
domain.RoundFailed with Timestamp > X), set failedRound.CollectedFees to a
non-zero value and call repo.AddOrUpdateRound(ctx, *failedRound), and then
assert this round is excluded from collected-fee totals; update the existing
fixture that currently only includes an unfinished failed round (the failedRound
variable and its AddOrUpdateRound call) to use this finalized-then-failed event
sequence so the test verifies rounds with ended=true && failed=true are ignored.
In `@internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go`:
- Around line 2185-2198: domain.Round.CollectedFees (uint64) is being directly
cast to int64 when populating UpsertRoundParams.CollectedFees in round_repo.go
(see the cast at internal/infrastructure/db/sqlite/round_repo.go:94 and
internal/infrastructure/db/postgres/round_repo.go:94); add an explicit bounds
check against math.MaxInt64 before casting and handle the out-of-range case
(e.g., return an error from the repository function or otherwise surface/log the
condition) instead of unguarded casting so you never overflow the int64 field in
UpsertRoundParams; reference domain.Round.CollectedFees and
UpsertRoundParams.CollectedFees when implementing the check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/admin_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (25)
api-spec/openapi/swagger/ark/v1/admin.openapi.jsonapi-spec/protobuf/ark/v1/admin.protointernal/core/application/admin.gointernal/core/application/alert.gointernal/core/application/service.gointernal/core/domain/round.gointernal/core/domain/round_event.gointernal/core/domain/round_repo.gointernal/core/domain/round_test.gointernal/infrastructure/db/badger/ark_repo.gointernal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.down.sqlinternal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/postgres/round_repo.gointernal/infrastructure/db/postgres/sqlc/queries/models.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.down.sqlinternal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/sqlite/round_repo.gointernal/infrastructure/db/sqlite/sqlc/queries/models.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/interface/grpc/handlers/adminservice.gointernal/interface/grpc/permissions/permissions.go
internal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sql
Outdated
Show resolved
Hide resolved
internal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sql
Outdated
Show resolved
Hide resolved
…round exclusion, type safety
Collaborator
Author
|
@arkanaai review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/infrastructure/db/sqlite/round_repo.go (1)
574-574:⚠️ Potential issue | 🟠 MajorGuard negative DB values before
int64 -> uint64conversion inrowsToRounds.At Line 574,
uint64(v.round.CollectedFees)can wrap negative DB values into very large positive numbers, silently corrupting round data downstream.🛠️ Suggested fix
@@ round, ok = rounds[v.round.ID] if !ok { + if v.round.CollectedFees < 0 { + return nil, fmt.Errorf( + "data integrity issue: negative collected_fees %d for round %s", + v.round.CollectedFees, v.round.ID, + ) + } round = &domain.Round{ @@ - CollectedFees: uint64(v.round.CollectedFees), + CollectedFees: uint64(v.round.CollectedFees), FailReason: v.round.FailReason.String, } }Use this read-only script to verify schema constraints and remaining conversion sites:
#!/bin/bash set -euo pipefail echo "== collected_fees in SQL schema/migrations ==" fd -e sql | while read -r f; do rg -n "collected_fees|CREATE TABLE[[:space:]]+round|CHECK[[:space:]]*\\(" "$f" -C1 || true done echo echo "== CollectedFees conversions in sqlite round repo ==" rg -n "int64\\(round\\.CollectedFees\\)|uint64\\(v\\.round\\.CollectedFees\\)|CollectedFees:" internal/infrastructure/db/sqlite/round_repo.go -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/sqlite/round_repo.go` at line 574, In rowsToRounds, guard against negative DB values before converting int64 -> uint64 for CollectedFees: check v.round.CollectedFees for < 0 and handle it (reject the row or return an error) instead of blindly doing uint64(v.round.CollectedFees); update the conversion site (uint64(v.round.CollectedFees)) to perform the check and either return a descriptive error from rowsToRounds or clamp to 0 if that policy is acceptable, and add a test exercising negative CollectedFees to ensure the new behavior is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client-lib/client/grpc/client.go`:
- Around line 271-278: The send closure currently drops events when ctx.Done()
is closed which can lose terminal errors; change send (and its callers using
eventsCh) to ensure best-effort delivery of terminal Err events by: when
ctx.Done() is selected, if ev represents a terminal/error event, block (or retry
with a short timeout/backoff) until eventsCh accepts it instead of returning
false; for non-terminal events you may keep the non-blocking behavior. Update
the send closure and the call sites that close the stream/channel to use this
ensured-delivery behavior so terminal client.BatchEvent (Err) cannot be lost,
referencing the send function, eventsCh, ctx.Done() and the terminal/error
BatchEvent type.
---
Duplicate comments:
In `@internal/infrastructure/db/sqlite/round_repo.go`:
- Line 574: In rowsToRounds, guard against negative DB values before converting
int64 -> uint64 for CollectedFees: check v.round.CollectedFees for < 0 and
handle it (reject the row or return an error) instead of blindly doing
uint64(v.round.CollectedFees); update the conversion site
(uint64(v.round.CollectedFees)) to perform the check and either return a
descriptive error from rowsToRounds or clamp to 0 if that policy is acceptable,
and add a test exercising negative CollectedFees to ensure the new behavior is
enforced.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sumpkg/arkd-wallet/go.sumis excluded by!**/*.sumpkg/kvdb/go.sumis excluded by!**/*.sumpkg/macaroons/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/unit.yamlMakefileapi-spec/openapi/swagger/ark/v1/admin.openapi.jsonapi-spec/protobuf/ark/v1/admin.protogo.modinternal/core/application/admin.gointernal/core/application/alert.gointernal/core/application/alert_test.gointernal/core/application/service.gointernal/core/domain/round_test.gointernal/infrastructure/db/badger/ark_repo.gointernal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/postgres/round_repo.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/sqlite/round_repo.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/test/e2e/e2e_test.gointernal/test/e2e/utils_test.gopkg/arkd-wallet/go.modpkg/client-lib/client/grpc/client.gopkg/kvdb/go.modpkg/macaroons/go.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/core/application/service.go
- internal/infrastructure/db/service_test.go
- internal/infrastructure/db/postgres/round_repo.go
- internal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sql
- internal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sql
- api-spec/openapi/swagger/ark/v1/admin.openapi.json
- internal/core/application/alert.go
bitcoin-coder-bob
force-pushed
the
bob/persist-collected-fees
branch
from
23bcb22 to
bd4b66a
Compare
louisinger
requested changes
Mar 3, 2026
| uses: securego/gosec@master | ||
| with: | ||
| args: '-severity high -quiet -exclude=G115 ./...' | ||
| args: '-severity high -quiet -exclude=G115,G118 ./...' |
Collaborator
There was a problem hiding this comment.
Suggested change
| args: '-severity high -quiet -exclude=G115,G118 ./...' | |
| args: '-severity high -quiet -exclude=G115 ./...' |
isn't fixed by master ?
Collaborator
Author
There was a problem hiding this comment.
did something change on master? I was having tests timeout without this
| get: "/v1/admin/liquidity/recoverable" | ||
| }; | ||
| } | ||
| rpc GetCollectedFees(GetCollectedFeesRequest) returns (GetCollectedFeesResponse) { |
Collaborator
Collaborator
Author
There was a problem hiding this comment.
was asked in the issue: Expose via admin gRPC API for revenue tracking
internal/core/application/alert.go
Outdated
Comment on lines
+142
to
+144
| if len(input.TaprootLeafScript) > 0 { | ||
| boardingInputAmount += uint64(input.WitnessUtxo.Value) | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if len(input.TaprootLeafScript) > 0 { | |
| boardingInputAmount += uint64(input.WitnessUtxo.Value) | |
| } | |
| // TODO fragile, it may fail if arkd-wallet uses TaprootLeafScript in the future | |
| if len(input.TaprootLeafScript) > 0 { | |
| boardingInputAmount += uint64(input.WitnessUtxo.Value) | |
| } |
Collaborator
There was a problem hiding this comment.
that's something we do everywhere, it's ok now but let's add TODO so we don't forget it's not ideal if arkd-wallet becomes a tapscript wallet
internal/core/application/alert.go
Outdated
| } | ||
|
|
||
| // calculateCollectedFees computes the total fees (sats) collected by the coordinator for a given round. | ||
| func calculateCollectedFees(round *domain.Round, boardingInputAmount uint64) uint64 { |
Collaborator
There was a problem hiding this comment.
this file is more for ports.Alerts methods, let's move the fee funcs to utils.go ?
internal/core/domain/round_event.go
Outdated
| RoundEvent | ||
| ForfeitTxs []ForfeitTx | ||
| FinalCommitmentTx string | ||
| CollectedFees uint64 |
Collaborator
There was a problem hiding this comment.
Suggested change
| CollectedFees uint64 | |
| Fees uint64 |
nit: Fees are always "collected" from arkd point of view no ?
Collaborator
Author
There was a problem hiding this comment.
only argument against that is to distinguish it from on chain fees?
| @@ -0,0 +1 @@ | |||
| ALTER TABLE round ADD COLUMN collected_fees BIGINT NOT NULL DEFAULT 0 CHECK (collected_fees >= 0); | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| ALTER TABLE round ADD COLUMN collected_fees BIGINT NOT NULL DEFAULT 0 CHECK (collected_fees >= 0); | |
| ALTER TABLE round ADD COLUMN fees BIGINT NOT NULL DEFAULT 0 CHECK (collected_fees >= 0); |
nit
| Version uint | ||
| Swept bool | ||
| VtxoTreeExpiration int64 | ||
| CollectedFees uint64 |
Collaborator
There was a problem hiding this comment.
should be big.Int to handle overflow
Collaborator
Author
There was a problem hiding this comment.
given bitcoins supply cap is this even a concern? I think adding this type adds complexity for all the places we then end up having to use it.
Comment on lines
+4230
to
+4246
| faucetOnchain(t, aliceBoardingAddr, 0.001) | ||
| time.Sleep(6 * time.Second) | ||
|
|
||
| wg := &sync.WaitGroup{} | ||
| wg.Add(2) | ||
|
|
||
| var incomingErr error | ||
| go func() { | ||
| _, incomingErr = alice.NotifyIncomingFunds(ctx, aliceOffchainAddr) | ||
| wg.Done() | ||
| }() | ||
|
|
||
| var settleErr error | ||
| go func() { | ||
| _, settleErr = alice.Settle(ctx) | ||
| wg.Done() | ||
| }() |
Collaborator
There was a problem hiding this comment.
let's add another participant to that batch, doing a renewal (not boarding). to verify the fees equal the sum and check offchain settlement are also included
| defer close(eventsCh) | ||
| backoffDelay := utils.GrpcReconnectConfig.InitialDelay | ||
|
|
||
| send := func(ev client.BatchEventChannel) bool { |
Collaborator
There was a problem hiding this comment.
can we open a dedicated PR for this ? it sounds like an enhancement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/infrastructure/db/postgres/round_repo.go (1)
508-508:⚠️ Potential issue | 🟠 MajorGuard row-level fee conversion before casting to
uint64.
uint64(v.round.Fees)can silently wrap if a negative value is ever present in storage. Return an error instead of coercing.🛠️ Suggested fix
func rowsToRounds(rows []combinedRow) ([]*domain.Round, error) { rounds := make(map[string]*domain.Round) for _, v := range rows { var round *domain.Round var ok bool round, ok = rounds[v.round.ID] if !ok { + if v.round.Fees < 0 { + return nil, fmt.Errorf( + "data integrity issue: negative collected_fees %d for round %s", + v.round.Fees, v.round.ID, + ) + } round = &domain.Round{ Id: v.round.ID, StartingTimestamp: v.round.StartingTimestamp, EndingTimestamp: v.round.EndingTimestamp, @@ - CollectedFees: uint64(v.round.Fees), + CollectedFees: uint64(v.round.Fees), FailReason: v.round.FailReason.String, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/postgres/round_repo.go` at line 508, The code currently casts v.round.Fees to uint64 for CollectedFees which will silently wrap on negative values; update the mapping where CollectedFees is set (reference v.round.Fees and CollectedFees) to first check if v.round.Fees < 0 and if so return an error (with a clear message like "negative fee value in DB") instead of coercing, otherwise safely cast to uint64; ensure the function that performs this mapping returns the error up the call chain so negative stored fees are surfaced rather than wrapped.internal/infrastructure/db/sqlite/round_repo.go (1)
574-574:⚠️ Potential issue | 🟠 MajorProtect row mapping from signed→unsigned wrap.
uint64(v.round.Fees)can produce corrupted large values ifv.round.Feesis negative. Fail fast with an explicit error.🛠️ Suggested fix
func rowsToRounds(rows []combinedRow) ([]*domain.Round, error) { rounds := make(map[string]*domain.Round) for _, v := range rows { var round *domain.Round var ok bool round, ok = rounds[v.round.ID] if !ok { + if v.round.Fees < 0 { + return nil, fmt.Errorf( + "data integrity issue: negative collected_fees %d for round %s", + v.round.Fees, v.round.ID, + ) + } round = &domain.Round{ Id: v.round.ID, @@ - CollectedFees: uint64(v.round.Fees), + CollectedFees: uint64(v.round.Fees), FailReason: v.round.FailReason.String, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/sqlite/round_repo.go` at line 574, The assignment CollectedFees: uint64(v.round.Fees) can wrap if v.round.Fees is negative; update the mapping (the code that sets CollectedFees from v.round.Fees) to check if v.round.Fees < 0 and return a clear error (or propagate one) before converting to uint64, e.g., validate v.round.Fees and fail fast with an explicit error message referencing the offending round ID or context instead of silently casting.
🧹 Nitpick comments (1)
internal/core/application/utils.go (1)
593-596: Log the anomaloustotalOut > totalInclamp path.Right now negative-fee scenarios are silently converted to zero. Emitting a warning only for
totalOut > totalInwould make accounting anomalies diagnosable without changing behavior.♻️ Suggested tweak
- if totalOut >= totalIn { + if totalOut > totalIn { + log.WithFields(log.Fields{ + "round_id": round.Id, + "total_in": totalIn, + "total_out": totalOut, + }).Warn("calculated negative collected fees; clamping to zero") + return 0 + } + if totalOut == totalIn { return 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/utils.go` around lines 593 - 596, Inside the function that currently clamps negative fees using the totalOut and totalIn variables, add a warning log when the anomalous path totalOut > totalIn is taken: keep the existing behavior of returning 0 when totalOut >= totalIn, but before returning, if totalOut > totalIn emit a warning including the totalOut and totalIn values (using the module's logger/Logger instance) so the anomaly is recorded, then return 0; otherwise return totalIn - totalOut as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/test/e2e/e2e_test.go`:
- Around line 4818-4828: The test currently treats select timeouts on wgDone and
sentinelDone as mere logs which allows leaked goroutines to go unnoticed; change
both timeout branches to fail the test (e.g., call t.Fatalf or t.Fatalf-like
helper) so the test stops and reports a failure when the wait exceeds the
timeout, and apply the same change to the other occurrences mentioned (the block
around wgDone/sentinelDone at lines ~5185-5195) so all timeout paths fail rather
than only logging.
---
Duplicate comments:
In `@internal/infrastructure/db/postgres/round_repo.go`:
- Line 508: The code currently casts v.round.Fees to uint64 for CollectedFees
which will silently wrap on negative values; update the mapping where
CollectedFees is set (reference v.round.Fees and CollectedFees) to first check
if v.round.Fees < 0 and if so return an error (with a clear message like
"negative fee value in DB") instead of coercing, otherwise safely cast to
uint64; ensure the function that performs this mapping returns the error up the
call chain so negative stored fees are surfaced rather than wrapped.
In `@internal/infrastructure/db/sqlite/round_repo.go`:
- Line 574: The assignment CollectedFees: uint64(v.round.Fees) can wrap if
v.round.Fees is negative; update the mapping (the code that sets CollectedFees
from v.round.Fees) to check if v.round.Fees < 0 and return a clear error (or
propagate one) before converting to uint64, e.g., validate v.round.Fees and fail
fast with an explicit error message referencing the offending round ID or
context instead of silently casting.
---
Nitpick comments:
In `@internal/core/application/utils.go`:
- Around line 593-596: Inside the function that currently clamps negative fees
using the totalOut and totalIn variables, add a warning log when the anomalous
path totalOut > totalIn is taken: keep the existing behavior of returning 0 when
totalOut >= totalIn, but before returning, if totalOut > totalIn emit a warning
including the totalOut and totalIn values (using the module's logger/Logger
instance) so the anomaly is recorded, then return 0; otherwise return totalIn -
totalOut as before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
internal/core/application/alert.gointernal/core/application/utils.gointernal/core/domain/round.gointernal/core/domain/round_event.gointernal/core/domain/round_test.gointernal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.down.sqlinternal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/postgres/round_repo.gointernal/infrastructure/db/postgres/sqlc/queries/models.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.down.sqlinternal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sqlinternal/infrastructure/db/sqlite/round_repo.gointernal/infrastructure/db/sqlite/sqlc/queries/models.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/test/e2e/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.down.sql
- internal/infrastructure/db/postgres/sqlc/queries/models.go
- internal/core/domain/round_event.go
- internal/infrastructure/db/postgres/migration/20260225000000_add_collected_fees.up.sql
- internal/core/domain/round_test.go
- internal/infrastructure/db/sqlite/migration/20260225000000_add_collected_fees.up.sql
Comment on lines
+4818
to
+4828
| select { | ||
| case <-wgDone: | ||
| case <-time.After(10 * time.Second): | ||
| t.Log("churn/producer goroutines did not exit within 10s") | ||
| } | ||
|
|
||
| select { | ||
| case <-sentinelDone: | ||
| case <-time.After(5 * time.Second): | ||
| t.Log("sentinel goroutine did not exit within 5s") | ||
| } |
Contributor
There was a problem hiding this comment.
Timeout paths should fail, not only log.
When wait timeouts trigger, the test currently continues and can pass with leaked goroutines, masking real shutdown regressions.
🛠️ Suggested fix
- case <-time.After(10 * time.Second):
- t.Log("churn/producer goroutines did not exit within 10s")
+ case <-time.After(10 * time.Second):
+ t.Fatalf("churn/producer goroutines did not exit within 10s")
@@
- case <-time.After(5 * time.Second):
- t.Log("sentinel goroutine did not exit within 5s")
+ case <-time.After(5 * time.Second):
+ t.Fatalf("sentinel goroutine did not exit within 5s")Also applies to: 5185-5195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/test/e2e/e2e_test.go` around lines 4818 - 4828, The test currently
treats select timeouts on wgDone and sentinelDone as mere logs which allows
leaked goroutines to go unnoticed; change both timeout branches to fail the test
(e.g., call t.Fatalf or t.Fatalf-like helper) so the test stops and reports a
failure when the wait exceeds the timeout, and apply the same change to the
other occurrences mentioned (the block around wgDone/sentinelDone at lines
~5185-5195) so all timeout paths fail rather than only logging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #859
Add GetCollectedFees admin endpoint (GET /v1/admin/fees/collected) that returns aggregate collected fees across batches, with optional time-range filtering (after/before unix timestamps)
a
calculateCollectedFeesfunction that correctly accounts for boarding input amounts (previously fees were accumulated per-intent without including boarding inputs, leading to incorrect values I believe)Bug breakdown:
prior: we were adding the boding input amount for each intent.
now: we add the boardingInput amount one time
fix boarding input detection: the
BoardingInputAmountis now computed from the PSBT taproot leaf scripts rather than being passed incorrectlyGetCollectedFeesfunction added to round repo. Queries db usingSelectCollectedFeeswhich is a new query, that runs on theroundtable.func (r *Round) EndFinalizationininternal/core/domain/round.gomodified to take in a new paramcollectedFees uint64and is added as a new field into theRoundFinalizedstruct.The
Roundstruct also has a newCollected Fees uint64field which is leveraged in theGetCollectedFeesfunctions.e2e_test.goadditions for hitting new admin endpoint.alert_test.goadditions for testing againstcalculateCollectedFees.NOTE: All fees are expressed in sats.
I found I had to increase the timeout in the Makefile's
integrationtestand needed to bump theotelpackage version due to a CVE in our version used causing CI to failSummary by CodeRabbit
New Features
Improvements
Tests