draft: UTXO reservation wallet-side foundations - #4238
Draft
mswilkison wants to merge 22 commits into
Draft
Conversation
Companion of the tbtc-v2 UTXO reservation draft (threshold-network/ tbtc-v2#1088). A reservation is a deposit the wallet anchors -- spends in a 1-input-1-output transaction into a fresh wallet-controlled output with no refund path -- instead of sweeping, so the reserved coins never commingle with the pooled supply and are redeemable in-kind. Adds the wallet-side foundations: - wallet action types for the four reservation lifecycle actions (anchor, reserved redemption, re-anchor, dissolution), appended after the existing enum values to preserve serialized compatibility, - coordination proposal types with marshaling and factory registration (JSON-based for now; switching to protobuf once the reservation message types are added to the coordination proto definition), - Chain interface extensions for reading reservations and parameters and validating the four proposal kinds via WalletProposalValidator, - unsigned transaction assembly for all four lifecycle shapes, enforcing the 1-input-1-output lineage (dissolution additionally spends the wallet main UTXO as its second input, per the Bridge rules), - tests for action parsing, proposal marshaling roundtrips, and assembler input validation. The Ethereum chain implementation stubs the new interface methods with descriptive errors: the contract bindings can only be regenerated once the reservation Bridge API is published with the @keep-network/tbtc-v2 package. Coordination executor wiring and tbtcpg proposal generation follow in the same step.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Repairs a pre-existing build break on main: the tbtcpg Chain interface was refactored to return tbtc.RedemptionParameters as a struct, but the fee-estimation call site in redemptions.go still destructured the old 8-value tuple. All other call sites already use the struct form.
…through chain Addresses confirmed findings from a multi-agent review of the reservation wallet-side foundations: - assembleReservationAnchorTransaction/assembleReservationReanchorTransaction now take an action snapshot and enforce the action's TxMaxFee ceiling and the reservation minimum amount floor, mirroring the redemption/dissolution assemblers. - assembleReservedRedemptionTransaction and assembleReservationReanchorTransaction enforce the reservation minimum amount floor on their remainder/re-anchor outputs. - computeReservationRedeemerOutputScriptHash moved off pkg/tbtc (a host-chain-agnostic package) onto BridgeChain.ComputeReservationRedeemerOutputScriptHash, matching how ComputeMainUtxoHash is already delegated through the chain abstraction; the Ethereum implementation shares its keccak step with buildRedemptionKey. - assembleReservationDissolutionTransaction rejects a wallet main UTXO that wasn't part of the action's snapshot instead of silently discarding it, and requires a bridge chain unconditionally; its bridgeChain parameter is now a narrow inline interface instead of the full BridgeChain. - ReservationReanchorProposal.Unmarshal rejects a zero target wallet public key hash; ReservationAnchorProposal.Unmarshal rejects a zero deposit funding tx hash; all four proposal Unmarshal methods reject a non-positive or out-of-int64-range fee. - ReservedRedemptionProposal carries its redeemer output script on the wire (previously unconstructible - no source supplied it). - GetReservation/GetReservationAction/ReservationParameters moved from WalletProposalValidatorChain to BridgeChain, matching their Bridge-state-read peers. - The 7 Ethereum reservation stubs return a wrapped sentinel error so callers can errors.Is() them; the mismatched dissolution localChain stub parameter name now matches its siblings. - Marshal/Unmarshal for the four reservation proposals moved to marshaling.go alongside the other proposal marshalers; the reservation validity-block constants and the action type/state enums gained rationale and per-value doc comments. Test coverage: happy-path and fee/value boundary tests for the anchor and re-anchor assemblers (previously untested beyond a nil-input guard), dissolution's action-amount/target-wallet mismatch checks, the fee-exceeds-redemption-amount and partial-amount-exceeds-anchor-value guards, and fuzz coverage for all four proposals' Unmarshal methods.
MetricName() and clientinfo.GetAllWalletActionTypes() are two hand-maintained lists that must stay in sync; this pins that invariant so drift fails the test suite instead of silently degrading metrics.
agent-docs/ holds review scratch output and should never be committed.
…ouble LocalChain (pkg/tbtcpg's Chain test double) was missing the reservation methods added to the BridgeChain/WalletProposalValidatorChain interfaces, breaking staticcheck's compile of pkg/tbtcpg's tests. Adds panic-stub implementations matching this file's existing convention for chain functionality its tests don't exercise (e.g. ComputeMainUtxoHash).
assembleReservationAnchorTransaction was missing the action.ActionType == Acceptance and action.State == Pending guards that its redemption/ dissolution siblings both have (and that F1's original fix called for by name). A stale or wrong-type action snapshot previously passed straight through to fee/value validation instead of being rejected up front.
…nding - delete the undeclared partial-redemption capability from assembleReservedRedemptionTransaction, restoring the strict 1-input-1-output shape this PR's own body and the companion contracts spec both claim - add the missing TargetWalletPublicKeyHash check to assembleReservationAnchorTransaction, matching the guard already enforced by its re-anchor and dissolution siblings - move ReservationParameters to parameters.go alongside its siblings, drop the field-name stutter, and drop ReservationTotalAmount in favor of a dedicated accessor - extract requireReservationAction/requireValidActionFee to remove the duplicated nil/type/state/fee guard blocks across all four assemblers - narrow assembleReservedRedemptionTransaction's bridgeChain parameter to the single method it calls, matching the dissolution assembler's existing convention - drop the interface-doc restatements on the four new proposal types' ActionType/ValidityBlocks methods - add missing boundary test coverage (zero/negative fee, zero anchor value, zero redemption amount, nil action) and a fundUtxo test helper to de-duplicate funding-transaction setup
- change GetReservation to the (*Reservation, bool, error) found-flag convention already used by GetPendingRedemptionRequest, resolving the doc contradiction with GetReservationAction's error-on-not-found convention - rename ReservationParameters() to GetReservationParameters(), returning by value like every sibling parameter getter, and add a dedicated GetReservationTotalAmount() accessor - rename the shared redemption-key hashing helper from the reservation-only reservationRedeemerOutputScriptHash to redeemerOutputScriptHash since it names a rule shared by both buildRedemptionKey and the new Compute method, and restore the length-prefix doc comment dropped during extraction - replace the %w-wrapped sentinel returns on the reservation stubs with direct returns, matching this file's existing stub convention - add test coverage for the seven reservation stub methods and the renamed redeemer-output-script-hash helper - clarify the source/target wallet split in ValidateReservationReanchorProposal's doc comment - restore the blank line before ComputeReservationRedeemerOutputScriptHash's doc comment, matching the interface's blank-line-between-methods convention
- remove the dead, verbatim-duplicate ReservationKey nil check in ReservationDissolutionProposal.Unmarshal - extract validateProposalNonceAndFee to remove the duplicated request-nonce/fee validation sequence across all four proposal Unmarshal methods - add deterministic malformed-input tests (truncated JSON, negative fee, zero nonce) for each of the four new proposal types, since the existing fuzz targets only seed one well-formed input and exercise nothing under plain go test
…t bound - drop the undisclosed agent-docs/ .gitignore addition, out of scope for this PR - derive TestWalletActionType_MetricNameConsistency's loop bound from ParseWalletActionType's own domain instead of a hardcoded upper bound, so a future action type added without a clientinfo entry can't silently pass
- reanchor: bind action.Amount and the anchor outpoint, closing the only lane with no value/outpoint authorization (P1) - anchor happy-path test: assert constructed inputs/outputs instead of just the returned error (P1) - anchor/dissolution: derive the destination script from the wallet's own public key instead of trusting an RPC-supplied hash, matching every other self-paying assembler in the package - redemption/dissolution/reanchor: thread the reservation's authoritative anchor outpoint through and reject on mismatch, instead of relying on a value-only check - enforce ReservationAction.TimeoutAt in requireReservationAction - reject an all-zero wallet/target public key hash in the anchor, reanchor, and dissolution assemblers to prevent a silent burn output - restore validateMemberIndex's uint32->uint8 overflow guard (marshaling.go), dropped as unrelated collateral in this PR; keep the new zero-check alongside it - range-check ReservationKey (sign, bit length) on the three JSON-unmarshaled reservation proposals - simplify a redundant nil||len() check in ReservedRedemptionProposal.Unmarshal - complete six truncated/missing doc comments on the new Ethereum chain reservation stubs; fix a misnamed and a misplaced comment - dissolution: use builder.TotalInputsValue() instead of a hand-rolled sum; document the wallet-action enum as append-only - add missing structural test coverage for reanchor/redemption inputs and five previously-untested fee/value boundary branches - note the dissolution input-order assumption against the unmerged tbtc-v2#1088 Bridge contract as a tracked TODO
… comments Silently deleted during the doc-comment fix for the reservation Ethereum chain stubs, breaking TbtcChain's tbtcpg.Chain interface implementation for FindDeposits/EstimateDepositsSweepFee/ NewProposalGenerator call sites in cmd/. Caught by CI (client-vet, client-scan), not by 'go build ./pkg/...' alone since cmd/ isn't under pkg/. Restored verbatim from the pre-fix commit.
EnsureWalletSyncedBetweenChains treated any 1-input-1-output transaction spending a revealed deposit as an unproven first deposit sweep and hard-errored. A reservation anchor transaction has the exact same shape but deliberately never becomes the wallet's main UTXO, permanently deadlocking wallet sync for any wallet holding a reservation anchor. Distinguish an anchor from a genuine unproven sweep by checking whether the spent deposit's vault matches the reservation vault (mirrors the existing sweep-vs-reservation check used elsewhere). Treat GetReservationParameters failing (not yet implemented on every chain backend) or an unset vault as "not a reservation" rather than propagating the error, so ordinary deposit sweeps are unaffected.
…rage - requireReservationAction now rejects a zero TimeoutAt as malformed instead of silently bypassing the timeout guard; adds test coverage for both the malformed-timeout and timed-out-action branches, which previously had none. - Adds missing test coverage for the anchor-outpoint-mismatch guard in the redemption and dissolution assemblers (only re-anchor was tested), the nil-outpoint guards in all three UTXO-consuming assemblers, the wallet-public-key nil guard in the anchor and dissolution assemblers, the reachable target-wallet-hash guard in the re-anchor assembler, and the 1-input (no main UTXO) dissolution success path. - Moves the unreachable TargetWalletPublicKeyHash zero-check in the anchor and dissolution assemblers ahead of the equality check it was shadowed by, so it provides real defense-in-depth. - Replaces the dissolution assembler's stale TODO and inline comment asserting an unverified anchor-first input-order requirement: the companion Bridge contract (threshold-network/tbtc-v2#1088) accepts either input order by matching outpoint hashes, not position. - Documents that the nonce-keyed GetReservationAction lookup and the terminal ReservationActionState values model the anticipated two-phase authorize-then-prove settlement redesign, not the currently-reviewed single-phase contract. - Removes a redundant action-type parse duplicate of TestParseWalletActionType and decorative scenario comments that only restated the assertion below them.
The four reservation proposal Unmarshal implementations decoded DepositFundingTxHash and TargetWalletPublicKeyHash into fixed-size byte arrays directly, so a wrong-length JSON array silently zero-fills or truncates instead of erroring (unlike the existing protobuf unmarshalers, which reject a bad length explicitly). Unmarshal into an intermediate []byte field first and reject a present-but-wrong-length value before copying into the fixed array; an absent field still falls through to the existing zero-value "required" check unchanged. Also closes the equivalent oversized-fee gap already covered for ReservationKey, and extends each proposal's protobuf-migration TODO with the sequencing constraint: it must land before any code that generates these proposals on the wire.
Documents the 0-means-disabled convention on the launch-throttle fields (MinAmount, MaxTotalAmount, MaxReservationsPerWallet), and replaces the repeated 'pending unpublished Bridge API' narrative on the eight Ethereum reservation stub methods with a concise statement of their current sentinel-error behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion of threshold-network/tbtc-v2#1088 (UTXO reservations: segregated custody with in-kind redemption). A reservation is a deposit the wallet anchors — a 1-input-1-output spend into a fresh wallet-controlled output with no refund path — instead of sweeping, so reserved coins never commingle with the pooled supply and are redeemable in-kind by their owner.
What's included
CoordinationProposal, registered in the unmarshaling factory. Marshaling is JSON-based for now with an explicit TODO — switching to protobuf requires adding the reservation message types to the coordination proto definition and regeneratingpkg/tbtc/gen/pb.GetReservation,GetReservationAction,GetReservationParameters,GetReservationTotalAmount, and the fourValidateReservation*Proposalmethods mapping onto the newWalletProposalValidatorviews from the contracts PR, plusComputeReservationRedeemerOutputScriptHash— implemented (not stubbed), since it needs no new ABI, only the existing keccak256 rule already used bybuildRedemptionKey. Note: the nonce-keyedGetReservationAction(reservationKey, requestNonce)lookup and the terminalReservationActionStatevalues (Settled/TimedOut/Vetoed/Superseded) model the anticipated two-phase authorize-then-prove settlement redesign tracked in tbtc-v2#1088's own review findings, not the currently-reviewed single-phase (request→prove) contract — this interface may change once the contracts PR's final shape lands.go test ./pkg/tbtc/passes in full.Deliberately deferred (and why)
TbtcChainstubs the new methods with descriptive errors. The generated contract bindings can only be regenerated once the reservation Bridge ABI is published with the@keep-network/tbtc-v2package — i.e., after the contracts PR merges.Note for maintainers
origin/maincurrently fails to build (pkg/tbtcpg/redemptions.go:225: the Chain interface was refactored to returntbtc.RedemptionParametersas a struct, but the fee-estimation call site still destructured the old 8-value tuple) — the Client workflow is red on the main tip as well. This PR carries the one-line repair as a separate labeled commit so CI can run green here; feel free to cherry-pick it to main independently of the reservation work.