multi: extract darepocli commands, add darepod RunWithContext - #174
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the modularity and testability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the darepocli commands into a reusable package and introduces a context-aware RunWithContext method for the darepod server, which is a great improvement for testing and embedding. The new board command is also a welcome addition.
My review includes a couple of suggestions for improvement:
- In
darepod/rpc_server.go, I've suggested handling a specific error case in theBoardRPC to provide a more informative response to the client, aligning with the API definition. - In
darepod/logging.go, I've pointed out some code duplication that could be refactored to improve long-term maintainability.
Overall, the changes are well-structured and move the project in a good direction.
| if result.IsErr() { | ||
| return nil, status.Errorf(codes.Internal, | ||
| "board registration failed: %v", | ||
| result.Err()) | ||
| } |
There was a problem hiding this comment.
The Board RPC can fail if there are no boarding UTXOs available, with an error message containing no inputs to register. Currently, this is returned as a generic internal error to the client. To adhere to the API contract defined in the protobuf file (which specifies a no_boarding_utxos status), this specific error case should be handled to return a BoardResponse with the appropriate status.
While string matching on the error is not ideal, it's a pragmatic short-term solution. A TODO to use a typed error in the future would be a good addition.
if result.IsErr() {
err := result.Err()
// TODO: Use a typed error once available from the round actor.
if strings.Contains(err.Error(), "no inputs to register") {
log.InfoS(ctx, "Board registration skipped: no boarding UTXOs")
return &daemonrpc.BoardResponse{
Status: "no_boarding_utxos",
}, nil
}
return nil, status.Errorf(codes.Internal,
"board registration failed: %v", err)
}| subsystems := []struct { | ||
| name string | ||
| useLogger func(btclog.Logger) | ||
| }{ | ||
| {Subsystem, UseLogger}, | ||
| {actor.Subsystem, actor.UseLogger}, | ||
| {round.Subsystem, round.UseLogger}, | ||
| {oor.Subsystem, oor.UseLogger}, | ||
| {vtxo.Subsystem, vtxo.UseLogger}, | ||
| {wallet.Subsystem, wallet.UseLogger}, | ||
| {lwwallet.Subsystem, lwwallet.UseLogger}, | ||
| {serverconn.Subsystem, serverconn.UseLogger}, | ||
| {chainbackends.Subsystem, chainbackends.UseLogger}, | ||
| { | ||
| chainbackends.LndClientSubsystem, | ||
| chainbackends.UseLndClientLogger, | ||
| }, | ||
| {lndbackend.Subsystem, lndbackend.UseLogger}, | ||
| {indexer.Subsystem, indexer.UseLogger}, | ||
| {db.Subsystem, db.UseLogger}, | ||
| } |
There was a problem hiding this comment.
The subsystems slice defined here is a duplicate of the one in the (now unused) SetupLoggers function. To improve maintainability and avoid having to update two lists in the future, consider extracting this slice into a package-level variable. Both SetupLoggers and SetupLoggersWithShutdownFn can then iterate over this shared slice.
There was a problem hiding this comment.
Pull request overview
This PR modularizes darepocli so it can be embedded (e.g., in the REPL), adds a context-driven lifecycle entrypoint for darepod.Server, and introduces a new Board RPC/CLI command for triggering round registration.
Changes:
- Extract
darepoclicobra command tree intocmd/darepocli/darepoclicommandswith exportedNewRootCmd()andPrintError(). - Add
Server.RunWithContext(ctx)and refactor shared startup intoServer.run(ctx, shutdownFn). - Add
BoardRPC (proto + generated stubs) and corresponding server/CLI implementations; ignore/darepoclibinary in.gitignore.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| darepod/server.go | Refactors daemon startup/shutdown to support context-managed lifecycle via RunWithContext. |
| darepod/logging.go | Adds SetupLoggersWithShutdownFn to decouple logging-triggered shutdown from signal.Interceptor. |
| darepod/rpc_server.go | Adds Board RPC handler that triggers round registration via the round actor. |
| daemonrpc/daemon.proto | Defines new Board RPC and request/response messages. |
| daemonrpc/daemon.pb.go | Regenerated protobuf types to include BoardRequest/BoardResponse and service descriptor updates. |
| daemonrpc/daemon_grpc.pb.go | Regenerated gRPC client/server stubs to include Board. |
| daemonrpc/daemon_mailboxrpc.pb.go | Regenerated mailbox RPC router/client to include Board. |
| cmd/darepocli/main.go | Converts binary into a thin wrapper around darepoclicommands.NewRootCmd(). |
| cmd/darepocli/darepoclicommands/root.go | New importable root command + shared PrintError; wires in new board subcommand. |
| cmd/darepocli/darepoclicommands/cmd_board.go | Adds darepocli board command that calls the new Board RPC. |
| cmd/darepocli/darepoclicommands/schema_registry.go | Moves schema registry into darepoclicommands package. |
| cmd/darepocli/darepoclicommands/json_input.go | Moves JSON parsing utilities into darepoclicommands package. |
| cmd/darepocli/darepoclicommands/format.go | Moves formatting helpers into darepoclicommands package. |
| cmd/darepocli/darepoclicommands/client.go | Moves daemon client connection logic into darepoclicommands package. |
| cmd/darepocli/darepoclicommands/cmd_getinfo.go | Repackages existing command under darepoclicommands. |
| cmd/darepocli/darepoclicommands/cmd_wallet.go | Repackages existing command under darepoclicommands. |
| cmd/darepocli/darepoclicommands/cmd_vtxos.go | Repackages existing command; updates printError -> PrintError. |
| cmd/darepocli/darepoclicommands/cmd_send.go | Repackages existing command under darepoclicommands. |
| cmd/darepocli/darepoclicommands/cmd_schema.go | Repackages existing command; updates printError -> PrintError. |
| cmd/darepocli/darepoclicommands/cmd_mcp.go | Repackages existing command under darepoclicommands. |
| .gitignore | Adds /darepocli binary to ignore list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| future := roundRef.Ask(ctx, msg) | ||
| result := future.Await(ctx) | ||
|
|
||
| if result.IsErr() { | ||
| return nil, status.Errorf(codes.Internal, | ||
| "board registration failed: %v", | ||
| result.Err()) | ||
| } | ||
|
|
||
| log.InfoS(ctx, "Board registration triggered") | ||
|
|
||
| return &daemonrpc.BoardResponse{ | ||
| Status: "registered", | ||
| }, nil |
There was a problem hiding this comment.
Board currently always returns status "registered" on success and maps any round-actor error to codes.Internal. If there is no pending round (or no confirmed boarding intents), the round actor can return errors like "no pending round for event *round.RegistrationRequested" which are expected/valid states and should map to a user-facing result (e.g., status "no_boarding_utxos" per daemon.proto, or codes.FailedPrecondition) instead of Internal.
| // callers manage daemon lifecycle via context cancellation instead | ||
| // of requiring a signal.Interceptor (which is process-global). | ||
| func (s *Server) RunWithContext(ctx context.Context) error { | ||
| return s.run(ctx, func() {}) |
There was a problem hiding this comment.
RunWithContext passes a no-op shutdownFn into run(), which means critical log events (via SubLoggerManager's shutdown callback) can no longer trigger a shutdown when using the context-based entrypoint. Consider deriving a child context with context.WithCancel(ctx) inside RunWithContext and passing the cancel func as shutdownFn, so both caller cancellation and critical logs can stop the daemon.
| return s.run(ctx, func() {}) | |
| ctx, cancel := context.WithCancel(ctx) | |
| defer cancel() | |
| return s.run(ctx, cancel) |
| // ServerMessageNotification, matching the pattern used by | ||
| // the systest client (systest/client.go:TriggerRegistration). |
There was a problem hiding this comment.
This comment references systest/client.go:TriggerRegistration, but there is no systest/client.go in the repo. Please update the reference to an existing file/location (or remove it) so future readers can find the pattern being referenced.
| // ServerMessageNotification, matching the pattern used by | |
| // the systest client (systest/client.go:TriggerRegistration). | |
| // ServerMessageNotification, following the standard client | |
| // interaction pattern for the round service. |
| // to acknowledge the JoinRoundRequest via the mailbox before giving | ||
| // up. This must be long enough for the durable egress path and one | ||
| // mailbox round-trip. | ||
| const boardJoinTimeout = 30 * time.Second |
There was a problem hiding this comment.
I think this should instead be a non-blocking call. Then we'll add a CLI rounds command that allows a user to check on pending/requested rounds.
| defer assemblyTicker.Stop() | ||
|
|
||
| var reachedAssembly bool | ||
| for !reachedAssembly { |
There was a problem hiding this comment.
We can simplify this a lot, instead we can just go through the wallet entirely here. Then have it send the message to the round actor as needed re the VTXO requests.
|
|
||
| for { | ||
| select { | ||
| case <-joinCtx.Done(): |
There was a problem hiding this comment.
Same here, we can remove this, then make a rounds CLI command that lets the caller list/query the active rounds (and the state of the round), and also watch to be notified when a round concludes or advances.
|
|
||
| // JoinAck: server accepted the client's join request. | ||
| addRoundRoute( | ||
| roundpb.MethodJoinAck, |
|
@claude review this |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Add Board RPC with BoardRequest/BoardResponse messages to the DaemonService proto. This enables clients to trigger round registration for confirmed boarding UTXOs via the daemon's gRPC API. Regenerated Go stubs via make rpc.
Implement the Board RPC handler in darepod that triggers round registration by sending RegistrationRequested to the round actor via its service key. Add the corresponding darepocli board CLI command and register it in the root command. The implementation mirrors the pattern used in systest/client.go:TriggerRegistration, sending a ServerMessageNotification with RegistrationRequested to the round actor via Ask.
Move all command implementations from package main into a new darepoclicommands sub-package with an exported NewRootCmd() constructor. This allows other packages (e.g., darepotest repl) to import and embed the darepocli command tree without compiling a separate binary. The main.go becomes a thin wrapper that calls darepoclicommands.NewRootCmd().Execute().
Refactor RunUntilShutdown to extract the core startup logic into a shared run(ctx, shutdownFn) method, then add RunWithContext as a context-based entry point that avoids the process-global signal.Interceptor. This allows test harnesses and the darepotest REPL to manage multiple concurrent darepod instances via context cancellation instead of fighting over the singleton OS signal handler.
Move the DAREPOD_WALLET_PASSWORD environment variable check before the stdin pipe detection in readPassword(). This prevents the function from blocking on a pipe that never produces data (e.g. when running inside the darepotest REPL which pipes stdin through readline).
Add Service and Method fields to the serverconn TLV wire format so that outbound mailbox envelopes carry the fully-qualified protobuf service and method names needed for dispatch on the operator side. The round actor's processOutbox extracts these from the new RpcRouted interface that client outbox messages implement. Without routing metadata the operator's clientconn ingress loop silently drops envelopes because it cannot match them to a handler.
Several fixes to make the full boarding flow work: - Add signing_key field (field 6) to VTXORequest proto so the server can verify BIP-322 join-auth messages. - Serialize SigningKey in JoinRoundRequest.ToProto() and add RpcService/RpcMethod routing methods to all client outbox message types. - Add MethodJoinAck constant and register the JoinAck event route in darepod so the client FSM receives operator acknowledgment. - Rewrite Board RPC to fetch boarding balance, compute VTXO amounts, register output requests, and poll the FSM until the operator acknowledges the join (RoundJoined state). - Set default MaxOperatorFee in round client config.
The tree node constructors (NewLeafNode, NewBranchNode) compute FinalKey via MuSig2 key aggregation, but proto deserialization bypasses them. Without FinalKey, signature verification in VerifySigned fails with a nil-pointer dereference after the operator sends aggregated signatures. Compute FinalKey for each deserialized node that has cosigners, copying the slice first since MuSig2 AggregateKeys sorts in-place. Update the round-trip test to clear FinalKey on both sides since it is a derived field not carried in the proto.
Add an exported method that connects to the harness LND node, requests a P2WKH address, faucets the given amount, and mines 6 blocks to confirm. This allows callers like darepotest to ensure the operator has funds for commitment transactions without manual docker exec steps.
Replace the standalone RpcRouted interface with a ServiceMethod() method on the ServerMessage interface, reusing the existing mailboxrpc.ServiceMethod type that the unary facade, codegen stubs, and EventRouter already use. This simplifies the outbound event path by removing the optional type assertion in processOutbox and making routing metadata mandatory for all server-bound messages. The OOR transport messages (SendSubmitPackageRequest, SendFinalizePackageRequest, SendIncomingAckRequest) gain ServiceMethod() implementations, fixing a gap where their outbound envelopes previously had empty routing metadata. Local-only OOR types (MarkInputsSpentRequest, ScheduleRetryRequest) no longer satisfy ServerMessage since they lack routing and are never sent to the server. Client-to-server method name constants are added to roundpb and oorpb alongside the existing server-to-client push event constants.
Extract the repeated subsystem registration list into a package-level allSubsystems slice. Both SetupLoggers and SetupLoggersWithShutdownFn now iterate over this single source of truth, so adding a new subsystem requires only one edit instead of maintaining two identical lists that can drift apart.
6207195 to
5a8d060
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 4m 1s —— View job Code ReviewThis PR is in good shape overall — the Bugs1. func (s *Server) RunWithContext(ctx context.Context) error {
ctx, cancel := context.WithCancel(ctx)
return s.run(ctx, cancel) // no defer cancel()
}
func (s *Server) RunWithContext(ctx context.Context) error {
ctx, cancel := context.WithCancel(ctx)
defer cancel()
return s.run(ctx, cancel)
}2. // requireWalletReady() checks the walletReady channel...
if err := r.requireWalletReady(); err != nil { return nil, err }
// ...but walletRef is set at step 9, while the gRPC server
// starts at step 6 and walletReady is closed at step 1 (lnd mode).
wRef := r.server.walletRef.UnsafeFromSome() // can panic!In lnd mode, 3. The proto comment ( // status is "registered" on success, "no_boarding_utxos" if
// there are no confirmed boarding UTXOs available.
string status = 1;But when 4. Silent When Style / Logging5. // Wrong — error as positional variadic argument
a.logger(ctx).WarnS(ctx, "Failed to forward board to round actor", err)Per the project's structured-logging guidelines, errors should be passed as a key-value pair: a.logger(ctx).WarnS(ctx, "Failed to forward board to round actor",
btclog.Fmt("err", "%v", err))The Design Observations6. The polling loop in 7. key := info.RoundId
if info.IsTemp {
key = "temp:" + key // RoundId is "" for temp rounds → always "temp:"
}If there were ever multiple concurrent temp rounds, they'd all map to Minor / Nits
|
RunWithContext was passing a no-op func(){} as the shutdownFn to
s.run(), which meant critical-error paths that call shutdownFn to
trigger graceful shutdown were silently ignored. Derive a child
context with context.WithCancel and pass the cancel func instead,
matching the semantics of RunUntilShutdown which wires the
signal.Interceptor's RequestShutdown.
This commit restructures the boarding flow so that the Board RPC is a thin, non-blocking gateway: it validates that the wallet is ready, fetches operator terms, and delegates the entire flow to the wallet actor via a BoardRequest message. The wallet actor's new handleBoard handler fetches the confirmed boarding balance from its store, computes the VTXO output amount (balance minus operator fee), validates it exceeds the dust limit, and then sends a TriggerBoardMsg to the round actor. This message combines the two previously separate steps (RegisterVTXORequests + RegistrationRequested) into one atomic Tell. The round actor's handleTriggerBoard handler receives the amounts, builds VTXO requests via buildVTXORequest, registers an IntentPackage on the assembling round, and triggers registration via RegistrationRequested — all within a single handler. The old Board RPC had two blocking polling loops: one waiting for PendingRoundAssembly and another waiting for RoundJoined, each with 30-second timeouts. These are replaced by a single Ask to the wallet actor that returns immediately. Callers can use ListRounds/WatchRounds (added in a subsequent commit) to observe round progress.
Define the proto schema for round observability: RoundState enum with all 14 FSM states, RoundVTXOInfo for per-round VTXO details, and RoundInfo combining round_id, state, is_temp, and vtxos. Add ListRoundsRequest with page_size, page_token, and persisted_only fields. Pending (in-memory) rounds are always returned and do not count against the page limit. Add WatchRoundsRequest/Response for server-streaming state updates. On the database side, add a ListRoundsPaginated SQL query using cursor-based pagination (WHERE round_id > cursor ORDER BY round_id ASC LIMIT N) so we never load all rounds into memory.
Regenerated with protoc v5.28.0 / protoc-gen-go v1.36.6 after adding RoundState enum, RoundVTXOInfo, RoundInfo, ListRounds, and WatchRounds to daemon.proto.
Regenerated via make sqlc after adding the ListRoundsPaginated query to db/sqlc/queries/round.sql.
Add SQL-backed pagination for persisted rounds via RoundPersistenceStore.ListRoundsPaginated, which returns lightweight RoundSummary structs (round ID, status, and VTXO outpoints/amounts) without deserializing full commitment transactions or tree data. The ListRounds RPC splits round sources: pending (in-memory) rounds are fetched from the round actor and always returned on the first page, while persisted rounds (input_sig_sent, confirmed) come from SQL with cursor-based pagination. Each persisted round includes its VTXOs via ListVTXOsByRound. The persisted_only flag skips in-memory rounds entirely. WatchRounds opens a server-streaming connection that polls the round actor at 500ms intervals and pushes RoundInfo messages whenever a round's FSM state changes, enabling real-time observability without blocking RPCs. The Server struct now stores a roundStore reference so the RPC layer can query persisted rounds directly without routing through the actor system.
Add a rounds parent command with list and watch subcommands for round FSM observability. rounds list calls the ListRounds RPC and prints the JSON response. It supports --persisted-only, --page-size, and --page-token flags for pagination control. rounds watch opens a WatchRounds streaming connection and prints each state update as it arrives.
Test cursor-based pagination across 5 rounds: empty database returns empty, page_size limiting works correctly, cursor advances through pages, VTXOs are attached to the correct rounds (2 VTXOs each for the first two rounds), and status transitions from input_sig_sent to confirmed after FinalizeRound are reflected in subsequent queries.
Update CLAUDE.md/AGENTS.md for packages affected by this PR: - darepod: add RPCServer type, Board non-blocking invariant, ListRounds split (pending vs persisted), roundStore reference. - round: add TriggerBoardMsg receive from wallet via lib/actormsg. - wallet: add BoardRequest/BoardResponse types, TriggerBoardMsg send. - db: add RoundSummary/VTXOSummary types, ListRoundsPaginated method. - lib: add TriggerBoardMsg and other cross-package message types to lib/actormsg section. - cmd/darepocli/darepoclicommands: new CLAUDE.md with command table.
5a8d060 to
1bad401
Compare
Address golangci-lint findings: remove unnecessary int32 conversion in ListRounds, use errors.Is for io.EOF check in rounds watch, add nolint:funlen to processOutbox dispatch loop, wrap long comment in actormsg, lowercase error string in cmd_board, remove leading newline in cmd_wallet, and drop stale nolint:funlen from RunUntilShutdown.
eec2f75 to
07f3b5d
Compare
The commitment transition accepts one funding source per boarded input: per-source amounts, proofs, witnesses, and signing plans, with the request total pinned to the sources' sum. The tree root source becomes a V1 compact path whose additional confirmed bases carry the co-inputs' lineages, bound to the transition by the path verifier. Requires tap-sdk multi-base proof paths (#174).
The commitment transition accepts one funding source per boarded input: per-source amounts, proofs, witnesses, and signing plans, with the request total pinned to the sources' sum. The tree root source becomes a V1 compact path whose additional confirmed bases carry the co-inputs' lineages, bound to the transition by the path verifier. Requires tap-sdk multi-base proof paths (#174).
The commitment transition accepts one funding source per boarded input: per-source amounts, proofs, witnesses, and signing plans, with the request total pinned to the sources' sum. The tree root source becomes a V1 compact path whose additional confirmed bases carry the co-inputs' lineages, bound to the transition by the path verifier. Requires tap-sdk multi-base proof paths (#174).
Summary
package maininto importabledarepoclicommandssub-package with exportedNewRootCmd(),enabling embedding in the darepotest REPL.
RunWithContext(ctx)todarepod.Serveras a context-basedalternative to
RunUntilShutdown(interceptor), allowing testharnesses and the REPL to manage multiple concurrent darepod
instances without the process-global signal handler.
/darepoclibinary to.gitignore.Test plan
go build ./cmd/darepocli/— existing binary still compilesgo build ./darepod/— package compiles with new APImake unit pkg=internal/repl— REPL tests passdarepotest replstarts darepod viaRunWithContext