rfq+rfqmsg: add fill quantity negotiation and constraint validation#2050
Draft
jtobin wants to merge 30 commits intolightninglabs:mainfrom
Draft
rfq+rfqmsg: add fill quantity negotiation and constraint validation#2050jtobin wants to merge 30 commits intolightninglabs:mainfrom
jtobin wants to merge 30 commits intolightninglabs:mainfrom
Conversation
Add a new optional TLV field (Type 29, TlvFixedPoint) to requestWireMsgData for carrying rate limit constraints on quote requests. This field supports both buy requests (minimum acceptable rate) and sell requests (maximum acceptable rate). Wire encode/decode follows the existing InAssetRateHint pattern.
Add AssetMinAmt and AssetRateLimit to BuyRequest, and PaymentMinAmt and AssetRateLimit to SellRequest. Update constructors, FromWire extraction, Validate (min <= max, rate positive), String output, and wire bridging functions. Add corresponding fields to BuyOrder and SellOrder structs and thread them through the negotiator's outgoing order handlers. Existing callers pass fn.None for both new params.
Add min fill and rate limit fields to RFQ and portfolio pilot proto definitions. Update rpcserver unmarshalling, portfolio pilot RPC marshal/unmarshal functions, and add new reject codes and quote response status values.
Add checkRateBound and checkMinFill enforcement to the internal portfolio pilot's VerifyAcceptQuote. Rate bound checks that the accepted rate satisfies the requester's limit (floor for buy, ceiling for sell). Min fill checks that the minimum amount is transportable at the accepted rate. Also adds MinFillNotMetRejectCode, PriceBoundMissRejectCode, and corresponding QuoteRespStatus values.
Add typed roundtrip tests for BuyRequest and SellRequest covering min fill, rate limit, and backward-compatible no-optional-field cases. Add validation tests for min > max and zero rate limit. Add rapid-based property tests covering wire roundtrip, min/max constraint validation, and rate bound enforcement semantics for both buy and sell requests. Extend TestVerifyAcceptQuote with rate bound enforcement cases (buy below/at/above limit, sell above/at limit) and min fill cases (zero msat, transportable). Add dedicated unit tests for checkRateBound and checkMinFill helpers. Update wire-level roundtrip test to cover AssetRateLimit TLV 29.
Add Validate() to NewBuyRequest and NewSellRequest constructors so that invalid requests (e.g. min > max) are caught locally before being sent over the wire. Add testRfqLimitConstraints: a pure RFQ integration test with five sub-tests exercising buy/sell orders with satisfied and violated rate limits, plus client-side min > max rejection. Add testCustomChannelsLimitConstraints: a custom channels smoke test that negotiates a sell quote with asset_rate_limit and payment_min_amt constraints over a real asset channel, then pays an invoice using the pre-negotiated quote.
Harden rate limit validation to reject negative coefficients (not just zero), add default cases to checkRateBound/checkMinFill type switches, fix stale godoc on unmarshalOptionalFixedPoint, path- anchor the gitignore entry, rewrite TestRateBoundEnforcementProperty to exercise wire roundtrip code, and add TestNegativeRateLimitRejected.
Fix gofmt alignment in rfq_test.go and request_property_test.go. Fix line-length violations in portfolio_pilot.go, portfolio_pilot_test.go, and request_property_test.go. Remove PaymentMinAmt from rate-limit-focused sell sub-tests: at the test oracle rate (1000 units/BTC), 1000 msat converts to zero asset units, tripping the checkMinFill guard.
Introduce ExecutionPolicy (uint8) with IOC (0) and FOK (1) constants. Add TLV type alias at TlvType31 and wire it into requestWireMsgData Encode/Decode. Add the field to BuyRequest and SellRequest with constructor parameters, Validate() checks, and wire extraction.
Add ExecutionPolicy field to BuyOrder, SellOrder, AssetSalePolicy, and AssetPurchasePolicy. Thread the field from orders through the negotiator to NewBuyRequest/NewSellRequest. HTLC policy annotation is for future workstream D; no HTLC-level enforcement.
Add FOKNotViableRejectCode (4) and FOKNotViableQuoteRespStatus (7). Implement isFOK() and checkFOK() helpers that verify the full max amount is transportable at the accepted rate when FOK is set. Wire checkFOK into VerifyAcceptQuote after checkMinFill.
Add ExecutionPolicy enum (IOC=0, FOK=1) to rfq.proto with field 10 on AddAssetBuyOrderRequest and AddAssetSellOrderRequest. Regenerate proto Go code. Unmarshal FOK in unmarshalAssetBuyOrder/unmarshalAssetSellOrder; IOC/unset maps to fn.None (default behavior).
Add roundtrip tests for FOK and absent execution policy on both BuyRequest and SellRequest. Add Validate() rejection test for unknown policy values. Add optionalExecutionPolicyGen and requireOptExecPolicyEq helpers to property tests; include execution policy in all roundtrip property checks.
Add table-driven test exercising checkFOK across buy/sell with no policy, IOC, FOK viable, and FOK rounds-to-zero cases. Update all existing NewBuyRequest/NewSellRequest calls with the new execution policy parameter.
Add FOK sub-tests to testRfqLimitConstraints: buy FOK accepted, sell FOK accepted, buy FOK rejected (extreme rate), and IOC default accepted. Add FOK sell scenario to custom channels limit constraints test with a real asset channel payment.
Add FOK_NOT_VIABLE (7) to QuoteRespStatus enum in both rfqrpc and portfoliopilotrpc protos. Add mapping case in rpcUnmarshalQuoteRespStatus. Extract unmarshalExecutionPolicy helper that rejects unknown enum values instead of silently treating them as IOC. Add FOK cases to TestVerifyAcceptQuote for pipeline coverage. Annotate FOKNotViableRejectCode and ErrFOKNotViable as reserved for workstream D.
Add missing execPolicy argument to NewBuyRequest/NewSellRequest calls in request_property_test.go. Replace createAssetInvoice with regular BTC invoice in the FOK sell test, since Erin has no asset channel.
Use rfqmath.NewBigIntFixedPoint in checkFOK and checkMinFill instead of verbose struct literals. Treat zero values for min fill amounts as unset (fn.None) at the wire deserialization boundary, since 0 is semantically meaningless as a minimum. Update property test generators to start from 1, matching the wire roundtrip invariant.
Add an optional FillAmount field (TLV types 11/13) to BuyAccept and SellAccept, allowing the responder to propose a fill smaller than the requester's max. Normalise zero fill values to None on decode for consistency. Add RequestConstraints interface to unify access to rate limits, min/max amounts, and execution policy across buy and sell request types. Remove workstream D reservation annotations.
Add fill amount to ResolveResp so responders can propose a partial fill. Validate the fill against the requester's min fill and execution policy in VerifyAcceptQuote. Cap HTLC forwarding policies with the negotiated fill amount. Unify checkRateBound, checkFOK, and checkMinFill via the RequestConstraints interface and a shared amountIsTransportable helper, replacing per-type switches.
Add accepted_asset_amount / accepted_payment_amount to the AcceptedQuote RPC messages for buy and sell quotes. Add accepted_max_amount to the portfolio pilot's AcceptedQuote proto. Marshal fill amounts and AcceptedMaxAmount through the RPC layer.
Add accepted_max_amount column to rfq_accepted_quotes (migration 55). Persist and retrieve the negotiated max fill amount so HTLC policy caps survive restarts.
Add property tests for execution policy wire roundtrips. Add responder-side constraint rejection tests (rate bound, min fill, FOK) for both buy and sell requests. Add fill-vs-constraint validation tests in order_test.go. Add DB persistence test for AcceptedMaxAmount. Update property test generators to start from 1, matching the wire invariant that zero means unset.
Add sell-side FOK, IOC, and fill-cap integration tests to rfq_test.go. Add explicit IOC execution policy test coverage. Add FOK sell test to the custom channels limit_constraints test using a regular BTC invoice (Erin has no asset channel).
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 enhances the RFQ wire protocol by introducing advanced order constraints and negotiation capabilities. It allows for more flexible order execution through fill quantity negotiation and explicit execution policies, while also enabling stricter limit-order validation. These changes improve the robustness of asset channel operations by ensuring that quotes are only accepted if they meet the requester's specific requirements. 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. 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. 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 introduces limit-order constraints (rate limits, minimum fill amounts) and execution policies (IOC/FOK) to the RFQ and Portfolio Pilot services. It includes necessary RPC changes, database migrations, and comprehensive unit, property-based, and integration tests. I have filtered out the review comments as they were not actionable and instead served to validate the code changes, which is contrary to the instructions.
Add //nolint:lll to gofmt-aligned ExecutionPolicy lines in rfq_test.go. Replace createAssetInvoice with a regular BTC invoice in the IOC sell test, matching the FOK test pattern. Handle IncomingRejectQuoteEvent in AddAssetBuyOrder and AddAssetSellOrder so responder-side constraint rejections surface as errors instead of timing out. Bump sell sub-test payment amounts to values that convert to non-zero asset units at the test oracle rate (1000 units/BTC).
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
1 participant
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.
Depends on #2049. Partially resolves #2004 (remainder of workstream C, and essence of workstream D).
Adds fill quantity negotiation to the RFQ wire protocol, such that a responder can propose a fill amount smaller than a requester's maximum, with validation against other limit order constraints at the quote acceptance level. HTLC forwarding policies are capped at the (persisted) negotiated amount so that the channel enforces the agreed-upon limit as a max bound.
There's also some substantial refactoring done around the limit order constraint logic via the RequestConstraints interface.
Here's an illustrative use of the feature from an e2e regtest demo: