rfqmsg+rfq: add execution policy to rfq negotiation#2049
rfqmsg+rfq: add execution policy to rfq negotiation#2049jtobin wants to merge 21 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.
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 introduces execution policy semantics and limit-order constraints to the Request-for-Quote (RFQ) negotiation process. By allowing users to specify policies like Fill-Or-Kill and define explicit price bounds and minimum fill amounts, the system provides more granular control over trade execution. These changes are implemented in an additive, backward-compatible manner, ensuring continued interoperability with existing peers while enhancing the robustness of the RFQ wire protocol and RPC surface. 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 and execution policies to the RFQ system, enabling minimum fill sizes, rate limits, and Fill-Or-Kill (FOK) policies across the RPC, internal, and wire layers. The implementation includes new validation logic in the portfolio pilot and comprehensive integration and property-based tests. Review feedback identified logic bugs in the minimum fill validation where zero amounts were incorrectly rejected and recommended treating zero values from wire messages as unset to improve safety. Suggestions were also provided to use math helper functions for more concise code initialization.
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.
jtobin
force-pushed
the
execution-policy
branch
2 times, most recently
from
8bca482 to
b20fcee
Compare
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.
1 participant
Depends on #2048. Partially resolves #2004 (most, though not all, of workstream C).
Adds execution policy semantics (immediate-or-cancel, fill-or-kill) to the RFQ wire protocol and RPC surface, which again, as in #2048, are here enforced at the quote acceptance level.
Feature TLDR:
Like the functionality added in #2048, the execution policy field is additive and optional; old peers that ignore the new TLV record continue to interoperate.
Here's an illustrative use of the feature, from an e2e regtest demo: