Skip to content

[codex] add Ark receive and transfer status CLI - #451

Draft
bhandras wants to merge 1 commit into
mainfrom
codex/ark-transfer-status
Draft

[codex] add Ark receive and transfer status CLI#451
bhandras wants to merge 1 commit into
mainfrom
codex/ark-transfer-status

Conversation

@bhandras

@bhandras bhandras commented May 15, 2026

Copy link
Copy Markdown
Member

Summary

This PR makes the Ark CLI receive/status story usable without forcing users to know which lower-level protocol path they are exercising or to inspect daemon internals.

Today the CLI can create a boarding address, but there is no friendly generic receive command and no single place to inspect in-round/OOR transfer activity. That makes quick local testing awkward: users can generate a receive script, but then have to infer what happened by stitching together other command output.

This adds:

  • darepo receive as the friendly default receive target command.
  • darepo receive list to show locally known reusable receive/boarding targets.
  • darepo transfers list to show in-round and OOR sends/receives with status.
  • filters for transfer mode, direction, status, limit, and offset.
  • daemon RPCs backing those CLI surfaces: ListReceiveScripts and ListTransfers.
  • a bech32m address in NewReceiveScriptResponse, so callers no longer need to derive it from the returned script.

User-Facing Examples

Create a generic Ark receive target:

darepocli receive

The command uses the same receive-script machinery as OOR receive target creation, but exposes it under the simpler command name users expect when they just want an address to hand out.

Create the same kind of receive target through the existing OOR namespace:

darepocli oor receive

Both commands return the pkScript plus the derived bech32m address. Existing scripts that call oor receive keep working, while new local-testing flows can use receive directly.

List receive targets this wallet/daemon knows about:

darepocli receive list

This is useful after a test session where multiple boarding/receive addresses were generated and you want to recover the exact script/address pairs without digging through logs.

List all transfer activity:

darepocli transfers list

List only pending in-round activity:

darepocli transfers list --mode inround --status pending

List only completed OOR receives:

darepocli transfers list --mode oor --direction incoming --status completed

Page through a larger history:

darepocli transfers list --limit 25 --offset 25

The transfer rows include stable identifiers, mode, direction, status, amount, timestamps where available, and context such as txid/round ID/error text depending on the source record.

API Changes

  • NewReceiveScriptResponse now includes address, derived from the taproot pkScript and the active network parameters.
  • ListReceiveScripts returns the local receive targets indexed by the daemon.
  • ListTransfers returns a normalized view over:
    • pending non-terminal in-round operations from round status tracking,
    • completed/failed in-round rows from transaction history,
    • OOR send/receive sessions from OOR session tracking.

The transfer list intentionally normalizes these sources into one response so CLI callers do not need separate commands for “in-round send”, “in-round receive”, “OOR send”, and “OOR receive” just to answer “what happened?”

Current Limitations

ListTransfers aggregates several existing status/history sources in memory and then applies the public limit/offset page. That is expected to be fine for the small local-wallet histories this CLI is aimed at today, but it is not yet a database-level merged cursor for very large archival wallets. The code carries a TODO to replace the full scan with source-aware cursors if transfer history grows enough to need that.

Some rows can only expose the data available from their source. Live in-round rows may have unknown direction until local input/output hints or persisted ledger rows exist. Direction filters keep those unknown live rows visible rather than hiding pending activity behind --direction incoming or --direction outgoing. OOR receive/session rows can also report amount_sat = 0 when the local status source does not yet know the final amount. The CLI still shows these rows so users can track lifecycle status instead of losing pending activity entirely.

Schema Registry

The method registry now includes schemas for:

  • receive
  • receive.list
  • transfers.list

It also refreshes the stale send.oor schema so it advertises the actual destination inputs: exactly one of to or pubkey. The removed pk_script schema entry was not accepted by the command parser, so this is a registry correction rather than a daemon/API removal.

Validation

  • go test ./...
  • make fmt-changed
  • make fmt-changed-check
  • make lint-native
  • make commitmsg-lint range="origin/main..HEAD"
  • make schema-check
  • make doc-check (passes; reports pre-existing CLAUDE.md/AGENTS.md divergence warnings in baselib/actor and db/actordelivery/migrations)
  • git diff --check

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces new CLI commands and RPC endpoints for listing Ark receive targets and transfer statuses. Key additions include the receive list and transfers list commands, supported by server-side logic that aggregates transfer data from rounds, OOR sessions, and transaction history. Feedback identifies several critical issues: the collectTransfers method uses a hardcoded limit that breaks pagination for large histories, and an integer conversion of the offset could cause panics on 32-bit systems. Additionally, it is recommended to use the command context for RPC calls to ensure proper cancellation, and to improve the uniqueness of the TransferId for temporary rounds to avoid collisions.

Comment thread darepod/rpc_transfers.go Outdated
Comment on lines +83 to +102
pending, err := r.pendingRoundTransfers(ctx)
if err != nil {
return nil, err
}

transfers = append(transfers, pending...)

history, err := r.historyTransfers(
ctx, "round",
daemonrpc.TransferMode_TRANSFER_MODE_INROUND,
)
if err != nil {
return nil, err
}

transfers = append(transfers, history...)
}

if transferModeAllowed(req, daemonrpc.TransferMode_TRANSFER_MODE_OOR) {
oorTransfers, err := r.oorTransfers(ctx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The collectTransfers method uses a hardcoded limit of maxListTransfersLimit (1000) when fetching from underlying sources (ListRounds, ListTransactions). This effectively caps the total number of transfers that can be listed and searched, regardless of the offset provided in the ListTransfersRequest. If a user has more than 1000 history entries, older entries will be unreachable via this RPC. Additionally, filtering happens in-memory after this truncation, which may lead to incomplete results if matching entries exist beyond the first 1000. Consider fetching a larger window or implementing a more robust pagination strategy that accounts for the requested offset and limit across all sources.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and folded into 1ef001da. historyTransfers now walks ListTransactions source pages until exhaustion, and pendingRoundTransfers walks ListRounds cursor pages instead of truncating at the first page.

Comment thread darepod/rpc_transfers.go Outdated
Comment on lines +49 to +52
start := int(req.GetOffset())
if start > len(transfers) {
start = len(transfers)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On 32-bit systems, converting a uint32 offset to int can overflow if the offset is greater than math.MaxInt32. This would result in a negative start value, causing a panic when slicing the transfers array. It is safer to compare the uint32 offset with the length of the slice before conversion.

Suggested change
start := int(req.GetOffset())
if start > len(transfers) {
start = len(transfers)
}
offset := req.GetOffset()
if offset > uint32(len(transfers)) {
offset = uint32(len(transfers))
}
start := int(offset)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and folded into 1ef001da. ListTransfers now converts the uint32 cursor through clampedTransferStart, which compares using uint64 before converting to int.

return err
}

resp, err := client.ListReceiveScripts(context.Background(), req)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use cmd.Context() instead of context.Background() to ensure the RPC call is properly canceled if the user interrupts the CLI command (e.g., via Ctrl+C).

Suggested change
resp, err := client.ListReceiveScripts(context.Background(), req)
resp, err := client.ListReceiveScripts(cmd.Context(), req)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and folded into 1ef001da. The new receive-list RPC call now uses cmd.Context() so CLI cancellation propagates.

return err
}

resp, err := client.ListTransfers(context.Background(), req)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use cmd.Context() instead of context.Background() to ensure the RPC call is properly canceled if the user interrupts the CLI command.

Suggested change
resp, err := client.ListTransfers(context.Background(), req)
resp, err := client.ListTransfers(cmd.Context(), req)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and folded into 1ef001da. The transfer-list RPC call now uses cmd.Context() so CLI cancellation propagates.

Comment thread darepod/rpc_transfers.go Outdated
Comment on lines +206 to +207
id = fmt.Sprintf("temp:%t:%s", round.GetIsTemp(),
round.GetState().String())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The generated TransferId for temporary rounds (where round_id is empty) is not guaranteed to be unique if multiple rounds are in the same state. This can lead to collisions in the TransferId field, which is intended to be a stable local identifier. Consider incorporating a more unique property, such as a local sequence number or the creation timestamp, to ensure uniqueness.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and folded into 1ef001da. Temporary round rows now use temporaryRoundTransferID, which includes round state, timestamps, tx/outpoint context, and a per-list index to avoid collisions before a server round id exists.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

PR: [codex] add Ark receive and transfer status CLI
Author: bhandras

Overview

This PR adds two new daemon RPCs (ListReceiveScripts, ListTransfers), matching CLI surfaces (darepo receive, darepo receive list, darepo transfers list), and backfills a bech32m address field onto NewReceiveScriptResponse. The generated protobuf code is cleanly regenerated. The approach of doing an in-memory merge/sort/paginate across three transfer sources (pending rounds, round history, OOR sessions) is pragmatic for now.


Bugs / Correctness

1. ListTransfers skips requireWalletReady()

ListReceiveScripts guards with r.requireWalletReady() before touching the indexer. ListTransfers calls r.ListRounds, r.ListTransactions, and r.listOORSessions without that check. If the wallet isn't ready those inner RPCs will each surface their own errors, but the guard is a single consistent early-return that should be applied uniformly.

// darepod/rpc_transfers.go – ListTransfers
func (r *RPCServer) ListTransfers(ctx context.Context, ...) {
    // missing: if err := r.requireWalletReady(); err != nil { return nil, err }

2. Silent data truncation before pagination

pendingRoundTransfers fetches at most maxListTransfersLimit (1000) rounds, and historyTransfers similarly caps at 1000 rows — both before direction/status filtering and pagination. If more than 1000 rows exist in any source, older entries are silently dropped and never surfaced. The caller's offset then pages over an already-incomplete dataset. For a first cut this may be acceptable, but the silent truncation should at minimum be surfaced via a has_more-equivalent on each source, or documented as a known limitation.

3. Direction is always UNSPECIFIED for pending round transfers

In transferFromRound the direction is hardcoded:

direction := daemonrpc.TransferDirection_TRANSFER_DIRECTION_UNSPECIFIED

This means a request with direction_filter = incoming will never match any pending in-round entry. If RoundInfo carries enough information to infer direction (e.g. from input_outpoints ownership), it should be populated. If not, that's worth a comment explaining why direction is unknowable for live rounds.


Design / Maintainability

4. newReceiveCmd reuses oorReceive as RunE via implicit flag coupling

// cmd/darepocli/darepoclicommands/cmd_receive.go
cmd := &cobra.Command{
    RunE: oorReceive,   // borrows OOR implementation
}
cmd.Flags().String("label", "", "...")

oorReceive is defined in a separate file and was written against a different command's flag set. If oorReceive accesses flags that exist on the OOR command but not on the new receive command, those reads silently return zero values. As the OOR command evolves, this will be a latent breakage. A thin wrapper that calls a shared newReceiveScriptRPC helper would make the coupling explicit.

5. Unusual make formatting

targets := make(
    []*daemonrpc.ReceiveTarget, 0,
    len(
        resp.GetScripts(),
    ),
)

len(resp.GetScripts()) is a simple inline expression; the multi-line split adds visual noise without helping readability. Per project style (function calls: closing ) on its own line when wrapping), this works syntactically, but the nested single-expression len(...) split is non-idiomatic. Same pattern recurs in pendingRoundTransfers and historyTransfers.


Minor Nits

6. Missing t.Parallel() in two unit tests

TestParseTransferFilters and TestParseTransferFiltersRejectUnknown in cmd_transfers_test.go don't call t.Parallel(), while the schema tests in the same file do. This is inconsistent and slows down the test run when the suite is parallelised.

7. transferFromTransaction sets UpdatedAtUnixS = CreatedAtUnixS

UpdatedAtUnixS: tx.GetCreatedAtUnixS(),   // no separate update time available

The intent is clear but a short comment would make the duplication intentional, not a copy-paste oversight.

8. fmt.Sprintf("temp:%t:...") for boolean

%v is idiomatic for booleans in Go; %t is correct but less commonly seen. Tiny nit.


What Looks Good

  • The three filter parser functions in cmd_transfers.go are clean and exhaustively tested, including the reject-unknown path.
  • addressFromTaprootPkScript is a well-scoped pure function with correct length and opcode checks, and the two unit tests cover both the happy path and the reject-non-taproot path.
  • Schema registry entries match CLI defaults exactly; the schema tests verify the mapping end-to-end.
  • Removal of pk_script from the send.oor schema is a correct cleanup with matching description updates on the remaining fields.
  • Proto field numbering and enum values are stable; the generated code bump-shifts all downstream message type indices correctly.

Summary

The blocking items are (1) the missing requireWalletReady guard and (3) the always-UNSPECIFIED direction for pending round transfers which breaks the direction filter for that source. Item (2) (silent truncation) is worth at minimum a comment or TODO. The rest are style/nit-level.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 5781f33 to 1ef001d Compare May 15, 2026 10:01
@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds a user-friendly darepo receive command, receive list, and transfers list CLI surfaces with matching daemon RPCs (ListReceiveScripts, ListTransfers). It also enriches NewReceiveScriptResponse with a pre-derived address field so callers no longer need to compute it themselves. The overall design is clean and the new proto schema is well-structured.


Bugs / Correctness

1. UpdatedAtUnixS set to CreatedAtUnixS in transferFromTransaction (darepod/rpc_transfers.go)

// current code
UpdatedAtUnixS: tx.GetCreatedAtUnixS(),

This field is intentionally set to the creation time (likely because history rows have no separate update time), but it means callers who inspect updated_at_unix_s see the same value as created_at_unix_s, which is misleading. If history rows genuinely have no update time, consider leaving it as zero and let transferSortTime fall through to created_at_unix_s — or add a comment explaining the intentional copy.

2. ListTransfers does not call requireWalletReady

ListReceiveScripts (correctly) gates on requireWalletReady() and returns a clean error if the wallet is not up. ListTransfers skips this check, so a pre-wallet-ready call will return a less user-friendly error from the downstream ListRounds / ListTransactions / listOORSessions calls. Add the guard at the top of ListTransfers, consistent with sibling RPCs.

3. In-round pending transfers always show TRANSFER_DIRECTION_UNSPECIFIED

transferFromRound hard-codes direction to UNSPECIFIED because RoundInfo does not carry a per-wallet direction field. This is probably unavoidable given the current data model, but it should be noted prominently: users running transfers list --direction incoming will never see pending in-round items, even if they are incoming. This could be surprising. Consider documenting this limitation in the proto comment for direction on TransferInfo, or adding a note to the CLI help text.


Performance Concern

ListTransfers loads all transfers into memory before paginating

collectTransfers issues unbounded fan-out calls to ListRounds (full-scan loop), ListTransactions (full-scan loop), and listOORSessions (full response). All results are accumulated into a single slice, sorted, and then sliced. For any wallet with significant history this means:

  • Response latency is proportional to total transfer history size, not the requested page size.
  • Memory usage spikes with each request.

The safe path for an initial landing is to cap the source fetches — e.g., only fetch limit rows from each source before merging, accepting that the sort across sources is best-effort. The current maxListTransfersLimit cap on the output does not bound the input work. A comment in the code acknowledging this tradeoff (and flagging it as a known limitation to be addressed with DB-side pagination later) would help future maintainers.


Style / Minor Issues

4. Unusual make formatting in ListReceiveScripts (darepod/rpc_receive_targets.go)

// current
targets := make(
    []*daemonrpc.ReceiveTarget, 0,
    len(
        resp.GetScripts(),
    ),
)

len(resp.GetScripts()) does not need its own wrapped call. Per the style guide, closing ) goes on its own line when wrapping, but not for a single-expression argument like this:

targets := make(
    []*daemonrpc.ReceiveTarget, 0, len(resp.GetScripts()),
)

5. Redundant mode filtering in collectTransfers

transferModeAllowed is checked before querying each source, and transferMatchesFilters rechecks the same mode afterward. The second check for mode is redundant (direction and status filtering still need it). No functional impact, but the redundancy may confuse future readers. A comment would clarify that transferMatchesFilters applies the full filter triple and the upfront transferModeAllowed guards are just source-level short-circuits.

6. historyType is an untyped string literal

history, err := r.historyTransfers(ctx, "round", ...)

If ListTransactions ever changes the accepted type strings, this silently returns empty results. A named constant would make the coupling explicit.


Test Coverage

The tests are well-structured and cover the key utility functions. A few gaps worth noting:

  • No test for requireWalletReady guard in ListTransfers (once added per item 2 above).
  • No test for the historyTransfers pagination safety guard (nextOffset <= offset). This guard prevents infinite loops on a misbehaving backend — it would be good to have a unit test that confirms a stale cursor halts iteration.
  • No test for transferStatusFromRoundState with ROUND_STATE_UNSPECIFIED, which currently falls through to TRANSFER_STATUS_PENDING. Depending on whether that state can occur in practice, it may warrant explicit handling or at least a test asserting the current behaviour.
  • The test for TestParseTransferFilters only checks one value per filter. It would be worth also asserting the "all"/"" passthrough maps to UNSPECIFIED, to catch regressions in the symmetric case.

Summary

The CLI and proto design is solid, and the utility functions (addressFromTaprootPkScript, the sort/pagination helpers) are correct and well-tested. The main issues are: a misleading updated_at_unix_s copy, a missing requireWalletReady guard, and the load-all-then-paginate approach that will cause latency and memory problems at scale. The direction limitation for pending in-round transfers should at minimum be documented. The style issues are minor.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 1ef001d to e17d5f0 Compare May 15, 2026 10:05

Copy link
Copy Markdown
Member Author

Addressed the first Claude review in e17d5f03:

  • added the missing requireWalletReady() guard to ListTransfers
  • changed transfer source collection to walk paginated ListRounds/ListTransactions sources instead of truncating at the first page
  • inferred pending round direction from persisted local input/output outpoints when available, with an explicit comment for live rounds where direction is not yet knowable
  • split the generic receive command through its own wrapper while sharing the NewReceiveScript helper with oor receive
  • cleaned up the formatting/test/context nits and kept the fixes folded into the single branch commit

Re-running Claude via the existing claude-review label/synchronize workflow now.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds three user-facing features that make the Ark CLI usable without requiring knowledge of protocol internals:

  • darepo receive — friendly alias for creating a receive target (delegates to the existing NewReceiveScript RPC)
  • darepo receive list — lists locally registered receive/boarding targets
  • darepo transfers list — normalized view over in-round and OOR transfer activity with mode/direction/status/pagination filters

The approach of stitching three backend sources (pending round FSM states, committed transaction history, OOR sessions) into one response is well-motivated and the shape of the API is clean. Generated code (daemon.pb.go, daemon_grpc.pb.go, daemon_mailboxrpc.pb.go) is correctly committed alongside the .proto changes.


Correctness

Round deduplication across sources

pendingRoundTransfers fetches non-terminal round states from ListRounds, then historyTransfers reads committed round history. If a round finalizes in the narrow window between these two calls, it can appear in both lists. The IDs differ (round:<ID> vs round:<ID>:<entryID>), so they won't silently collapse — but the caller will see the same round twice at different statuses. A comment acknowledging this TOCTOU is worth adding; whether deduplication is needed now depends on acceptable UX for the MVP.

Direction inference is fragile

transferDirectionFromTransaction uses substring matching to classify direction:

if strings.Contains(subtype, "received") ||
    tx.GetCreditAccount() == "transfers_in" {

If subtype strings are ever renamed or localized, direction silently falls back to UNSPECIFIED. An explicit switch over known subtype constants (or a dedicated direction field on the history entry) would be more robust. If the set of valid subtypes is small and stable, that's worth documenting explicitly here.

pendingRoundTransfers filter ordering

pendingRoundTransfers fetches all rounds regardless of the mode filter, then applies transferStatusFromRoundState and skips non-pending. The outer collectTransfers already guards this call with transferModeAllowed, so the per-source fetches are correctly suppressed. However, the inner function does no early-out on direction — all rounds are promoted to transfers and the direction filter is applied later in transferMatchesFilters. For correctness this is fine; for clarity a comment explaining why direction filtering is deferred would help readers.


Performance

All data is loaded before pagination

collectTransfers fetches every in-round and OOR record, sorts all of them, and then slices to the requested page. For a wallet with a large transaction history this is O(total history) per page request. For the current use case (local testing, developer tooling) this is acceptable, but it should be noted as a known limitation in a follow-up if this path ever becomes hot. An inline comment in collectTransfers noting this trade-off would help future readers.


Style

Over-wrapped make call in rpc_receive_targets.go

// current
targets := make(
    []*daemonrpc.ReceiveTarget, 0,
    len(
        resp.GetScripts(),
    ),
)

The inner len(resp.GetScripts()) wrapping is unnecessary. The project style guide wraps calls when the closing ) moves to its own line, but the argument itself shouldn't be split:

// suggested
targets := make(
    []*daemonrpc.ReceiveTarget, 0, len(resp.GetScripts()),
)

Empty validation closure in receiveList

