docs: add portfolio pilot architecture guide#2146
Conversation
Document the portfolio pilot subsystem as a one-stop reference for developers and agents building custom market-making, OTC, or exchange flows on top of the RFQ layer. The guide walks the PortfolioPilot interface and its two built-in implementations, the limit-order constraint model (rate bounds, min/max amounts, IOC/FOK execution policy, fill caps, and the unified RequestConstraints view), and the hint/bind/qualify price-query intents that distinguish indicative lookups from binding quotes. It also includes runnable Go sketches for a custom pilot: an in-memory order-book pilot, a circular-swap wrapper that coordinates two RFQ legs against a combined slippage tolerance, and a hedging pilot composition pattern. Sections on OTC desks and exchanges cover customer tiering via PriceOracleMetadata, spread/skew/inventory pricing, last-look mechanics, layering a continuous double auction onto the protocol, and reconciliation and audit requirements for any operator treating tapd as a settlement front end. The doc complements docs/rfq_architecture.md and points at the existing examples/basic-portfolio-pilot example as the shortest path from interface to running daemon.
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 a detailed architecture guide for the Portfolio Pilot subsystem, providing developers with a centralized reference for building custom market-making and exchange flows on top of the RFQ layer. The documentation clarifies the decision-making interface, protocol-enforced constraints, and integration patterns, facilitating the development of sophisticated trading strategies. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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 a comprehensive architecture document for the Portfolio Pilot, which serves as the pluggable decision layer for Taproot Asset RFQs. The document covers the interface, limit-order model, internal and RPC pilot implementations, and provides Go code sketches for custom pilots. The review feedback focuses on correcting several issues within these Go code sketches: a logical bug in the order book matching helper (bestOrderLocked) that fails to find the best rate when queried without a rate bound, corresponding signature mismatches in its callers, style guide violations regarding missing comments on exported methods, and an anti-pattern involving variable reassignment inside a closure in the hedging pilot sketch.
| } | ||
|
|
||
| cons := request.Constraints() | ||
| best := p.bestOrderLocked(assetID, side, cons.RateBoundCmp) |
There was a problem hiding this comment.
Update the call to bestOrderLocked to match the updated signature that eliminates the redundant rateBoundCmp parameter.
| best := p.bestOrderLocked(assetID, side, cons.RateBoundCmp) | |
| best := p.bestOrderLocked(assetID, side) |
| // bestOrderLocked returns the most aggressive live order on the given | ||
| // side that the caller can match against. Caller must hold p.mu. | ||
| func (p *OrderBookPilot) bestOrderLocked(id asset.ID, side bookSide, | ||
| rateBoundCmp int) *restingOrder { | ||
|
|
||
| var best *restingOrder | ||
| now := time.Now() | ||
| for _, o := range p.book[id] { | ||
| if o.Side != side || o.Available == 0 || o.Expires.Before(now) { | ||
| continue | ||
| } | ||
| if best == nil { | ||
| best = o | ||
| continue | ||
| } | ||
| // A bid (peer is buying from us) is better when the rate is | ||
| // lower; an ask (peer is selling to us) is better when the | ||
| // rate is higher. RateBoundCmp tells us which way to lean. | ||
| if rateBoundCmp < 0 && o.Rate.Cmp(best.Rate) < 0 { | ||
| best = o | ||
| } else if rateBoundCmp > 0 && o.Rate.Cmp(best.Rate) > 0 { | ||
| best = o | ||
| } | ||
| } | ||
| return best | ||
| } |
There was a problem hiding this comment.
In QueryAssetRates (line 766), bestOrderLocked is called with 0 as the rateBoundCmp argument. Because of this, both rateBoundCmp < 0 and rateBoundCmp > 0 evaluate to false, meaning the function never compares rates and simply returns the first non-expired order in the book instead of the actual best order.
Since side already uniquely determines whether we want a lower rate (bookBid) or a higher rate (bookAsk), we can eliminate the rateBoundCmp parameter entirely and determine the comparison direction directly from side.
// bestOrderLocked returns the most aggressive live order on the given
// side that the caller can match against. Caller must hold p.mu.
func (p *OrderBookPilot) bestOrderLocked(id asset.ID, side bookSide) *restingOrder {
var best *restingOrder
now := time.Now()
for _, o := range p.book[id] {
if o.Side != side || o.Available == 0 || o.Expires.Before(now) {
continue
}
if best == nil {
best = o
continue
}
// A bid (peer is buying from us) is better when the rate is
// lower; an ask (peer is selling to us) is better when the
// rate is higher.
if side == bookBid && o.Rate.Cmp(best.Rate) < 0 {
best = o
} else if side == bookAsk && o.Rate.Cmp(best.Rate) > 0 {
best = o
}
}
return best
}| func (p *OrderBookPilot) VerifyAcceptQuote(_ context.Context, | ||
| accept rfqmsg.Accept) (rfq.QuoteRespStatus, error) { |
There was a problem hiding this comment.
Exported functions/methods in Go sketches should have comments explaining their purpose and assumptions, starting with the function name as a complete sentence, to adhere to the repository style guide.
| func (p *OrderBookPilot) VerifyAcceptQuote(_ context.Context, | |
| accept rfqmsg.Accept) (rfq.QuoteRespStatus, error) { | |
| // VerifyAcceptQuote verifies that an accepted quote from a peer meets | |
| // acceptable conditions. | |
| func (p *OrderBookPilot) VerifyAcceptQuote(_ context.Context, | |
| accept rfqmsg.Accept) (rfq.QuoteRespStatus, error) { |
References
- Exported functions require detailed comments for the caller. Every function must be commented with its purpose and assumptions, starting with the function name as a complete sentence. (link)
| func (p *OrderBookPilot) QueryAssetRates(_ context.Context, | ||
| q rfq.AssetRateQuery) (rfqmsg.AssetRate, error) { |
There was a problem hiding this comment.
Exported functions/methods in Go sketches should have comments explaining their purpose and assumptions, starting with the function name as a complete sentence, to adhere to the repository style guide.
| func (p *OrderBookPilot) QueryAssetRates(_ context.Context, | |
| q rfq.AssetRateQuery) (rfqmsg.AssetRate, error) { | |
| // QueryAssetRates returns current asset rate information for a given asset | |
| // and direction. | |
| func (p *OrderBookPilot) QueryAssetRates(_ context.Context, | |
| q rfq.AssetRateQuery) (rfqmsg.AssetRate, error) { |
References
- Exported functions require detailed comments for the caller. Every function must be commented with its purpose and assumptions, starting with the function name as a complete sentence. (link)
| side = bookBid | ||
| } | ||
|
|
||
| best := p.bestOrderLocked(assetID, side, 0) |
There was a problem hiding this comment.
Update the call to bestOrderLocked to match the updated signature that eliminates the redundant rateBoundCmp parameter.
| best := p.bestOrderLocked(assetID, side, 0) | |
| best := p.bestOrderLocked(assetID, side) |
| ), nil | ||
| } | ||
|
|
||
| func (p *OrderBookPilot) Close() error { return nil } |
There was a problem hiding this comment.
Exported functions/methods in Go sketches should have comments explaining their purpose and assumptions, starting with the function name as a complete sentence, to adhere to the repository style guide.
| func (p *OrderBookPilot) Close() error { return nil } | |
| // Close releases any resources held by the order book pilot. | |
| func (p *OrderBookPilot) Close() error { return nil } |
References
- Exported functions require detailed comments for the caller. Every function must be commented with its purpose and assumptions, starting with the function name as a complete sentence. (link)
| // We're about to commit. Reserve hedging capacity before | ||
| // returning the accept. If reservation fails, kill the trade. | ||
| resp.WhenAccept(func(rate rfqmsg.AssetRate) { | ||
| if !p.hedger.Reserve(request, rate, resp.FillAmount()) { | ||
| resp = rfq.NewRejectResolveResp(rfqmsg.ErrUnknownReject) | ||
| } | ||
| }) | ||
| return resp, nil |
There was a problem hiding this comment.
Reassigning the captured resp variable inside the closure passed to WhenAccept to modify the outer function's return value is an anti-pattern and can be confusing. It is cleaner and safer to use a local boolean flag to track if the reservation failed, and then return the rejection response from the outer function scope.
| // We're about to commit. Reserve hedging capacity before | |
| // returning the accept. If reservation fails, kill the trade. | |
| resp.WhenAccept(func(rate rfqmsg.AssetRate) { | |
| if !p.hedger.Reserve(request, rate, resp.FillAmount()) { | |
| resp = rfq.NewRejectResolveResp(rfqmsg.ErrUnknownReject) | |
| } | |
| }) | |
| return resp, nil | |
| // We're about to commit. Reserve hedging capacity before | |
| // returning the accept. If reservation fails, kill the trade. | |
| var reserveFailed bool | |
| resp.WhenAccept(func(rate rfqmsg.AssetRate) { | |
| if !p.hedger.Reserve(request, rate, resp.FillAmount()) { | |
| reserveFailed = true | |
| } | |
| }) | |
| if reserveFailed { | |
| return rfq.NewRejectResolveResp(rfqmsg.ErrUnknownReject), nil | |
| } | |
| return resp, nil |
darioAnongba
left a comment
darioAnongba
left a comment
There was a problem hiding this comment.
worth a quick review by @jtobin as well.
| or buggy counterparty from getting away with a rate that violates a limit | ||
| the protocol promised to enforce. |
There was a problem hiding this comment.
Would be good to add a warning like:
If you ship a custom
RpcPortfolioPilot, the local tapd performs no independent verification of limit-order constraints. You must reproduce these checks in your pilot.
Somewhere for custom Pilots. We currently do not re-validate.
Note that I'm not personally fond of this behaviour as I think we could always validate even when using a custom pilot.
|
|
||
| For pilots that must run out of process, `RpcPortfolioPilot` is a thin | ||
| gRPC client. It uses URIs of the form `portfoliopilotrpc://host:port` and | ||
| dials with self-signed TLS (or insecure if explicitly requested). |
There was a problem hiding this comment.
I think custom portfolio pilots still have InsecureSkipVerify: true. So this statement is not true yet. We should implement TLS verification aligned with price oracle here.
cc: @jtobin
There was a problem hiding this comment.
Woah, yeah that's an oversight on my part if so. Will check (and patch).
4 participants
In this PR, we add a new architecture doc for the portfolio pilot
subsystem under
docs/. The doc is meant to serve as a one-stop referencefor devs (and agents) building custom market-making, OTC desks, or
exchange flows on top of the RFQ layer, in the same spirit as the existing
docs/rfq_architecture.md.
The portfolio pilot is the decision layer that sits between the RFQ
negotiator and the price oracle.
ResolveRequestdecides whether to takean inbound quote and at what rate,
VerifyAcceptQuoteis our last look atthe peer's accept, and
QueryAssetRatesis pure rate discovery. Theinterface is small but the policy surface it opens up is large, and the
existing source comments only get you partway.
What the doc covers
The doc walks the
PortfolioPilotinterface, the in-processInternalPortfolioPilot, and the gRPCRpcPortfolioPilot. From there itcovers the limit-order model the protocol enforces on every accept (rate
bounds, min/max amounts, IOC vs FOK execution policy, fill caps, and the
unified
RequestConstraintsview), and the hint/bind/qualify intentlifecycle that distinguishes indicative pricing from binding commitments.
There's also a section pinning down the vocabulary: the order vs request
vs accept vs rate distinction, the asset-units-per-BTC convention, the
perspective flip on incoming requests (a peer's
BuyRequestis a sellopportunity for us), and how
RequestConstraints.RateBoundCmpcollapsesbuy and sell into a single check.
Custom pilots
The doc includes Go sketches for non-trivial pilots:
against resting orders.
slippage tolerance.
moment of commitment.
These are illustrative, not production-ready (called out as such), but
they're complete enough that a pilot author can grow their own
implementation from there. They also point at
docs/examples/basic-portfolio-pilot
as the shortest path from interface to running daemon.
OTC and exchange considerations
Dedicated sections cover the things an OTC desk or an exchange operator
should think about before treating tapd as a market-making front-end:
customer tiering via
PriceOracleMetadata, spread/skew/inventory pricing,last-look mechanics (and the caveats around them), layering a continuous
double auction matching engine onto the pilot, reconciliation of internal
trade ledgers against RFQ IDs, and crash recovery / audit requirements.
Test plan