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)
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
…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
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
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.
louisinger
reviewed
Mar 13, 2026
louisinger
reviewed
Mar 13, 2026
Comment on lines
+81
to
+83
| if round.CollectedFees > uint64(math.MaxInt64) { | ||
| return fmt.Errorf("collected_fees %d overflows int64", round.CollectedFees) | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if round.CollectedFees > uint64(math.MaxInt64) { | |
| return fmt.Errorf("collected_fees %d overflows int64", round.CollectedFees) | |
| } |
we need to find a way to remove this. I understand u cast to int64 later to set UpsertRound params but in theory the column is BIGINT so should not be int64 ? sqlc generated code problem or missing config maybe ?
but having BIGINT column, uint64 domain field but int64 integer limit is confusing
Collaborator
Author
There was a problem hiding this comment.
ok I think I have to change CollectedFees / Fees from uint64 to int64 across the entire stack then. Removes confusion (in theory) but at the cost of a relatively large repo wide change. Attempted to do this in commit: c2c3447
louisinger
reviewed
Mar 13, 2026
| if fees < 0 { | ||
| return 0, fmt.Errorf("data integrity issue: got negative collected_fees %d", fees) | ||
| } | ||
| return uint64(fees), nil |
Collaborator
There was a problem hiding this comment.
column is BIGINT but return int64. I think we encountered this for asset supply column, worth double checking how we handle this there
Collaborator
Author
There was a problem hiding this comment.
heres how we do in internal/infrastructure/db/postgres/asset_repo.go:
arkd/internal/infrastructure/db/postgres/asset_repo.go
Lines 140 to 143 in 9ac7220
so appears we are getting back supply as a string from the query then we parse it. That approach completely avoids the int64 boundary but is much heavier IMO.
louisinger
reviewed
Mar 13, 2026
Comment on lines
+81
to
+83
| if round.CollectedFees > uint64(math.MaxInt64) { | ||
| return fmt.Errorf("collected_fees %d overflows int64", round.CollectedFees) | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if round.CollectedFees > uint64(math.MaxInt64) { | |
| return fmt.Errorf("collected_fees %d overflows int64", round.CollectedFees) | |
| } |
louisinger
reviewed
Mar 13, 2026
| @@ -0,0 +1 @@ | |||
| ALTER TABLE round ADD COLUMN fees INTEGER NOT NULL DEFAULT 0 CHECK (fees >= 0); | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| ALTER TABLE round ADD COLUMN fees INTEGER NOT NULL DEFAULT 0 CHECK (fees >= 0); | |
| ALTER TABLE round ADD COLUMN fees REAL NOT NULL DEFAULT 0 CHECK (fees >= 0); |
"REAL" is BIGINT in sqlite (for consistency with pg migration)
louisinger
requested changes
Mar 13, 2026
Contributor
|
Hey @bitcoin-coder-bob — friendly nudge 👋 It's been 6 days since @louisinger requested changes on this PR. Any updates or blockers? If the feedback needs clarification, now's a good time to loop back in. |
Contributor
|
Correction to my previous nudge: @bitcoin-coder-bob already addressed @louisinger's review feedback on March 13th (commits d0458d8 through cb215ff). The nudge was sent in error — the review cycle is waiting on @louisinger for a re-review, not on bob. Sorry for the noise 🙏 |
Contributor
🔍 Arkana PR ReviewPR: arkd#933 — Persist collected fees SummaryAdds a new admin API endpoint ArchitectureKey Findings✅ Fee calculation bugfix is correct. The old code added ✅ DB migration is clean. ✅ SQL query is correct. ✅ Unit tests cover the fee calculation edge cases including the boarding-counted-once invariant and the underflow guard.
✅ E2e test hits the new admin endpoint and verifies round-trip fee persistence. ✅ Permissions — the new endpoint is properly wired in ✅ Cross-repo impact: This adds a new admin API surface. SDK consumers (ts-sdk, rust-sdk, etc.) will need to add client bindings if they want to expose this to users. No breaking changes to existing APIs. Security
Verdict: LGTM — solid implementation. The boarding input detection fragility is acknowledged and acceptable for now. The fee calculation bugfix is the most valuable part of this PR. |
Contributor
|
👋 @bitcoin-coder-bob — nudging on this one. @louisinger has outstanding change requests and the last commit was about 8 days ago. Any blockers? Would be great to get this moving. |
Contributor
|
Hey @bitcoin-coder-bob — this one has changes requested from @louisinger (last requested Mar 13). The last code update was before then. Are the requested changes addressed, or still in progress? |
Contributor
|
Hey @bitcoin-coder-bob — @louisinger requested changes on this on March 13th. Looks like you responded to the comments the same day, but no new commits since. Is this ready for a re-review, or still in progress? |
Contributor
|
Changes were requested 6+ days ago. @bitcoin-coder-bob need any help addressing the feedback? |
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