if err := parseRequest(cmd, req, func() error {
    return nil
}); err != nil {

If parseRequest accepts no extra parsing work here, passing a no-op closure is noise. Either skip parseRequest entirely (call ListReceiveScripts directly as in the OOR variant) or, if parseRequest has side effects worth keeping, extract the no-op as a named variable or document the intent.

taprootPkScriptKeyOffset slice bound

key := pkScript[taprootPkScriptKeyOffset:]

The length check above guarantees exactly 34 bytes, so this always yields 32 bytes. An explicit end bound makes the intent self-documenting with no runtime cost:

key := pkScript[taprootPkScriptKeyOffset:taprootPkScriptLen]

Tests

Unit coverage of the helper functions is solid: direction/status converters, pagination helpers, temporaryRoundTransferID, clampedTransferStart, nextTransferOffset, and addressFromTaprootPkScript round-trip are all tested. The schema registry entries for the new commands are also verified.

What's missing:

  • Tests for collectTransfers with a stubbed RPCServer exercising the mode/direction/status filter combinations end-to-end (not just the per-converter unit tests).
  • A test for sortTransfersNewestFirst — the tie-breaking by TransferId is a non-obvious invariant that would benefit from an explicit test.
  • nextTransferOffset already has overflow testing; clampedTransferStart has offset-past-end testing — good catches.

Minor Notes

  • The context.Background()cmd.Context() fix in oorReceive is correct and worth keeping — good catch.
  • The send.oor schema correction (removing the stale pk_script param) is a clean housekeeping fix.
  • TransferInfo.transfer_id prefixing (round:, oor:, ledger:) is a thoughtful design that makes IDs source-typed and human-readable.
  • defaultListTransfersLimit = 100 vs maxListTransfersLimit = 1000: the CLI default of 50 (--limit 50) is lower than the RPC default of 100; this discrepancy is harmless but worth documenting.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from e17d5f0 to 5b45e3b Compare May 15, 2026 10:10

Copy link
Copy Markdown
Member Author

Folded the second Claude pass into 5b45e3b3 as well:

  • documented TransferInfo.direction and CLI direction filtering for live in-round rows whose direction is still unknown
  • added a code comment explaining the current in-memory aggregation tradeoff before the stores have a shared pagination model
  • replaced the raw "round" history type with a named constant
  • expanded parser tests for all/empty passthrough and added round-state status mapping coverage
  • kept the UpdatedAtUnixS = CreatedAtUnixS assignment documented as intentional for immutable history rows

Local gate is green again after regenerating protobuf stubs.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review: [codex] add Ark receive and transfer status CLI

Overall this is a well-scoped PR that delivers the stated UX goal clearly. The abstractions are clean, tests are present and parallel, and the context.Background()cmd.Context() fix in rpc_oor_receive.go is a good catch. A few things worth looking at:


Correctness / Logic

pendingRoundTransfers – confusing pagination with the persisted toggle

The loop initialises persisted = false, calls ListRounds with PersistedOnly: false on the first page, then sets persisted = true for subsequent pages. If the first call returns all results and nextToken == "", the flip never fires and everything is correct. But if pagination continues, pages 2+ only see persisted-only rounds. Live in-memory rounds on those pages would be silently skipped. The intent seems to be "start with live rounds then fall through to persisted" but that's not how a token-based cursor works — both stores need separate traversals, or the PersistedOnly flag shouldn't change mid-cursor.

// rpc_transfers.go – persisted flips mid-cursor, may skip live rounds
persisted = true  // set after first page; round-index also increments on skip

transferStatusFromRoundState treats all non-terminal states as PENDING

This is fine, but ROUND_STATE_CONFIRMED and ROUND_STATE_FAILED rounds will also pass through the ListRounds call in pendingRoundTransfers, be mapped to COMPLETED/FAILED, and then silently discarded by the status != PENDING filter. Those same rounds re-appear later via historyTransfers. There's no double-count today, but the logic is subtle. A comment explaining why confirmed/failed rounds from ListRounds are intentionally dropped (deferred to history) would help future readers.


Style / Conventions

Excessive make() nesting (rpc_receive_targets.go:3886):

targets := make(
    []*daemonrpc.ReceiveTarget, 0,
    len(
        resp.GetScripts(),
    ),
)

len(resp.GetScripts()) is not long enough to warrant its own lines. This fits on one line well within the 80-char limit.

No-op parseRequest in receiveList:

if err := parseRequest(cmd, req, func() error {
    return nil
}); err != nil {
    return err
}

ListReceiveScriptsRequest has no fields, so parseRequest adds no value here. The other commands in this file that do use it show what the pattern is for. Either omit it entirely or add a comment explaining why it's here (e.g. for future fields, or to satisfy the JSON-input path).

Missing Long description on receive list: every peer subcommand has both Short and LongnewReceiveListCmd only has Short. Minor, but inconsistent.


Performance

In-memory aggregation for ListTransfers

collectTransfers fetches all pages of rounds and transaction history into memory before filtering and paginating. The comment acknowledges this ("first implementation intentionally normalizes all matching local sources in memory"). Worth a brief // TODO noting the limit: once transfer history is large, this becomes an O(N) memory allocation per request. A future follow-up adding source-level limit/filter pushdown would improve this significantly.


Minor Observations

  • transferDirectionFromTransaction infers direction from magic strings ("received", "sent", "transfers_in", "transfers_out"). This works but ties correctness to undocumented internal ledger conventions. A constant or enum would make this more durable.
  • AmountSat is zero for all pending in-round transfers (the RoundInfo proto doesn't carry an amount). This is visible in the output and may confuse users. A // Note: amount is unavailable until the round is persisted comment on the field or a note in the CLI Long description would help.
  • The schema registry cleanup for send.oor (removing pk_script) is welcome, but it would help to reference where pk_script was actually removed from the implementation, so reviewers can verify the schema now matches the live code.

What's Good

  • context.Background()cmd.Context() is a correct and important fix.
  • addressFromTaprootPkScript validates script length and opcode before slicing — fails closed for unexpected input.
  • All tests use t.Parallel(), follow TestFoo naming, and cover both valid and invalid inputs.
  • The pagination cursor overflow protection in nextTransferOffset is a nice defensive touch.
  • Proto design is clean: UNSPECIFIED = 0 for all three enums means the zero-value is always "no filter", which is the right default.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 5b45e3b to d600f19 Compare May 15, 2026 10:14

Copy link
Copy Markdown
Member Author

Folded the remaining third-pass clarity nits into d600f196:

  • documented the possible round-status/history snapshot race while keeping source-prefixed transfer ids distinguishable
  • changed transaction direction inference to use ledger event/account constants instead of substring matching
  • removed the no-op receive list parse closure
  • made the taproot key slice bound explicit
  • added sort tie-break coverage for sortTransfersNewestFirst

Local full gate is green again: go test ./..., make lint-native, make fmt-changed-check, make commitmsg-lint range="origin/main..HEAD", make schema-check, make doc-check, and git diff --check.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds a user-facing receive and transfers list CLI surface over two new daemon RPCs (ListReceiveScripts, ListTransfers), plus an address field in NewReceiveScriptResponse. The overall design is clean and the code is well-tested. A few issues worth addressing before merge.


Issues

1. Bug: persisted flag changes mid-pagination in pendingRoundTransfers

In darepod/rpc_transfers.go the persisted variable starts false and flips to true after the first page. If ListRounds uses that flag to switch data sources independently of the page token, the second and later pages return a different dataset and rounds can be silently skipped. Either keep persisted constant throughout (relying on the page token to continue), or split the loop into two sequential walks: one for live rounds (PersistedOnly: false, single pass) and one for persisted rounds (PersistedOnly: true, paginated).

2. In-memory unbounded collection before pagination

collectTransfers exhausts every matching source store into memory before slicing for pagination. For a wallet with a long history, --limit 5 --offset 0 still forces a full scan of all stores. The in-code comment acknowledges this is a V1 tradeoff, which is fine, but a TODO would prevent it silently becoming a production performance problem as history grows.

3. Double-counting race window

The comment in collectTransfers notes: A round that completes between reads can briefly appear once as a pending round row and once as a committed history row. The transfer IDs differ (round: from the pending path vs round::<entry_id> from the history path), so they will not be deduplicated. A user running transfers list at exactly the wrong moment sees the same transfer twice with conflicting statuses. Is this an accepted known limitation, or should deduplication by round_id be applied before returning?

4. Style: awkward make formatting in ListReceiveScripts

In darepod/rpc_receive_targets.go the capacity argument to make is wrapped on its own line with len() on its own line inside. Project style puts simple expressions inline:

targets := make([]*daemonrpc.ReceiveTarget, 0, len(resp.GetScripts()))

Minor Notes

  • No amount for pending rounds: transferFromRound leaves AmountSat at zero because RoundInfo does not expose a per-wallet transfer amount for in-progress rounds. Users will see 0 for all pending in-round transfers. A comment explaining why would help readers understand this is intentional.
  • receive list has no pagination: ListReceiveScriptsRequest has no limit/offset and the CLI exposes none. Acceptable for V1 if registrations are expected to be short-lived, but a TODO would make the intent explicit.
  • send.oor schema removal of pk_script: Dropping the advertised pk_script param is a breaking change for any MCP or automation tooling relying on the schema. If the field was already unsupported at the proto level this is correct housekeeping -- confirming that is the case.

Positives

  • The context.Background() to cmd.Context() fix in cmd_oor.go is a welcome correctness improvement independent of the rest of the PR.
  • addressFromTaprootPkScript is properly defensive: validates both byte length and exact opcode pattern before slicing the key.
  • clampedTransferStart and nextTransferOffset handle uint32 overflow correctly and have matching tests.
  • The source-prefixed transfer ID scheme (round:, oor:, ledger:) makes entries stable and distinguishable across sources.
  • Test coverage is solid for the pure functions: filter parsing, enum mapping, sort ordering, pagination math, and address derivation are all directly tested.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from d600f19 to 5d6926e Compare May 15, 2026 10:18

Copy link
Copy Markdown
Member Author

Folded the fourth Claude pass into 5d6926ef:

  • refactored pending round collection into explicit live-state and persisted-round traversals so the PersistedOnly cursor behavior is no longer subtle
  • documented why terminal persisted round summaries are dropped in favor of transaction history rows
  • added the TODO on the current O(total history) in-memory aggregation path
  • added a Long description for receive list
  • aligned the CLI transfer limit default with the RPC default (100)
  • documented zero amount_sat for live in-round rows until history supplies the persisted amount

Full local gate is green again, including regenerated protobuf stubs.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds user-facing receive and transfers list CLI commands, two backing daemon RPCs (ListReceiveScripts, ListTransfers), and a bech32m address field on NewReceiveScriptResponse. The normalized transfer view stitches together live round state, persisted round history, and OOR sessions into a single paginated response. Overall the design is clean and well-structured.


Positives

  • The context.Background()cmd.Context() fix in oorReceive is a correct bug fix and the shared newReceiveScript helper properly deduplicates CLI logic.
  • addressFromTaprootPkScript validates length and opcodes before indexing, so it fails closed for non-taproot scripts.
  • clampedTransferStart and nextTransferOffset both guard against int-overflow and uint32-wraparound correctly.
  • temporaryRoundTransferID makes a reasonable best-effort at stable IDs for live rounds that have no server-assigned round id.
  • transferMatchesFilters nil-guards the transfer pointer, which is the right defensive posture.
  • Test names follow the TestFoo style; tests are parallel throughout.

Issues

1. Potential duplicate rows under concurrent in-round round transitions (pendingRoundTransfers)

pendingRoundTransfers fetches live in-memory rounds via queryRoundStates, then independently walks persisted rounds via ListRounds(PersistedOnly: true). Both fetches are unsynchronized. A round that completes (transitions PENDING → CONFIRMED) between the two reads will appear once in the live slice and once in the persisted slice, both with status == PENDING (the live snapshot) and status == COMPLETED (the history path). Because history is fetched separately in historyTransfers, the confirmed row might appear in both pendingRoundTransfers and historyTransfers for the duration of a single ListTransfers call, giving a user two rows for the same round.

There is no dedup step between the slices. The source-prefixed transfer_id (round:<id> vs round:<id>:<entry_id>) makes them distinguishable but doesn't remove them. Under the current --status pending filter path the duplicated history row would be filtered out, but callers using no filter or --status completed could see both.

Suggested fix: after collectTransfers, deduplicate on a key that is stable across sources (e.g., the bare round id extracted from the transfer_id prefix).

2. O(full history) memory scan on every request

historyTransfers walks all pages using maxListTransfersLimit = 1000 per page with no aggregate bound. pendingRoundTransfers also walks all persisted pending rounds. For wallets with large histories, every ListTransfers call will load the entire ledger into memory before paginating. The TODO comment documents this, which is good, but it's worth noting that the --limit and --offset flags give callers a false impression of bounded server-side cost.

No action required now if the TODO is tracked, but consider dropping the --offset parameter (or documenting it as client-side pagination) until server-side pagination is implemented, to avoid misleading callers.

3. OOR transfers always show amount_sat: 0

transferFromOORSession does not populate AmountSat. If OORSessionInfo carries an amount field this is a gap; if it doesn't the proto comment on TransferInfo.amount_sat should say OOR entries may be zero. Either way the CLI output could confuse users who see an empty amount for OOR sends.

4. Formatting nit in ListReceiveScripts (rpc_receive_targets.go)

targets := make(
    []*daemonrpc.ReceiveTarget, 0,
    len(
        resp.GetScripts(),
    ),
)

len(resp.GetScripts()) fits on one line. The style guide wraps when lines would exceed ~80 characters; wrapping len(...) alone into three lines is unnecessary and reads oddly. Prefer:

targets := make(
    []*daemonrpc.ReceiveTarget, 0, len(resp.GetScripts()),
)

5. send.oor schema breaks the pk_script parameter

The schema registry update removes pk_script from the send.oor schema. Any MCP agent or script that passes pk_script today will now get a schema-validation error. This is likely intentional to match what the daemon actually accepts, but the PR description frames it only as refreshing a stale schema. If any callers depend on pk_script, this is a breaking change for the MCP surface.

6. ListReceiveScripts returns codes.Internal for uninitialized indexer

if r.server.indexer == nil {
    return nil, status.Error(
        codes.Internal, "indexer client not initialized",
    )
}

codes.Internal is reserved for internal bugs (as per the project's error-level policy). An uninitialized indexer is a configuration/precondition failure; codes.FailedPrecondition or codes.Unavailable would be more accurate here and give callers a better retry signal.


Minor Notes

  • TestParseTransferFilters tests "" and "all" for each filter but doesn't exercise "oor", "outgoing", "failed", or "pending" in the positive path — only "inround", "incoming", and "completed" get positive tests. The error-path test covers the gap partially, but adding one positive case per remaining value would make coverage exhaustive.
  • TestTransferDirectionFromRound and TestTransferMatchesFilters use flat require assertions rather than table-driven subtests, unlike the adjacent TestTransferDirectionFromTransaction. Minor consistency gap.
  • transferStatusFromRoundState maps ROUND_STATE_UNKNOWNPENDING, which is reasonable but could confuse callers who see a PENDING status for a round that the daemon actually knows nothing about. A comment or the proto field doc noting this default would help.

Summary

The design is sound and the unit test coverage is good. The main concerns before merging are the duplicate-row race in pendingRoundTransfers (issue 1) and the missing OOR amount (issue 3). The other items are style or documentation tweaks.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 5d6926e to 996974a Compare May 15, 2026 10:25

Copy link
Copy Markdown
Member Author

Folded the fifth Claude pass into 996974a3:

  • added round-id deduplication for in-round rows, preferring terminal/history rows over pending live rows when both describe the same persisted round
  • added unit coverage for that dedupe behavior
  • documented OOR session rows as another case where amount_sat may remain zero until history supplies a persisted amount
  • changed uninitialized indexer handling in ListReceiveScripts from Internal to FailedPrecondition
  • kept the receive-script list TODO and formatter-stable target allocation

Full local gate is green again after regenerating stubs.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds darepo receive, darepo receive list, and darepo transfers list CLI commands, plus the backing daemon RPCs (ListReceiveScripts, ListTransfers), three new proto enums, a unified TransferInfo message, and a bech32m address field on NewReceiveScriptResponse. The design intentionally normalises three disjoint local sources (live round FSM states, committed transaction history, OOR sessions) into one user-facing view.


Strengths

  • Good DRY factoring: extracting newReceiveScript and sharing it between oorReceive and receive is the right call.
  • Context propagation fix: context.Background()cmd.Context() in the OOR receive handler is a genuine correctness improvement.
  • Overflow guards: clampedTransferStart and nextTransferOffset are correctly defensive against 32-bit int overflow.
  • Proto field docs: Field comments are honest about when a field can be zero (live in-memory rows, OOR sessions lacking amount, etc.). This is good API hygiene.
  • Unit-test breadth: The dedup, sort, filter, pagination, address-formatting, and filter-parsing paths all have explicit tests. The error-injection test (TestAddressFromTaprootPkScriptRejectsNonTaproot) is particularly appreciated.

Issues

Medium – transferFromTransaction always reports COMPLETED

// darepod/rpc_transfers.go
status := daemonrpc.TransferStatus_TRANSFER_STATUS_COMPLETED

The ledger history subtype is used to infer direction but not status. If the transaction history can record aborted or partially-applied rounds (e.g. a failed subtype), those would be mislabelled as COMPLETED. Please confirm the subtype space never includes failure modes, or add a subtype→status pass analogous to transferDirectionFromTransaction.

Medium – O(total-history) full scan on every call

collectTransfers fetches all round rows, all transaction history rows, and all OOR sessions into memory before filtering and paging. The historyTransfers loop re-pages through ListTransactions with maxListTransfersLimit=1000 per page until HasMore is false. For a wallet with a large history this will be slow and allocate a lot even when the caller only wants the last 10 rows.

The TODO comment acknowledges this, but given the pagination is exposed as a first-class API surface it is worth being explicit in the PR description about the expected scale at which this becomes a problem. Can the current implementation realistically handle the wallet histories expected in production?

Minor – OORTransfer missing AmountSat

transferFromOORSession leaves AmountSat at zero. The proto comment calls this out, but users doing transfers list --mode oor will see a column of zeros with no in-band explanation. Consider either:

  • populating AmountSat from session.GetAmountSat() if the OOR session type carries it, or
  • adding a comment in the CLI JSON output (via a wrapper type) or to the TransactionHistoryEntry TODO.

Minor – subtest parallelism

TestTransferDirectionFromTransaction calls t.Parallel() on the outer test but not within t.Run. Per project convention (and Go best practice) each table subtest should also call t.Parallel().

for _, test := range tests {
    test := test // capture
    t.Run(test.name, func(t *testing.T) {
        t.Parallel()
        ...
    })
}

Nit – tie-breaking in transferSupersedesRoundDuplicate

The final tie-breaker candidate.GetTransferId() > current.GetTransferId() relies on lexicographic ordering of prefixed IDs ("round:abc:7" > "round:abc"). This happens to work because history rows append ":<entryId>" to the live-round ID, but it is fragile: it encodes an implicit knowledge of the ID format. A short comment explaining why lexicographic ordering is the right choice here would prevent future confusion.

Nit – txscript.OP_DATA_32 comment

// taprootPkScriptKeyOffset is the offset of the x-only output key
// inside a native v1 taproot pkScript.
taprootPkScriptKeyOffset = 2

The constant correctly skips OP_1 + OP_DATA_32. For future readers it is worth noting that OP_DATA_32 is a length prefix (0x20), not a version opcode, to distinguish it from the witness version byte.


Quick checks

  • Generated code (daemon.pb.go, daemon_grpc.pb.go, daemon_mailboxrpc.pb.go) carries mechanical message-type index renumbering only — no hand-edits visible.
  • send.oor schema refresh correctly removes the stale pk_script parameter.
  • ListReceiveScripts pagination TODO is noted in-code; the current target list is expected to remain small.
  • No test names contain underscores.

Overall the PR is well-structured and the test coverage is solid. The main thing to address before merge is confirming (or fixing) the COMPLETED-hardcode in transferFromTransaction, and adding a note on expected scale for the full-scan pagination model.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 996974a to 9d89047 Compare May 15, 2026 10:34
@bhandras

Copy link
Copy Markdown
Member Author

Folded the latest review pass into 9d89047. This adds transaction-history status mapping instead of hard-coding completed, covers failed status/subtype mapping in tests, parallelizes the table subtests, documents the OP_DATA_32 taproot-script offset, and explains the dedupe tie-break. I also expanded the PR body with concrete CLI examples, the full-scan limitation, and the rows that can currently report unknown direction or zero amount.\n\nLocal validation on the amended head: make commitmsg-lint range="origin/main..HEAD", go test ./..., make lint-native, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. doc-check still prints the two known pre-existing CLAUDE/AGENTS divergence warnings but exits successfully.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overall this is a well-structured PR. The intent is clear, the commit messages look tidy, and the core logic is easy to follow. Observations below, roughly in priority order.


Issues

1. transferStatusFromTransaction — fragile string matching

darepod/rpc_transfers.go, transferStatusFromTransaction:

subtype := strings.ToLower(tx.GetSubtype())
if strings.Contains(subtype, "failed") || status == "failed" {

strings.Contains(subtype, "failed") will misclassify any subtype that happens to contain the word "failed" in a non-failed context (e.g. a hypothetical "pre_failed_state_reset" subtype). The ledger package already has named constants for event subtypes — use those directly in a switch rather than substring-matching. The test covers package_failed which works, but the approach is fragile against future subtype names.


2. collectTransfers doc comment mismatch

// collectTransfers gathers transfers from the existing operation-status and
// transaction-history stores before common filtering and pagination.

Filtering happens inside collectTransfers (the transferMatchesFilters loop at the bottom), not after it. The comment says "before common filtering" but that is the caller's perspective of ListTransfers, not what this function does. Reword to reflect what the function actually does.


3. ListReceiveScripts — indexer error mapped to codes.Internal

darepod/rpc_receive_targets.go:3884:

return nil, status.Errorf(codes.Internal, "unable to list "+
    "receive scripts: %v", err)

codes.Internal signals an implementation bug, not an external dependency failure. If ListMyReceiveScripts fails because the indexer is temporarily unavailable or times out, codes.Unavailable (or codes.DeadlineExceeded) is more accurate and gives callers a meaningful signal for retry logic. Reserve codes.Internal for unexpected invariant violations.


4. TestTransferMatchesFilters — thin coverage

The test covers an empty request (pass), fully-specified match (pass), and direction mismatch (fail). Missing:

  • nil transfer — the function guards for it but the guard is untested
  • OOR mode filter
  • Status filter mismatch
  • Direction UNSPECIFIED on the transfer itself with a non-unspecified direction filter (the current direction-filter logic will reject such a row, which may be surprising)

5. Direction-filter interaction with UNSPECIFIED direction rows

transferMatchesFilters rejects any transfer where transfer.Direction != req.DirectionFilter when the filter is non-unspecified. Live in-round transfers explicitly carry TRANSFER_DIRECTION_UNSPECIFIED (by design, documented in the proto). That means --direction incoming will silently drop all live in-round rows, giving the impression the wallet has no pending in-round activity. Consider either:

  • documenting this behaviour clearly in the filter help text ("incoming; live in-round rows may be hidden until confirmed"), or
  • passing through rows whose direction is UNSPECIFIED regardless of the direction filter.

Minor / Style

6. Redundant hasMore guard around nextOffset

hasMore := end < len(transfers)
// ...
if hasMore {
    resp.NextOffset = nextTransferOffset(req.GetOffset(), limit)
}

nextTransferOffset is pure and cheap — setting NextOffset unconditionally and documenting "callers should only use it when has_more is true" removes a conditional with no behavioural difference from the caller's perspective. (Not required to fix, but simplifies the code slightly.)

7. TODO in production code without a tracking issue

darepod/rpc_receive_targets.go:

// TODO: add pagination if receive-script registrations become
// long-lived enough for this list to grow without bound.

TODOs are fine, but they tend to rot. If there is an issue tracker entry for this, reference it. If not, it is worth opening one so it does not disappear.

8. Inline fmt.Sprintf inside string concatenation

rpc_transfers.go, transactionTransferID:

return "round:" + roundID + fmt.Sprintf(":%d", tx.GetEntryId())

This mixes two concatenation styles. A single fmt.Sprintf("round:%s:%d", roundID, tx.GetEntryId()) is easier to read.


Positive Notes

  • The deduplication logic in dedupeTransfersByRoundID is correct and the test covers the important supersession case (terminal history row wins over pending live-round row for the same round ID).
  • The overflow-safe pagination helpers (clampedTransferStart, nextTransferOffset) are well-tested and correct on all boundary cases.
  • Propagating cmd.Context() instead of context.Background() throughout is the right fix and good to see applied consistently.
  • The addressFromTaprootPkScript validation (length + opcode checks) fails closed for non-taproot scripts rather than silently producing a wrong address.
  • The known O(total history) scan limitation is clearly documented and the scope is appropriately constrained to local wallets.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 9d89047 to c01ed49 Compare May 15, 2026 10:51
@bhandras

Copy link
Copy Markdown
Member Author

Folded the latest Claude round into c01ed49c0. Changes in this pass:\n\n- replaced fuzzy strings.Contains(subtype, "failed") status inference with exact matching on the persisted failed transaction-history status constant\n- added test coverage that a subtype containing the word "failed" does not get misclassified as failed\n- corrected the collectTransfers comment so it matches where filtering actually happens\n- mapped receive-script indexer read failures to codes.Unavailable\n- expanded transferMatchesFilters coverage for nil rows, OOR mode mismatch, status mismatch, and unknown-direction live rows\n- changed direction filtering so unknown live in-round rows remain visible under an incoming/outgoing filter, with CLI help and PR-body text documenting that behavior\n- removed the untracked receive-script pagination TODO wording and cleaned up the transfer ID formatting\n\nLocal validation on this head: make commitmsg-lint range="origin/main..HEAD", explicit commit-message shape check, go test ./..., make lint-native, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. doc-check still reports the two pre-existing CLAUDE/AGENTS divergence warnings but exits successfully.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review — PR #451: Add Ark receive and transfer status CLI

Overview

This PR adds darepo receive, darepo receive list, and darepo transfers list commands backed by two new daemon RPCs (ListReceiveScripts, ListTransfers). The design is sound — aggregating in-memory round state, persisted round history, and OOR session records into one normalized view is the right approach for a local-wallet CLI. The acknowledged in-memory aggregation trade-off is reasonable for this use case.


Bug Fix (Nice Catch)

cmd_oor.go: The replacement of context.Background() with cmd.Context() is a real bug fix, not just cleanup. Previously the RPC could not be cancelled by Ctrl-C or a parent process.


Issues

Direction filter silently passes unknown-direction rows

transferDirectionMatchesFilter returns true for TRANSFER_DIRECTION_UNSPECIFIED regardless of the requested filter:

if direction == daemonrpc.TransferDirection_TRANSFER_DIRECTION_UNSPECIFIED {
    // Live in-round rows can be useful even before the local
    // input/output hints needed for direction classification exist.
    return true
}

A user running transfers list --direction incoming will see live in-round rows whose direction is not yet known. The CLI flag description mentions this ("unknown live in-round entries are still shown"), but the behavior is surprising. If the intent is permanent, it deserves a comment on the proto field as well. If it's transient, consider returning DIRECTION_UNSPECIFIED entries only when no direction filter is set, and documenting that pending in-round transfers may not appear under a direction filter until they acquire outpoint context.

pendingRoundTransfers may double-count a persisted pending round

The function reads from two independent sources before dedup:

  1. r.queryRoundStates(ctx) — live in-memory FSM snapshot
  2. r.ListRounds(ctx, ..., PersistedOnly: true) — database rows

A round that is both persisted and still in the live FSM (pending) will appear in both. If the round has a non-empty round_id on both rows, dedupeTransfersByRoundID will collapse them correctly. But if the live FSM row lacks a round_id it gets a temporary ID and bypasses dedup, so both rows survive into the response. This can only happen for a brief window, but it's worth a comment explaining the expected transient duplicate and why it's tolerable (or why it can't happen in practice).

transactionTransferID has a dead TRANSFER_MODE_UNSPECIFIED arm

historyTransfers is only ever called with TRANSFER_MODE_INROUND, so the case TRANSFER_MODE_UNSPECIFIED: arm (which falls through to "ledger:%d") is unreachable. This is harmless but slightly misleading. Either add a comment noting the fallback is defensive, or drop the dead case.


Suggestions

collectTransfers: in-place filter slice

filtered := transfers[:0]
for _, transfer := range transfers {
    if transferMatchesFilters(transfer, req) {
        filtered = append(filtered, transfer)
    }
}

This is idiomatic and correct (write index ≤ read index invariant holds). A one-line comment would help the next reader avoid re-verifying it:

// Reuse backing array; write index <= read index ensures safety.
filtered := transfers[:0]

Though given the project's comment-sparingly policy, feel free to skip this if you consider it obvious.

Mode filter is checked twice

transferModeAllowed gates collection per source, and transferMatchesFilters re-checks mode on each row afterwards. The current comment in collectTransfers explains the design intent clearly. No change needed, but worth confirming the double-check stays intentional as the code evolves.

ListReceiveScripts depends on indexer being initialized

if r.server.indexer == nil {
    return nil, status.Error(codes.FailedPrecondition, "indexer client not initialized")
}

If a wallet can operate without an indexer (e.g., fully local mode), receive list silently fails with an opaque precondition error instead of returning an empty list. If receive scripts are always indexed, this is fine. If not, returning empty is safer UX.


Style / Convention

  • rpc_transfers.go is 667 lines. It stays under funlen thanks to helper decomposition, but it's dense. Worth noting if funlen caps are hit on future additions.
  • test := test loop capture (e.g., rpc_transfers_test.go:83) is a Go 1.21 artifact — no longer needed in Go 1.22+. Minor, and only worth changing if the project is on 1.22+.
  • All function comments start with the function name ✓
  • No underscores in test names ✓
  • Early returns throughout ✓

Proto Design

Clean. Field numbering is strictly additive (no reuse). The address = 6 addition to NewReceiveScriptResponse is backwards-compatible. The TransferInfo fields are well-separated between round and OOR contexts (round_id vs session_id). Enum 0 values are correctly _UNSPECIFIED throughout.


Test Coverage

Good coverage of the pure helper functions:

  • Filter predicates, direction inference, status mapping, sort, pagination arithmetic, dedup
  • addressFromTaprootPkScript round-trip and rejection tests

Missing: an integration/unit test for ListReceiveScripts handler itself (indexer nil path, successful path). Not a blocker given the simplicity of the handler, but worth a follow-up.


Summary

The core logic is correct and well-structured. The two main concerns worth addressing before merge:

  1. Direction filter semantics — document the "unknown passes" contract on the proto field (or reconsider the behavior).
  2. Double-count comment in pendingRoundTransfers — clarify why a persisted+live pending round won't produce a visible duplicate, or acknowledge it's transient and harmless.

Everything else is a nit or a suggestion. Overall this is solid work.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from c01ed49 to 2727a75 Compare May 15, 2026 11:04
@bhandras

Copy link
Copy Markdown
Member Author

Folded the latest Claude round into 2727a75. This pass adds the direction-filter contract directly to the proto docs and regenerates the daemon RPC stubs, so the TRANSFER_DIRECTION_UNSPECIFIED pass-through behavior is part of the API documentation instead of just CLI help text. It also documents the transient live-FSM/persisted-pending duplicate window in pendingRoundTransfers, including why rows with the same round id are deduped and why a temporary live row without a round id may briefly remain visible rather than hiding active local state.\n\nI left the defensive TRANSFER_MODE_UNSPECIFIED fallback in transactionTransferID but added an explanatory comment. Receive scripts remain indexer-backed here, so the initialized-indexer precondition stays explicit and indexer read failures return codes.Unavailable.\n\nLocal validation on this head: make rpc, make commitmsg-lint range="origin/main..HEAD", explicit commit-message shape check, go test ./..., make lint-native, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. doc-check still reports the two pre-existing CLAUDE/AGENTS divergence warnings but exits successfully.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds three user-facing improvements: a darepo receive command, a darepo receive list command, and a darepo transfers list command, backed by two new daemon RPCs (ListReceiveScripts and ListTransfers). The approach of normalizing multiple local data sources (live FSM state, persisted round summaries, transaction history, OOR sessions) into a single paginated view is sound for the local-wallet use case this targets.


Positives

  • Clean file organization — new features get dedicated files, existing files are minimally touched.
  • context.Background()cmd.Context() fix in oorReceive is a good correctness improvement.
  • Deduplication of round rows across live FSM + history snapshot is well thought through; transferSupersedesRoundDuplicate handles both orderings correctly.
  • Overflow guards in clampedTransferStart and nextTransferOffset are correct.
  • Unit tests cover all pure utility functions; the test-in-parallel pattern is consistent throughout.
  • Proto additions are backward compatible (new enums, new messages, new field appended to existing message).

Issues

Semantics / Correctness

1. transferDirectionMatchesFilter — unknown-direction rows pass any direction filter (darepod/rpc_transfers.go)

if direction == daemonrpc.TransferDirection_TRANSFER_DIRECTION_UNSPECIFIED {
    return true  // passes outgoing, incoming, or any filter
}

The intent (show live in-round rows even when direction is unknown) is documented in the PR description but not in the CLI --direction flag help text. A user running transfers list --direction incoming would be surprised to see unclassified pending rounds in the output. The flag description currently reads:

direction filter: all, outgoing, or incoming; unknown live in-round entries are still shown

This wording is in cmd_transfers.go, which is good. However it only appears in the extended flag description, not the Short help. Consider adding a note to the Long description of newTransfersListCmd as well, since cobra shows Long on --help.

2. transferDirectionFromRound — input-outpoints-only heuristic (darepod/rpc_transfers.go)

case len(round.GetInputOutpoints()) > 0:
    return TRANSFER_DIRECTION_OUTGOING
case len(round.GetOutputOutpoints()) > 0:
    return TRANSFER_DIRECTION_INCOMING

A round where both inputs and outputs are locally known (both sent and received VTXOs visible) would be classified as "outgoing" because InputOutpoints is checked first. In the current Ark protocol model, local wallet rounds are either send or receive, not both, so this is unlikely to misclassify. But if the semantics change it could silently mis-classify. A comment explaining why inputs take priority over outputs would help future readers.

3. historyTransfers type parameter is always "round" (darepod/rpc_transfers.go)

The function signature accepts a historyType string and a mode daemonrpc.TransferMode, suggesting it is intended to be reusable for future OOR history. Currently it is only ever called with roundTransactionHistoryType. OOR history comes from oorTransfers via sessions, not transaction rows. The unused generalization leaks intent that may never materialise. Either remove the historyType parameter and hard-code "round" inside the function, or add a comment explaining what OOR type string would be needed once OOR history is persisted to the ledger.


Performance / Scalability

4. Full in-memory scan in collectTransfers (darepod/rpc_transfers.go)

The code has a clear TODO comment, but the scale ceiling is worth calling out explicitly: historyTransfers paginates ListTransactions with pages of 1,000 rows and loops until exhausted. For a daemon with, say, 50,000 history rows, this is 50 round-trip RPCs before a single ListTransfers response is returned. The acknowledged TODO is adequate for the stated local-wallet scope, but the maxListTransfersLimit guard on the output page does not protect against that inner pagination depth.

5. pendingRoundTransfers — two full scans per call (darepod/rpc_transfers.go)

queryRoundStates scans live FSM state and ListRounds(PersistedOnly: true) paginates persisted summaries in a loop. For a wallet with many historical pending rounds this doubles the work. Since only non-terminal rounds survive both checks, this is correct but potentially expensive. Again the TODO covers the long-term fix; just flagging it as a known cost.


Minor / Style

6. ListTransfers nil-req guard is unnecessary (darepod/rpc_transfers.go)

if req == nil {
    req = &daemonrpc.ListTransfersRequest{}
}

gRPC handlers receive a decoded request from the framework; it will never be nil. The guard can be removed. ListReceiveScripts correctly omits this pattern.

7. test := test loop captures in test files (darepod/rpc_transfers_test.go)

The pre-Go-1.22 loop variable capture idiom (test := test) is used throughout the test files. If the module's go.mod specifies Go 1.22+, these can be removed. If it targets an older toolchain they are harmless but should remain. Worth a quick check against go.mod.

8. newReceiveListCmd and newTransfersListCmd have no Long description

newReceiveListCmd has a one-liner Long but newTransfersListCmd has none. The transfers list command is the most complex new command and the most likely to need extended help. Given the nuanced semantics around unknown-direction filtering, adding a Long field that explains the filter behaviour would help users.

9. temporaryRoundTransferID comment omission

The function builds an intentionally fragile id from live state. A comment explaining that these ids are not stable across restarts (the point being that they only need to be unique within a single response) would help reviewers distinguish this from the stable "round:<id>" prefix.


Summary

The implementation is correct and the known scalability limitations are clearly documented. The main actionable items are:

  1. Clarify the --direction filter semantics in the Long help text of newTransfersListCmd.
  2. Remove the unnecessary req == nil guard in ListTransfers.
  3. Either simplify or document the historyType parameter in historyTransfers.
  4. Verify test := test captures against the module Go version.

The deduplication logic, overflow guards, and context propagation fixes are all correct. The proto changes are backward compatible and follow existing patterns.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 2727a75 to 6530193 Compare May 15, 2026 11:12

Copy link
Copy Markdown
Member Author

Folded the latest hardening suggestions into e9a6bed0.

This pass keeps temporary live-round ids bounded by hashing the variable round fields and including only is_temp, the short digest, and the response-local index in the returned id. The test now asserts the temporary id remains short while still distinguishing two live rows from the same round snapshot.

I also expanded the persisted pending-round scan comment to cover the live-plus-persisted pending duplicate case, sharpened the ListRoundsRequest.state_filter TODO as a proto-level prerequisite, and documented that transaction direction inference is coupled to the ledger package's canonical event/account labels.

Local validation on this head: make fmt-changed, go test ./darepod ./cmd/darepocli/darepoclicommands ./daemonrpc, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. make doc-check still exits 0 with the pre-existing AGENTS/CLAUDE divergence warnings in baselib/actor and db/actordelivery/migrations.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds three user-facing CLI commands (receive, receive list, transfers list) backed by two new RPCs (ListReceiveScripts, ListTransfers). The design is clean: both RPCs normalize multiple internal data sources into a single user-visible response, and the PR is honest about the in-memory aggregation trade-off with a documented TODO.


What's Good

  • Context propagation fix in cmd_oor.gocontext.Background()cmd.Context() is a real correctness improvement; the original code ignored command cancellation signals.
  • addressFromTaprootPkScript validates the script type before deriving an address, failing closed on non-taproot input.
  • In-place filter reuse (filtered := transfers[:0]) is safe and avoids an allocation; the range iterates the original slice header so writes don't corrupt reads.
  • Overflow guards in clampedTransferStart and nextTransferOffset are thoughtfully defended.
  • Test coverage for the pure conversion/utility functions is thorough.

Issues and Suggestions

1. Direction filter silently passes UNSPECIFIED rows (medium)

transferDirectionMatchesFilter always returns true when direction is TRANSFER_DIRECTION_UNSPECIFIED, regardless of the requested filter. The proto comment explains this, but the CLI flag help is easy to miss. Because --direction incoming returns rows with unknown direction alongside genuinely incoming ones, a user triaging receives could be confused by extra pending in-round rows. The behaviour is intentional and correct, but the Long description for the command only mentions this in passing. Consider making it more prominent, e.g. adding a dedicated sentence explaining that live in-round rows are always visible until the daemon has enough outpoint context to classify them.

2. No dedup across OOR sources (low, future risk)

dedupeTransfersByRoundID only collapses duplicates for in-round rows with a non-empty round_id. OOR transfers are appended directly. Currently oorTransfers reads from a single source, so there are no duplicates. But if a future collectTransfers adds a second OOR path (e.g. history-backed OOR rows alongside live session rows), duplicate OOR entries will silently appear. A short comment near the OOR fetch in collectTransfers calling this out would help future contributors.

3. pendingRoundTransfers fetches all persisted round pages unconditionally (medium, acknowledged)

The inner pagination loop over ListRounds(PersistedOnly: true) fetches every persisted round page with a hard page size of 1000, just to filter for pending state in Go. For large round histories this can issue many RPCs even when the caller only asked for the 10 most recent pending transfers. The TODO is there; worth noting that wiring up state_filter on ListRounds should be high priority if per-wallet round history grows before the shared transfer store lands.

4. receiveTargetFromRegisteredScript nil-script path is untested (low)

rpc_receive_targets_test.go tests addressFromTaprootPkScript directly but not the script == nil guard inside receiveTargetFromRegisteredScript. Low risk since the nil guard is defensive, but a one-line table entry would close the gap.

5. Clamped limit not passed into collectTransfers (nit)

The clamped limit is only used for the final slice window. This is correct, but a reader might expect collectTransfers to respect it to bound the scan. A short comment near the clamping block ("limit is applied after collection and dedup") would make the intent clear without code changes.

6. Proto: amount_sat as int64 (nit)

TransferInfo.amount_sat is int64, consistent with TransactionHistoryEntry.amount_sat. The field is zero for live OOR sessions as documented. Worth confirming no path ever sets it negative.


Minor Style Notes

  • strings.Join([]string{...}, " ") in newTransfersListCmd for the Long description is readable but differs from the string-concatenation pattern used elsewhere in the package. Either is fine.
  • Params: nil in the receive.list schema entry is explicit; no change needed.
  • The send.oor schema correction (removing pk_script) is a registry-only fix and the PR description explains it clearly.

Summary

The overall approach is sound and the documented trade-offs are honest. The most actionable items are #1 (direction-filter UX surprise — the behaviour is correct but the help text undersells it) and #3 (unbounded round page scans — worth tracking as a follow-up). Everything else is low-risk. The test suite is well-structured and covers the key conversion and edge-case logic.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from e9a6bed to 4a1ae46 Compare May 15, 2026 12:46

Copy link
Copy Markdown
Member Author

Folded the latest Claude pass into 4a1ae46d1363cbb66925452ef00e36fb265ae555.

This round makes the transfers list long help text more explicit that direction filters still return live in-round rows whose direction is unknown, adds a note that future history-backed OOR transfer rows should dedupe by session id, documents that transfer-list limits are applied after collection/filtering/deduplication to preserve global newest-first ordering, and adds coverage for the nil registered receive-script guard.

Local validation on this head: make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, git diff --check, go test ./darepod ./cmd/darepocli/darepoclicommands ./daemonrpc, go test ./..., make lint-native, make schema-check, and make doc-check. make doc-check still exits 0 with the pre-existing AGENTS/CLAUDE divergence warnings in baselib/actor and db/actordelivery/migrations.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review — PR #451: Add Ark receive and transfer status CLI

Overview

This PR introduces three user-facing CLI surfaces (receive, receive list, transfers list) backed by two new daemon RPCs (ListReceiveScripts, ListTransfers), plus adds a bech32m address to NewReceiveScriptResponse. The aggregate-then-sort-then-paginate approach in collectTransfers is clearly explained and appropriately scoped for a local wallet. Overall the PR is well-structured, with solid test coverage and good adherence to project conventions.


Issues Worth Addressing

1. --label flag may be silently dropped in the receive CLI path

newReceiveListCmd in cmd_receive.go registers --label on the receive command, and the schema registry advertises it as a valid param. However receive just delegates to newReceiveScript(cmd), which is the old body of oorReceive. If the original oorReceive did not read cmd.Flags().GetString("label") to populate req.Label, then the flag is silently ignored on the cobra path (the MCP/JSON path works fine because parseRequest marshals the whole request from JSON). Worth verifying the body of newReceiveScript correctly forwards the flag.

2. Lexicographic comparison of numeric entry-ID suffixes is fragile

In transferSupersedesRoundDuplicate (rpc_transfers.go):

return candidate.GetTransferId() > current.GetTransferId()

History-row IDs are built as "round:<roundID>:<entryId>" where entryId is formatted with %d. String comparison works for single-digit IDs but breaks when the digit count changes — "round:abc:10" < "round:abc:9" lexicographically but represents a newer (larger) entry. The test only exercises single-digit IDs, so the bug is currently hidden. Suggest comparing the numeric suffix directly, or using EntryId on the proto type instead of the string ID:

// Compare by numeric entry id to avoid lexicographic ordering surprises
// when id crosses a decade boundary.
return candidate.GetEntryId() > current.GetEntryId()

3. Magic-string coupling in transferStatusFromTransaction

// rpc_transfers.go
transactionHistoryStatusFailed = "failed"

This string has to match whatever ListTransactions records in confirmation_status. If that string ever changes, transferStatusFromTransaction silently maps all failed rows to COMPLETED. A comment pointing to the ledger constant it mirrors (or a shared constant from the ledger package if one exists) would make the coupling explicit and catch future drift.


Minor Observations

4. pendingRoundTransfers performs an O(total-rounds) scan on every call

The inner pagination loop fetches all persisted round pages filtering for non-terminal rounds. The TODO is present and the reasoning is sound for current wallet sizes, but consider noting the worst-case latency with very large histories in the docstring, so future maintainers know what to expect before that store-level cursor lands.

5. Correctness nit in roundHistoryTransfers loop guard

for offset := uint32(0); ; {
    // ...
    nextOffset := resp.GetNextOffset()
    if nextOffset <= offset || nextOffset > maxTransferSourceOffset {
        break
    }
    offset = nextOffset
}

On the first iteration offset = 0. If ListTransactions ever returns nextOffset = 0 with hasMore = true (a malformed response), the guard exits early and silently truncates history. A more defensive condition would be !resp.GetHasMore() as the primary break, with the guard serving as overflow protection only.

6. temporaryRoundTransferID double-encodes IsTemp

parts := []string{
    fmt.Sprintf("is_temp:%v", round.GetIsTemp()),  // hashed
    ...
}
return fmt.Sprintf("is_temp:%v:%x:index:%d", round.GetIsTemp(), ...)  // also in output

IsTemp appears in both the hash input and the formatted ID prefix. Harmless, but one of the two is redundant.


What's Done Well

  • context.Background()cmd.Context() fix in the shared newReceiveScript is a clean improvement.
  • addressFromTaprootPkScript validates IsPayToTaproot before parsing and returns clear errors.
  • dedupeTransfersByRoundID correctly prefers terminal history rows over pending live-round rows.
  • transferDirectionMatchesFilter correctly passes unknown-direction rows through direction filters, and this behaviour is documented in both the proto comments and the CLI help text.
  • Pagination guards (clampedTransferStart, nextTransferOffset) handle uint32 overflow at both ends.
  • maxListTransfersLimit = 1000 caps in-memory allocation.
  • Test coverage is thorough: filter parsing, dedupe, sort, pagination, address formatting, and nil rejection are all exercised.
  • All test names follow TestFoo (not Test_Foo) and use t.Parallel().
  • Generated files (daemon.pb.go, daemon_grpc.pb.go, daemon_mailboxrpc.pb.go) were correctly regenerated via make rpc rather than edited by hand.

Summary

Items 1 and 2 are the ones worth double-checking before merge. Item 1 could result in the --label flag being silently ignored in the normal CLI flow, and item 2 could cause the wrong history row to be displayed once entry IDs reach two digits. Everything else is minor.

🤖 Reviewed with Claude Code

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 4a1ae46 to ea11364 Compare May 15, 2026 13:00

Copy link
Copy Markdown
Member Author

Folded the latest Claude findings into ea11364cd6e9cb7c95b744b6dfb7ebec8093d4be.

This pass makes the receive --label flag forwarding explicit and covered by TestNewReceiveScriptRequestFromCmdForwardsLabel, so the normal cobra path cannot silently drop the label. It also fixes round-history duplicate supersession to compare the numeric ledger entry suffix instead of lexicographic transfer-id strings, with the existing dedupe test updated to cover round:abc:10 beating an older single-digit row.

I also documented that the failed transaction-history status string mirrors TransactionHistoryEntry.confirmation_status.

Local validation on this head: make fmt-changed, go test ./darepod ./cmd/darepocli/darepoclicommands ./daemonrpc, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. make doc-check still exits 0 with the pre-existing AGENTS/CLAUDE divergence warnings in baselib/actor and db/actordelivery/migrations.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds three user-facing CLI surfaces — darepo receive, darepo receive list, and darepo transfers list — plus the backing daemon RPCs (ListReceiveScripts, ListTransfers). The approach is sound: a thin CLI layer delegates to a normalized aggregation layer in rpc_transfers.go that stitches together live round FSM state, persisted round history, and OOR session records. The known limitation (full in-memory scan before pagination) is clearly documented and acceptable for the target use case.


Correctness

rpc_transfers.go — in-place filter aliasing the source slice

filtered := transfers[:0]
for _, transfer := range transfers {
    if transferMatchesFilters(transfer, req) {
        filtered = append(filtered, transfer)
    }
}

This relies on the invariant that the write index never overtakes the read index in a filter-in-place loop. That invariant holds here, but the subsequent call to dedupeTransfersByRoundID(filtered) allocates a fresh backing array, so the original transfers slice is silently left with stale pointers in positions past len(filtered). The code is correct today, but the aliasing makes future modifications risky. A simple filtered := make([]*daemonrpc.TransferInfo, 0, len(transfers)) would be clearer and equally fast.

transferHistoryEntryID only strips "round:" prefix

_, suffix, ok := strings.Cut(strings.TrimPrefix(id, "round:"), ":")

For an OOR history-backed ID like oor:sessionId:7, TrimPrefix is a no-op, then Cut splits on the first : and suffix becomes "sessionId:7", which strconv.ParseInt rejects. The function returns (0, false), which is graceful — and dedupeTransfersByRoundID only calls this path for TRANSFER_MODE_INROUND entries so OOR rows never hit it today. But the function name and comment imply it handles any history-backed ID. Narrowing the name (e.g. roundHistoryEntryID) or adding an OOR arm would prevent a future maintainer from relying on it incorrectly.

pendingRoundTransfers — unbounded ListRounds pagination

The inner loop pages through every persisted round row to find pending ones:

for {
    roundResp, err := r.ListRounds(ctx, &daemonrpc.ListRoundsRequest{
        PageSize: int32(maxListTransfersLimit),
        ...
    })
    ...
}

maxListTransfersLimit is 1 000, so a wallet with 10 000 persisted rounds issues 10 serial RPCs before the first response byte is written. The TODO comment is accurate, but it's worth flagging for any reviewer who approves this as a long-term API shape. Consider at minimum capping the total persisted-round scan depth and returning has_more = true in ListTransfers when the cap is hit, so callers know the list is not exhaustive.

direction_filter always passes UNSPECIFIED rows through

func transferDirectionMatchesFilter(direction, filter daemonrpc.TransferDirection) bool {
    if direction == daemonrpc.TransferDirection_TRANSFER_DIRECTION_UNSPECIFIED {
        return true
    }
    return direction == filter
}

This is intentional and documented in both the proto and the CLI Long text, so it is not a bug. But it creates a subtle UX result: --direction outgoing returns some unknown-direction rows mixed into the outgoing results. Callers who rely on the filter to exclude rows will need to post-filter client-side. A short note in the ListTransfersResponse proto comment (parallel to the existing direction_filter comment on the request) would save future integrators from being surprised.


Style / Consistency

defer conn.Close() vs defer func() { _ = conn.Close() }()

cmd_receive.go and cmd_transfers.go use defer conn.Close() which silently discards the close error. The existing cmd_oor.go code uses defer func() { _ = conn.Close() }() to make the discard explicit. Neither form is wrong in a CLI context, but the new files are inconsistent with the pre-existing pattern in the same package. Aligning to the explicit form keeps make lint-native from flagging errcheck in the future.

maxTransferSourceOffset could be math.MaxInt32

maxTransferSourceOffset uint32 = 1<<31 - 1

The comment says it "mirrors the signed SQL offset bound" — using math.MaxInt32 from the standard library makes that intention immediately clear to the next reader without the bit-shift arithmetic.


Test Coverage

The unit-test coverage for pure conversion and filter helpers is solid. A few gaps worth noting:

  • transferHistoryEntryID has no direct test. Given the subtle prefix logic described above, a table-driven test covering round:abc:7, oor:sess:7, ledger:7, and a malformed ID would pin down the exact contract.
  • TestParseTransferFilters covers "inround" but not "oor" for mode, and "incoming" but not "outgoing" for direction. These are the happy-path cases most likely to be broken by a future enum rename.
  • TestReceiveTargetFromRegisteredScriptRejectsNil tests the nil guard but there is no test for the success path of receiveTargetFromRegisteredScript with a real script, analogous to what TestAddressFromTaprootPkScript does for the inner helper.

Proto / API Design

The shape is clean and consistent with existing responses (ListTransactions, ListRounds). A few notes:

  • confirmation_height int32 in TransferInfo mirrors RoundInfo.commitment_height. A negative confirmation height is not meaningful; uint32 would be more expressive, but matching the existing field type avoids an inconsistency.
  • amount_sat = 0 for live OOR rows is documented in both the proto and the PR summary. Clients that sum balances will need to guard against zero-amount rows; a sentinel value (e.g. -1) would be unambiguous, though that requires a proto convention decision.
  • NewReceiveScriptResponse.address field 6 is a backwards-compatible addition — ✓.

Summary

The code is well-factored, thoroughly commented, and handles edge cases (overflow, nil inputs, unknown enum values) defensively. The main items worth addressing before merge:

  1. MediumpendingRoundTransfers unbounded scan: consider adding a depth cap or noting it bounds has_more correctly.
  2. MinortransferHistoryEntryID name/scope mismatch with OOR IDs.
  3. Minordefer conn.Close() inconsistency with existing package style.
  4. Nit1<<31 - 1math.MaxInt32 for readability.
  5. Nit — Missing test cases for transferHistoryEntryID and receiveTargetFromRegisteredScript success path.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from ea11364 to cef870a Compare May 15, 2026 13:16

Copy link
Copy Markdown
Member Author

Folded the latest Claude cleanup pass into cef870ab29298e0ed1e57b549227ed6d10aa6096.

This pass removes the in-place transfer filter aliasing by building a fresh filtered slice, narrows transferHistoryEntryID into roundHistoryEntryID and adds table coverage for round/OOR/ledger/malformed ids, aligns the new CLI connection cleanup with the explicit close-discard pattern used in the package, switches the source offset cap to math.MaxInt32, and documents that the exhaustive persisted-round scan keeps ListTransfers.HasMore accurate until the source query can filter by round state.

I also added a success-path test for receiveTargetFromRegisteredScript and regenerated the daemon protobuf stubs for the ListTransfersResponse comment that warns response rows can still include unknown-direction entries under a direction filter.

Local validation on this head: make rpc, make fmt-changed, go test ./darepod ./cmd/darepocli/darepoclicommands ./daemonrpc, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, make schema-check, make doc-check, and git diff --check. make doc-check still exits 0 with the pre-existing AGENTS/CLAUDE divergence warnings in baselib/actor and db/actordelivery/migrations.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds three user-facing CLI commands (darepo receive, darepo receive list, darepo transfers list) backed by two new daemon RPCs (ListReceiveScripts, ListTransfers). It also enriches the existing NewReceiveScript response with a derived address field. The code is well-structured with good WHY-comments throughout, and the approach of normalising multiple history/status sources in memory before sorting is correctly documented as a known trade-off for small local wallets.


Positives

  • cmd.Context() fix in newReceiveScript / oorReceive — correctly propagates CLI cancellation signals. The old context.Background() was a real-world bug.
  • newReceiveScriptRequestFromCmd extraction cleanly eliminates duplication between oorReceive and the new receive handler.
  • Overflow-safe pagination helpers (clampedTransferStart, nextTransferOffset) are correct and cover uint32 wrap.
  • Pagination loop guard in roundHistoryTransfers (nextOffset <= offset || nextOffset > maxTransferSourceOffset) prevents infinite loops against misbehaving backends.
  • Proto backward-compat is respected — new address = 6 is appended after existing fields in NewReceiveScriptResponse.
  • addressFromTaprootPkScript correctly uses txscript.IsPayToTaproot before attempting script parse, and both nil-params and non-taproot inputs are covered by tests.

Issues

Performance: pendingRoundTransfers exhaustively scans all persisted rounds

pendingRoundTransfers pages through every persisted round (1000 rows per page) to find ones with pending status, because ListRoundsRequest has no state filter. The existing TODO acknowledges this. The more acute concern is that this full scan runs even when the caller passes --status completed or --status failed, at which point every row fetched from this path is immediately discarded by the final transferMatchesFilters pass.

Suggestion: short-circuit the persisted-round scan when req.GetStatusFilter() is explicitly COMPLETED or FAILED — those statuses are served entirely by roundHistoryTransfers, so the pending scan contributes nothing.

// Persisted rounds only contribute pending rows. Skip the scan if
// the request explicitly filters for terminal transfers.
if req.GetStatusFilter() == daemonrpc.TransferStatus_TRANSFER_STATUS_UNSPECIFIED ||
    req.GetStatusFilter() == daemonrpc.TransferStatus_TRANSFER_STATUS_PENDING {
    // ... existing persisted-round page loop
}

API semantics: direction filter silently includes UNSPECIFIED rows

transferDirectionMatchesFilter returns true for any row whose direction is UNSPECIFIED, even when the caller passes --direction incoming. This means --direction incoming returns "incoming plus anything not yet classified". The behaviour is clearly documented in the proto comment and the CLI help text, so it is not a bug — but it is a one-way door on API semantics that could surprise callers who expect filters to be exclusive.

Worth at minimum adding a note to the proto that this may be tightened in a future version (e.g., once live rounds always carry direction hints), so downstream tools do not rely on the current pass-through behaviour as a stable guarantee.

Missing unit tests for dedup / sort / supersession

The most complex logic in the PR — dedupeTransfersByRoundID, transferSupersedesRoundDuplicate, and sortTransfersNewestFirst — is not covered by direct unit tests. The existing filter tests in rpc_transfers_test.go exercise helper functions but do not exercise these paths. Given the in-place deduped[index] = transfer substitution and the multi-level tiebreak in transferSupersedesRoundDuplicate (status → time → entry ID → transfer ID string), targeted tests here have high value relative to their size.

Specifically missing:

  • A round that appears once as PENDING (live) and once as COMPLETED (history) — the supersession case users will actually see.
  • sortTransfersNewestFirst with two rows where one has zero updated_at and the other does not.

receiveList has no pagination or size bound

ListReceiveScripts fetches all scripts via indexer.ListMyReceiveScripts in one call. The code comment ("expected to remain small") is reasonable for the current use case. A TODO noting that a limit or page cursor may be needed if the indexer grows is worth adding, since the response type already uses repeated ReceiveTarget and callers may develop expectations about completeness.

Minor: strings.Join([]string{...}, " ") diverges from project style

The long description string in newTransfersListCmd is assembled with strings.Join. The rest of the codebase uses + continuations for multi-line string building. Both are valid Go; the strings.Join approach is more unusual and adds a strings import that is otherwise only used in filter parsers. Small — mention only for consistency.

Minor: temporaryRoundTransferID imports crypto/sha256

The temporary ID is entirely local and non-security-sensitive; a non-cryptographic hash would be lighter. Not a correctness issue, but crypto/sha256 carries an expectation of security use.


Style Observations

  • Comments throughout follow the project convention of stating the function name and explaining WHY. ✓
  • Early returns used consistently. ✓
  • maxTransferSourceOffset comment could be more precise: naming the ListTransactions constant it mirrors (if one exists) or citing the accepted range would make it self-documenting at a glance.

Summary

The core change is correct and well-tested for the happy paths. The three items that would strengthen it before merge:

  1. Short-circuit the pendingRoundTransfers persisted-round scan for terminal status filters (performance, not correctness).
  2. Add targeted unit tests for dedupeTransfersByRoundID and transferSupersedesRoundDuplicate covering the pending→completed supersession case.
  3. Add a TODO on ListReceiveScripts noting the absent pagination.

The direction-filter semantics observation is more of a design discussion point than a required change.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from cef870a to 9388507 Compare May 15, 2026 13:53

Copy link
Copy Markdown
Member Author

Folded the latest Claude review into 93885070af52e9e25992af8d2e23b953c9827bcb.

This pass addresses the new review items directly: collectTransfers now skips the persisted pending-round scan whenever the request explicitly filters for terminal statuses, ListReceiveScripts has an explicit pagination TODO for future larger receive-target registries, and the transfer response proto comment now warns downstream callers not to rely on unknown-direction pass-through as permanent exclusive-filter semantics.

I also added targeted tests for the transfer dedupe/supersession path and expanded the newest-first sort test so it covers the zero-updated_at_unix_s fallback against populated update times.

Local validation on this head: make rpc, make fmt-changed, go test ./darepod ./daemonrpc, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, and git diff --check.

Claude, please re-review this latest head.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR adds darepo receive / receive list and transfers list CLI commands backed by two new daemon RPCs (ListReceiveScripts, ListTransfers). The transfer list aggregates live round FSM state, persisted round history, and OOR session status into a single normalized view. The design tradeoffs are clearly documented both in the PR description and in inline comments.

Positives

  • cmd.Context() fix in the existing oorReceive path — context.Background() was a latent bug, good catch.
  • newReceiveScriptRequestFromCmd extraction is clean and the test coverage for the label-forwarding path is appreciated.
  • Pagination overflow guards (clampedTransferStart, nextTransferOffset) are correct and tested.
  • Proto documentation is thorough; the field-level comments on TransferInfo and ListTransfersRequest explain the UNSPECIFIED pass-through semantics well.
  • Forward compatibility via UNSPECIFIED-as-catch-all for all three filter enums is the right call.
  • Defensive nil checks in transferMatchesFilters and receiveTargetFromRegisteredScript.
  • Dedup logic (dedupeTransfersByRoundID, transferSupersedesRoundDuplicate) is well-thought-out and the tests cover the status-supersedes-pending and entry-id tie-breaking paths.

Issues

Medium

1. O(full history) scan on every ListTransfers call — no concurrency guard

pendingRoundTransfers exhaustively pages through all persisted rounds to find pending ones:

for {
    roundResp, err := r.ListRounds(ctx, &daemonrpc.ListRoundsRequest{
        PageSize:      int32(maxListTransfersLimit),
        PageToken:     nextToken,
        PersistedOnly: true,
    })
    // ... iterates until nextToken == ""
}

Similarly, roundHistoryTransfers pages through all round transaction history. Both loops hold no position and issue N sequential RPCs proportional to history size. For a developer who has run hundreds of test rounds, the first transfers list call will page through all of them.

The TODO (ListRoundsRequest.state_filter) is good — just worth flagging that the scan is unbounded today and the existing ListTransactions/ListRounds calls in these loops are not in a goroutine, so a large wallet will block the handler visibly.

2. No RPCServer-level test for ListTransfers or ListReceiveScripts

The individual helper functions are well-tested, but the full handler paths — including requireWalletReady, the r.server.indexer == nil guard, and the three-source aggregation pipeline — have no test coverage. Even a table-driven test with a stub DaemonService would catch regressions in the collection/filter/paginate pipeline that the per-function tests cannot see (e.g., a mode filter accidentally short-circuiting the OOR source).


Low

3. Schema test misses limit and offset params

TestMethodRegistryTransfersSchema verifies mode, direction, and status but skips limit and offset. Given the test already exercises the registry lookup, adding two require.Equal assertions for those params is a one-liner and prevents a future accidental param removal going undetected.

4. transferDirectionFromTransaction has no test for the compound case

The subtype field takes priority over credit/debit account. There is no test for a row where Subtype == EventVTXOReceived AND DebitAccount == AccountTransfersOut. The test for "failed word in subtype" (correctly checking that ConfirmationStatus drives status, not subtype) suggests this was thought about — a compound-priority test would round out the coverage.

5. Unknown-direction pass-through may need more prominent CLI help text

The --direction flag description says "unknown live in-round entries are still shown" but buried at the end. A user who runs transfers list --direction incoming expecting an exclusive filter will be confused. The Long description already explains it, but the flag description itself could say:

direction filter: all, outgoing, or incoming
(pending in-round rows whose direction is not yet known
are always included regardless of this filter)

Nits

  • ListReceiveScripts error message on the indexer nil check uses codes.FailedPrecondition — correct — but the message "indexer client not initialized" is an internal detail. The parallel pattern elsewhere (e.g., requireWalletReady) keeps internal messages consistent; this one is fine as-is but worth a second look when the indexer nil check is generalised.

  • transactionTransferID UNSPECIFIED case comment — the fallback return fmt.Sprintf("ledger:%d", ...) with the comment "Defensive fallback for future callers" is good. However, the switch has no default label for the UNSPECIFIED case; it falls through implicitly because the UNSPECIFIED arm has only a comment. A bare default: with that comment would make the intent more explicit to a future reader adding a new TransferMode value.


Summary

The design is sound and the PR is well-scoped. The two medium issues are pre-acknowledged scalability tradeoffs that are acceptable for local-wallet CLI use — they just deserve a follow-up ticket if this RPC is ever exposed in production tooling. The unit test gap on the RPC handlers is the most actionable item before merge.

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 9388507 to 2457386 Compare May 15, 2026 14:06

Copy link
Copy Markdown
Member Author

Folded one more Claude follow-up pass into 2457386b4b020d76afa94457fe3b8c6505016c80.

This pass adds handler-level readiness tests for both new RPCs, expands the transfer direction test with the subtype-vs-account priority case, verifies the schema registry keeps limit and offset, tightens the --direction flag help so unknown pending in-round rows are called out directly, and makes the transactionTransferID fallback arm explicit with default.

I left the full-history scan as the documented local-wallet tradeoff for this PR; the code already skips the pending-round scan for terminal status filters and the remaining O(total history) behavior is called out in the implementation comments for a future source-level pagination/filter pushdown.

Local validation on this head: make fmt-changed, go test ./darepod ./cmd/darepocli/darepoclicommands, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, and git diff --check.

Claude, please re-review this latest head.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review: PR #451 — Add Ark receive and transfer status CLI

Summary: Adds darepo receive, darepo receive list, and darepo transfers list CLI commands backed by new ListReceiveScripts and ListTransfers daemon RPCs. Clean feature addition with good structure overall. Main findings below.


Code Quality & Style

  • Good: context.Background()cmd.Context() in oorReceive is the right fix, propagating cancellation correctly.
  • Good: Extracting newReceiveScriptRequestFromCmd avoids duplicating the flag-parsing logic between oorReceive and receive; the wrapper comment on newReceiveScript ("shared by receive command aliases") names the WHY correctly.
  • Nit — awkward multiline enum reference in cmd_transfers.go:
    unspecified := daemonrpc.
        TransferDirection_TRANSFER_DIRECTION_UNSPECIFIED
    An intermediate const or a local alias at the top of the file would read more naturally.
  • Nit — flag help text for limit (line 215) does not mention the default. The cobra default is shown in --help, but adding "(default 100)" in the description string keeps it visible in schema output too.
  • Schema description strings in transferMethodRegistry() are very terse ("mode filter", "direction filter"). Expanding them inline (e.g., "mode filter: all | inround | oor") would let MCP/tool callers see valid values without a separate round-trip.

Potential Bugs & Correctness

  • Schema removal of pk_script (schema_registry.go lines 603–614): The PR description frames this as a "registry correction" because pk_script was never accepted by the parser, so removing it is safe. That said, it's unrelated to the receive/transfers feature — it belongs in its own commit to keep history clean.
  • newTransfersCmd has no RunE — running darepocli transfers with no subcommand prints usage but exits 0. Other command groups (e.g., newOORCmd) behave the same way, so this is consistent with existing convention, but worth noting if UX expectations differ.
  • Direction filter semantics: the CLI help string says "pending in-round rows with unknown direction are always shown" but only appears in the Long description. A user running transfers list --direction incoming and seeing unexpected rows may be confused. Consider surfacing this in the Short too, or printing a note in the JSON output (e.g., an info field in the response).

Performance

  • The PR description explicitly calls out that ListTransfers does a full in-memory scan across round status, history rows, and OOR sessions, then applies limit/offset after filtering. The daemon-side TODO to replace this with source-aware cursors is acknowledged.
  • This is fine for today's wallet sizes but is worth tracking. Suggest ensuring the TODO comment in the daemon implementation references an issue number so it doesn't get lost.
  • offset is uint32, which matches proto3 convention. Verify the daemon guards against pathologically large offsets (e.g., offset > len(results) should return an empty list, not panic).

Security

No direct security issues. Two minor points to verify on the daemon side:

  • Label field is passed through to the indexer without CLI-side validation. If the indexer uses the label in any storage key or query, confirm length and character sanitisation happens server-side.
  • address field in NewReceiveScriptResponse is daemon-derived bech32m. Callers should treat it as display-only and not round-trip it back to the daemon as an authoritative script source (the pk_script_hex field is the canonical form).

Test Coverage

What's covered well:

  • TestParseTransferFilters and TestParseTransferFiltersRejectUnknown cover all enum paths and the error branch — good.
  • TestMethodRegistryTransfersSchema / TestMethodRegistryReceiveSchema verify schema export which is often missed.
  • TestNewReceiveScriptRequestFromCmdForwardsLabel confirms flag wiring.

Gaps:

  • No test for the receive() command handler itself (only newReceiveScriptRequestFromCmd and receiveList are indirectly exercised).
  • No test for transfersList() end-to-end (including the filter-assembly path through parseRequest).
  • No negative test: what happens when --limit 0 is passed? Does the daemon treat it as "no results" or "default"?
  • The schema tests check RequestType/ResponseType strings but not that the types actually exist in the generated proto — a typo in the registry wouldn't be caught until a schema-check run.

API Design

  • NewReceiveScriptResponse.address at field 6 is a clean backwards-compatible addition in proto3. Old clients that don't know about it will silently ignore it. ✓
  • ReceiveTarget.expires_at_unix_s = 0 meaning "daemon default applies" is a reasonable sentinel, but consider documenting in the proto comment what the daemon default actually is (or that it's indexer-configurable), so callers don't have to guess.
  • ListReceiveScriptsRequest has no filters — presumably intentional for MVP scope, but a label filter or pagination would be useful as wallets accumulate old receive scripts. A TODO comment there would signal intent.
  • The three new enums (TransferMode, TransferDirection, TransferStatus) all use _UNSPECIFIED = 0 as "all/no-filter", which is the correct proto3 pattern. ✓

Summary

Severity Issue
Low pk_script schema removal mixed into this commit; separate it
Low Terse schema descriptions hide valid enum values from tool callers
Low No RunE on newTransfersCmd root (consistent with convention but silent 0 exit)
Low ListReceiveScriptsRequest has no pagination; add a TODO
Info In-memory aggregation in ListTransfers is acknowledged; ensure daemon TODO references a tracking issue
Info expires_at_unix_s = 0 sentinel undocumented in proto

The core logic is correct, context propagation is fixed, tests cover the new enum paths and schema registry, and the protobuf changes are backwards compatible. The items above are all minor — approve after addressing the schema description nits and confirming the daemon-side TODO is tracked.


Reviewed by Claude (claude-sonnet-4-6)

@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from 2457386 to ea997fd Compare May 15, 2026 14:21

Copy link
Copy Markdown
Member Author

Folded the final Claude polish pass into ea997fd6f4682f8b00ac5d052c577df9bcf18f09.

This pass expands the transfer schema descriptions so tool callers can see the valid enum values directly, updates the transfer-list --limit help to mention its default behavior, keeps the schema descriptions covered by TestMethodRegistryTransfersSchema, and gives the source-pagination TODO an explicit tracking reference.

Local validation on this head: make fmt-changed, go test ./cmd/darepocli/darepoclicommands ./darepod, go test ./..., make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, and git diff --check.

Claude, please re-review this latest head.

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Code Review — PR #451: Add Ark receive and transfer status CLI

Overview

This PR adds three user-facing CLI commands (receive, receive list, transfers list) backed by two new RPCs (ListReceiveScripts, ListTransfers), plus a bech32m address field on the existing NewReceiveScriptResponse. The transfer list normalises in-round round state, persisted ledger history, and OOR session status into a single sorted, paginated response.

Overall this is well-structured, well-documented, and well-tested. The design choices are justified in comments and the PR description. A few items are worth addressing before merge.


Issues

1. roundHistoryTransfers called unconditionally when --status pending is set

In collectTransfers (darepod/rpc_transfers.go):

if transferModeAllowed(req, TRANSFER_MODE_INROUND) {
    if transferStatusAllowed(req, TRANSFER_STATUS_PENDING) {
        // fetches pending rounds — correctly gated
    }

    history, err := r.roundHistoryTransfers(ctx)  // always called, no status gate

When a user passes --status pending, all committed round history (which consists entirely of completed/failed rows) is fetched, page by page, and then discarded by transferMatchesFilters. For large wallets this is a full O(history) scan on every pending-only request.

Suggestion:

historyStatusAllowed := transferStatusAllowed(req, TRANSFER_STATUS_COMPLETED) ||
    transferStatusAllowed(req, TRANSFER_STATUS_FAILED)
if historyStatusAllowed {
    history, err := r.roundHistoryTransfers(ctx)
    ...
}

(Since transferStatusAllowed returns true when the filter is UNSPECIFIED, this guard passes through unfiltered requests and only skips the scan when the caller asks for pending-only.)

2. roundTransactionHistoryType is a raw string literal

// darepod/rpc_transfers.go
roundTransactionHistoryType = "round"

Direction inference already uses typed ledger package constants (ledger.EventVTXOReceived, ledger.AccountTransfersIn, etc.). Filtering by a string literal "round" is inconsistent and would silently break if the ledger ever renames the type. If a ledger.TransactionTypeRound (or equivalent) constant exists, this should reference it; if it doesn't, it should be added.

3. Comment inaccuracy in transferFromRound

// transferFromRound converts one pending round status into a transfer row.
func transferFromRound(round *daemonrpc.RoundInfo,
    status daemonrpc.TransferStatus, ...) *daemonrpc.TransferInfo {

The function accepts any TransferStatus, not only pending. Both callers happen to pre-filter to pending, but the comment implies a contract that the function doesn't enforce. Change the comment to describe what the function actually does, or add an assertion if "pending-only" is genuinely intended.


Nits

  • Missing test coverage for "pending" and "failed" status filter values: TestParseTransferFilters covers "completed" but not the other two valid status strings. Given that TestParseTransferFiltersRejectUnknown already tests the rejection path, adding these two cases is low-cost.

  • test := test capture in TestRoundHistoryEntryID: If the project is on Go 1.22+, loop-variable capture is per-iteration and this pattern is no longer needed. Minor cleanup.

  • dedupeTransfersByRoundID — function name vs. future scope: The function comment and a TODO inside collectTransfers mention that OOR rows should eventually be deduped by session ID. Renaming to something like dedupeTransfers and adding a TODO inside the function itself (alongside the existing OOR comment) would make future work easier to find.


Positive Highlights

  • context.Background()cmd.Context() in oorReceive is a correct and important fix; context cancellation was silently swallowed before.
  • Pagination guard functions (clampedTransferStart, nextTransferOffset) are small, pure, and tested against overflow; good.
  • The proto schema follows the established pagination contract (limit, offset, has_more, next_offset) and all new fields are additive / backward-compatible.
  • The unknown-direction pass-through behaviour in transferDirectionMatchesFilter is correctly documented in the proto comment, the CLI flag help text, and the PR description.
  • Tests are thorough — direction inference, status mapping, deduplication logic, sorting, pagination boundaries, and wallet-readiness guards all have explicit unit tests.
  • The acknowledged in-memory full-scan TODO is clearly scoped to local-wallet CLI use and not a correctness issue for the intended scale today.

🤖 Generated with Claude Code

Users need a receive command that works for quick local Ark tests.
The old CLI exposed lower-level pieces, so basic receive flows were
hard to discover without reading code.

This adds generic receive commands plus a transfer list that reports
in-round and OOR sends and receives through one status surface. The
daemon also returns the bech32m address for new receive scripts, so
callers do not need to derive it themselves.

Transfer listing now walks paginated source history before filtering,
so older matching rows are not hidden behind the first source page. The
CLI uses command contexts for new RPCs, and temporary round ids now
avoid collisions when live rounds have no server id yet.
@bhandras
bhandras force-pushed the codex/ark-transfer-status branch from ea997fd to 5e6c0be Compare May 15, 2026 14:32
@bhandras

bhandras commented May 15, 2026

Copy link
Copy Markdown
Member Author

Folded the latest Claude review into 5e6c0be1c4a4de37a7e6e1aa0a3bff2ae223b539.

This pass skips committed round-history scans for pending-only transfer requests, replaces the raw "round" transaction-history filter with the typed ledger.TransactionTypeRound constant, updates transferFromRound and transfer dedupe naming/comments, and moves the future OOR session-id dedupe TODO next to the dedupe logic. It also adds parser coverage for --status pending and --status failed, adds focused coverage for the history-scan status gate, and removes the now-unneeded loop-variable capture in the round history ID test.

Local validation on this head: make fmt-changed, go test ./cmd/darepocli/darepoclicommands ./darepod ./ledger with a clean rerun of go test ./darepod after a timing-only listener-test timeout, go test ./..., GOMAXPROCS=2 GOGC=50 make lint-native, make commitmsg-lint range="origin/main..HEAD", explicit 3-10 sentence commit-message check, make fmt-changed-check, and git diff --check.

@claude, please re-review this latest head.

@bhandras

Copy link
Copy Markdown
Member Author

@claude re-review

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 2m 27s —— View job


Code Review

  • Reading changed files
  • Reviewing rpc_transfers.go
  • Reviewing rpc_receive_targets.go
  • Reviewing CLI commands
  • Reviewing tests
  • Posting final review

This PR has been through an extensive review cycle and the resulting code is in very good shape. The main concerns from previous rounds have all been addressed. Here is what the current head looks like.

What Looks Good

  • requireWalletReady() guard is present in both ListTransfers and ListReceiveScripts. ✓
  • context.Background()cmd.Context() fix is correctly applied in newReceiveScript and all new CLI commands. ✓
  • transferHistoryStatusAllowed correctly short-circuits the committed history scan when --status pending is given, avoiding a full round-table page walk. ✓
  • dedupeTransfers correctly collapses the live/history overlap for the same round, preferring terminal history over pending live rows and breaking ties on numeric entry ID. ✓
  • roundHistoryEntryID scope is correctly narrowed to round-prefixed IDs; the test covers oor:, ledger:, and malformed variants. ✓
  • addressFromTaprootPkScript delegates to txscript.IsPayToTaproot/ParsePkScript/Address — no manual opcode slicing. ✓
  • Label forwarding is tested via TestNewReceiveScriptRequestFromCmdForwardsLabel. ✓
  • Proto documentation on direction_filter pass-through semantics and the silent limit cap are present in both ListTransfersRequest and ListTransfersResponse. ✓
  • All typed ledger constants (ledger.EventVTXOReceived, ledger.TransactionTypeRound, etc.) are used in place of raw strings. ✓
  • Overflow-safe pagination (clampedTransferStart, nextTransferOffset) is correct and tested including the ^uint32(0) edge. ✓
  • t.Parallel() in all test functions including the new receive-targets tests. ✓
  • dedupeTransfersByRoundIDdedupeTransfers rename correctly positions for future OOR session dedup. ✓

Minor Remaining Observations

1. oorGet and oorList still use context.Background() (pre-existing)

Not introduced by this PR, but now that oorReceive is fixed and newReceiveScript/transfersList use cmd.Context(), the two remaining callsites are the only ones in the package that still ignore CLI cancellation signals. Worth a follow-up cleanup.

2. OOR transfers fetched unconditionally under --status pending

collectTransfers correctly gates roundHistoryTransfers via transferHistoryStatusAllowed, but oorTransfers is always called when OOR mode is allowed, regardless of status. A --status pending request will still fetch all OOR sessions (including completed/failed ones) before discarding them. This is consistent with the existing in-memory tradeoff and fine for the local-wallet scope, but the TODO comment in collectTransfers could mention this source too for completeness.

3. Schema description vs. CLI flag behavior for --limit

The schema describes limit as "max rows to return; zero uses default", which correctly documents the RPC contract. However the cobra flag registers a default of 100 (cmd.Flags().Uint32("limit", 100, ...)), so the CLI never sends 0 — the daemon's "zero → default" path is unreachable from the CLI. This is a cosmetic inconsistency: the effective behavior is correct (the user gets 100 by default), but the schema wording implies a caller could send 0 to get the default, while the CLI always sends an explicit value. No change required, just worth noting.

4. crypto/sha256 for response-local temporary IDs

temporaryRoundTransferID uses crypto/sha256 for a non-security-sensitive purpose. The choice works and is stable, but fnv.New64a or xxhash would be lighter and more idiomatic for a pure-uniqueness hash. Low priority — not a correctness concern.

Summary

After this many review iterations, the code is clean, well-commented, and well-tested. None of the above observations are blockers. The pre-existing context.Background() issue in oorGet/oorList is the most actionable cleanup item and could be folded into this PR or done as a follow-up — your call. Everything else is ready to merge from a correctness and maintainability standpoint.

@levmi levmi added the P2 Priority 2 — medium label Jun 10, 2026
@levmi levmi added cli CLI and user-facing commands rpc RPC transport and protobuf ux Usability, error messages, first-run labels Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-review cli CLI and user-facing commands P2 Priority 2 — medium rpc RPC transport and protobuf ux Usability, error messages, first-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants