vtxo+round+wallet: purify VTXO FSM and move intent composition to wallet - #172
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 refactors the VTXO lifecycle management by clearly separating responsibilities across the VTXO actor, round actor, and wallet. The VTXO actor is now a lean state machine focused solely on coin lifecycle and forfeit execution. The wallet assumes the role of composing user-driven intents for VTXO operations, while the round actor's responsibility is narrowed to validating and registering these pre-composed intent packages. This architectural change reduces coupling, improves extensibility, and lays the groundwork for future features like coin selection and in-round sends. 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
Activity
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
|
8c450da to
699bb06
Compare
![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 introduces a comprehensive three-PR execution plan document for refactoring VTXO management, reservation safety, and in-round sends. The code changes primarily focus on the first phase of this plan, which involves refactoring the VTXO actor's state machine and routing. Key changes include moving intent composition logic from the round actor to the wallet, introducing a new RegisterIntentMsg for the wallet to send pre-composed intent packages to the round actor, and routing VTXO actor outbound signals (like forfeit requests and signatures) through the VTXO manager instead of directly to the round actor. The VTXO actor's states RefreshRequestedState and ExpiringState have been renamed to PendingForfeitState and UnilateralExitState respectively, and related trigger messages have been removed. Additionally, a VTXOReader interface is introduced in the wallet to allow it to load VTXO descriptors for intent package creation, and ForfeitRequest now includes an Amount field to optimize total forfeit amount computation. A review comment highlights a documentation issue where hardcoded local file paths in the execution plan make links unusable for other developers.
I am having trouble creating individual review comments. Click here to see my feedback.
docs/vtxo_three_pr_execplan.md (7)
This design document contains a hardcoded local file path, which makes the link unusable for other developers. It would be better to use a relative path if the file is within the repository.
A similar hardcoded path is present on line 1017 in a cd command example, which should also be generalized for other developers.
4e5d307 to
6be4ecc
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 37s —— View job Code Review
This is a well-structured, clearly motivated refactor. The core idea—separating lifecycle concerns (VTXO FSM) from business intent composition (wallet)—is sound, and the code executes on it cleanly. Below are the findings from a full read of the changed files. Correctness Issues1. When building the sorted output, only requests = append(requests, &types.ForfeitRequest{
VTXOOutpoint: &op,
// Amount is NOT copied
})In the current call graph this is safe —
2. Asymmetric VTXO state-transition responsibility is undocumented at the seam — The two intent paths have fundamentally different ownership of the
This asymmetry is correct (handled gracefully — idempotent Design / Behavioral Observations3. All VTXOs in a single 4. Partial-success silent promotion in When some VTXOs fail to load (errors populated) but at least one succeeds, the successful forfeits are still submitted to the round. The caller gets Nits / Minor Issues5. The wallet-level descriptor renames 6. The 7. case *actormsg.RegisterIntentMsg:
return a.handleRegisterIntent(ctx, &RegisterIntentRequest{
Package: &IntentPackage{Intents: Intents{
Forfeits: m.Forfeits,
VTXOs: m.VTXOs,
Leaves: m.Leaves,
}},
})If What's Well Done
|
6be4ecc to
b490af3
Compare
|
@claude review again |
|
Claude finished @ellemouton's task in 2m 56s —— View job Code Review
All seven findings from the previous review have been resolved. Noting them explicitly and then adding fresh observations. Previously Raised Issues — All Resolved
Fresh Observations1. Forfeit/leave positional pairing is an implicit contract — Each 2.
3. Inconsistency: all-fail vs partial-fail return conventions — When the 4. If a VTXO actor has terminated between FSM registration and the notification loop (race between What's Well DoneThe resolutions to all prior findings are clean and surgical — no unnecessary changes leaked in. Specifically:
LGTM with the minor observations above (none are blockers). |
117462b to
a2dec77
Compare
Rename the terminal expiry state to better reflect its semantics: the VTXO has reached critical expiry and is being handed off to the chain resolver for unilateral on-chain exit. This is a pure rename with no behavioral changes. - ExpiringState → UnilateralExitState - VTXOStatusExpiring → VTXOStatusUnilateralExit
Rename the pre-forfeit waiting state to reflect its actual semantics: the VTXO is committed to cooperative consumption and is awaiting concrete forfeit details from the round actor. The old name implied this state was specific to the "refresh" product concept, but it is equally reachable via leave requests and in-round sends. - RefreshRequestedState → PendingForfeitState - VTXOStatusRefreshRequested → VTXOStatusPendingForfeit - Update test names and transition comments
The VTXO actor no longer distinguishes between refresh and leave as separate product concepts. Both TriggerRefreshEvent and TriggerLeaveEvent now map to the same lifecycle action: commit to cooperative consumption via PendingForfeitState. - Collapse handleTriggerRefresh and handleTriggerLeave into a single handleExternalForfeitTrigger method - Remove LeaveRequest outbox message (leave vs refresh is a round/wallet concern, not a VTXO lifecycle concept)
Remove the direct RoundActor reference from VTXOActorConfig. The VTXO actor now routes ForfeitRequest and ForfeitSignatureSubmission through the manager via RelayToRoundMsg. The manager unwraps and forwards to the round actor. ChainResolver remains as a direct reference on the VTXO actor since it is not yet wired up and keeping it direct is simpler for now. - Add RelayToRoundMsg manager message type - Add tellManager helper on VTXOActor for consolidated outbound routing - Add handleRelayToRound handler on Manager - Remove RoundActor from VTXOActorConfig - Update tests to verify relay through manager
Document and test the liveness guarantee: when a VTXO approaches expiry, the VTXO actor autonomously emits a ForfeitRequest through the manager without requiring wallet intervention. The manager relays it promptly to the round actor, ensuring cooperative action is always attempted before critical expiry. - Add liveness policy comment on handleRelayToRound - Add TestManagerRelayToRound proving forfeit requests reach the round actor via the manager relay path - Add TestManagerRelayForfeitSig for signature submission
Remove the RefreshAcknowledgedEvent from the VTXO FSM and the round actor. This event was a no-op acknowledgment sent from the round actor back to the VTXO actor after queuing a refresh, adding complexity without value. The VTXO actor in PendingForfeitState simply waits for the ForfeitRequestEvent with concrete forfeit details. - Remove RefreshAcknowledgedEvent type definition from round/vtxo_messages.go - Remove sending code from round/actor.go - Remove handler from PendingForfeitState.ProcessEvent - Remove type alias from vtxo/events.go
Update comments throughout the VTXO package to reflect the refactored architecture. The VTXO FSM no longer distinguishes between refresh and leave as separate concepts — both are cooperative forfeiture from the FSM's perspective. - Replace "refresh" terminology with "cooperative forfeit" in state transition comments - Remove product-level leave/refresh distinction from ForfeitedState documentation - Update MessageSpec comments for PendingForfeitState - Update ForfeitRequest and manager comment wording
Replace TriggerRefreshEvent and TriggerLeaveEvent with a single PendingForfeitEvent that carries no business intent. The round actor now owns intent composition: it loads VTXO descriptors, builds the IntentPackage (refresh or leave), feeds the FSM, and sends PendingForfeitEvent to mark each VTXO as pending cooperative consumption. The VTXO FSM no longer handles TriggerRefreshEvent or TriggerLeaveEvent. It accepts PendingForfeitEvent to transition Live → PendingForfeit, with a no-op handler for duplicate PendingForfeitEvent in PendingForfeitState. This completes the Phase A goal: the VTXO actor speaks only lifecycle, not business intent. Phase B will move intent composition from the round actor to the wallet.
Add RegisterIntentRequest as the primary entry point for registering pre-composed intent packages with the round actor. The caller (wallet) builds the full IntentPackage and the round actor validates, registers it with the FSM, and notifies affected VTXO actors with PendingForfeitEvent. The handler derives forfeited outpoints from the package rather than accepting them separately, and only sends PendingForfeitEvent after FSM registration succeeds. If a VTXO notification fails, the handler logs and continues — the missed VTXO will receive the concrete ForfeitRequestEvent later via the fast path. The existing RefreshVTXORequest and LeaveVTXORequest paths remain active until callers are switched to RegisterIntentRequest in subsequent commits.
Move intent composition for VTXO refresh from the round actor to the wallet. The wallet now loads VTXO descriptors via a VTXOReader interface, builds forfeit + VTXO request pairs, and sends a RegisterIntentMsg to the round actor. The round actor converts this to its internal RegisterIntentRequest and handles FSM registration and VTXO actor notification. This is the key boundary shift: the wallet owns intent composition, the round only registers and validates. The leave flow still uses the old TriggerVTXOLeaveMsg path (next commit).
Mirror the refresh flow change: the wallet now loads VTXO descriptors, builds forfeit + leave request pairs, and sends a RegisterIntentMsg to the round actor instead of TriggerVTXOLeaveMsg. Both wallet flows now use the same intent registration path.
Remove TriggerVTXORefreshMsg, TriggerVTXOLeaveMsg from actormsg and their corresponding round actor handlers (handleTriggerVTXORefresh, handleTriggerVTXOLeave). Also remove buildRefreshVTXORequest helper whose only caller was the deleted refresh handler. The wallet now sends RegisterIntentMsg directly, making these trigger messages and their round-side intent composition logic dead code.
Remove LeaveVTXORequest type, its Receive case, and the handleLeaveVTXORequest handler. No external callers send this message since the wallet now uses RegisterIntentMsg for leaves. Also remove buildRefreshVTXORequest whose only caller was deleted in the previous commit. Update tests to use RegisterIntentRequest with forfeit + leave pairs instead of the removed LeaveVTXORequest.
Keep local amount metadata alongside forfeits when the wallet or the auto-expiry refresh path builds a round intent. The new RegisterIntent flow validates forfeited input value during round registration. Relying on store lookups there made the refresh systest fragile because the short-lived actor request context could be canceled before those reads finished. Using locally available amounts keeps validation deterministic and makes the refresh and leave paths independent from a store round-trip.
61f031b to
b3ef24e
Compare
61f031b to
a15ea97
Compare
Run `make rpc` to regenerate protobuf Go stubs so the raw descriptor bytes match the renamed VTXO_STATUS_PENDING_FORFEIT and VTXO_STATUS_UNILATERAL_EXIT enum values. The previous manual sed renames updated Go-level maps but left the binary-encoded FileDescriptorProto with stale length prefixes, causing a panic at init time. The regeneration also adds ListRounds and WatchRounds to the mailbox server interface. Since WatchRounds is a server-streaming RPC incompatible with the unary mailbox transport, introduce an rpcMailboxAdapter that wraps RPCServer and returns an error for WatchRounds over mailbox.
a15ea97 to
696ae8b
Compare
…edup Replace findPendingRound with findAssemblingRound in handleRegisterIntent and handleRefreshVTXORequest. findPendingRound matches by temp-key status which includes rounds in RegistrationSentState. Feeding an IntentPackage to RegistrationSentState causes a silent self-loop, discarding the intent without returning an error. findAssemblingRound correctly filters by FSM state (Idle or PendingRoundAssembly). Also add PkScript-based deduplication for VTXO requests in PendingRoundAssembly.ProcessEvent. Two refresh paths (wallet-driven and auto-expiry) could race to create output requests for the same VTXO. The forfeit pool already deduplicates by outpoint, but duplicate VTXO outputs would inflate totalOutput and cause the balance check to fail.
… embedded Change computeTotalForfeitAmount to always look up the canonical VTXO amount from VTXOStore when a store is available. Previously, a non-zero embedded Amount field on ForfeitRequest would skip the store lookup entirely, allowing a buggy or compromised caller to inflate the forfeit total. The embedded Amount is now only used as a fallback when no store is provided (test environments). Also update the Forfeits field comment in Intents to reflect that the store is the canonical source of truth, and remove the stale fast-path reference from the sortedForfeitRequests comment.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Solid refactor
Found a few things locally and regenerated the docs, will tack on as extra commits.
| ctx, &TriggerRefreshEvent{ | ||
| ForceRefresh: cmd.ForceRefresh, | ||
| }, | ||
| err = serviceKey.Ref(a.cfg.ActorSystem).Tell( |
There was a problem hiding this comment.
So the round actor speaks directly to the vtxo actors, but the vtxo actor relays through the round actor?
Why not make this symmetric? On either end of the relationship. One other thing to think about here is: which of these actors will be made durable? Right now we have plas to make the round actor durable, so it can survive restarts when receiving messages from the wallet/server.
| case *ForfeitSignatureResponse: | ||
| return a.handleForfeitSignatureResponse(ctx, m) | ||
|
|
||
| case *actormsg.TriggerVTXORefreshMsg: |
There was a problem hiding this comment.
👍 for removing these, was working in this area earlier in the week and was a bit confused re why we had to very similar messages sent into the actor.
Review FixesPushed three commits addressing findings from code review:
|
Update round, vtxo, and wallet CLAUDE.md/AGENTS.md to reflect the new message types and FSM states from the VTXO FSM purification: - round: Add PendingForfeitEvent, RegisterIntentMsg, IntentPackage to message flows. Remove RefreshAcknowledgedEvent and LeaveVTXORequest. - vtxo: Rename states (PendingForfeit, UnilateralExit). Remove TriggerRefreshEvent/TriggerLeaveEvent from receives. Add PendingForfeitEvent, RelayToRoundMsg. - wallet: Document intent composition responsibility. Replace TriggerRefreshEvent/TriggerLeaveEvent sends with RegisterIntentMsg. - ARCHITECTURE.md: Update VTXO FSM state diagram with new state names and the fast-path ForfeitRequestEvent transition.
a459522 to
a421b04
Compare
Update the client submodule pointer to include the merged VTXO FSM refactor (PR #172). This replaces RefreshRequestedState with PendingForfeitState, renames ExpiringState to UnilateralExitState, removes trigger messages, and routes all round-bound signals through the VTXO manager. Fix systest compilation: wire a VTXOReader adapter (closure over vtxoStore) into wallet.NewArk so the wallet can load VTXO descriptors for intent composition, and replace the removed VTXOStatusRefreshRequested constant with VTXOStatusPendingForfeit.
2 participants
Refactor groundwork for VTXO coin selection (#150).
This PR simplifies the boundary between
vtxo,round, andwalletso upcoming work on FSM-based locking (#168) and directed in-round
sends can build on a cleaner ownership model.
At a high level:
instead of assembling them itself.
No protocol change is intended here. This is a structural refactor that
removes overlapping responsibilities, dead message paths, and context
lifetime bugs.
Why
Before this change, intent ownership was split across multiple layers:
That made the architecture harder to extend for coin selection and
locking, because the system did not have a single clear owner for
"which VTXOs are being consumed, and for what purpose".
This PR makes that ownership explicit:
walletdecides what intent package to register.rounddrives protocol execution for that package.vtxotracks availability and lifecycle while remaining agnostic towhether a cooperative consume is a refresh, leave, or later another
flow.
What changed
1. VTXO FSM now models lifecycle, not business intent
Refresh and leave were previously represented as separate concepts in
the VTXO FSM even though, from the VTXO actor's perspective, both are
just cooperative forfeits.
This PR simplifies that model:
RefreshRequestedState->PendingForfeitStateExpiringState->UnilateralExitStateTriggerRefreshEventandTriggerLeaveEventcollapse intoPendingForfeitEventRefreshAcknowledgedEventis removed as dead/no-op state machineryThis leaves the VTXO FSM focused on lifecycle transitions:
2. Round-bound VTXO signals now route through the manager
VTXO actors no longer hold a direct round actor reference.
Instead:
This keeps VTXO actors simpler and makes outbound coordination flow
through one place.
3. Wallet now owns intent composition
The biggest architectural change is that the wallet now builds the full
intent package for refresh and leave flows.
That means:
RegisterIntentRequest/RegisterIntentMsgThe round actor no longer needs separate wallet-trigger paths for
refresh/leave intent assembly.
4. Forfeit amounts are carried locally in intents
Forfeit amounts are now carried in the local intent data used during
registration.
This removes fragile registration-time store lookups and makes amount
validation more direct when building the round request.
The amount remains local bookkeeping, not a wire-level protocol change.
5. Persistence work no longer depends on request-context lifetime
Several local persistence operations now use
context.WithoutCancelwhen the work must survive beyond the actor request that triggered it.
This applies to operations like:
triggering request context expired
Diagrams
Ownership after this PR
VTXO lifecycle before vs after
Before, the FSM encoded refresh/leave intent explicitly.
After, it models only the lifecycle of cooperative consumption.
Result
After this PR:
selection work
Test plan
make unit pkg=vtxomake unit pkg=roundmake unit pkg=walletmake buildmake lint