WIP: rfq: add internal portfolio pilot for buy requests only #1907
Conversation
Summary of ChangesHello @ffranr, 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 new architectural component, the Highlights
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
|
![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 PortfolioPilot interface to abstract the RFQ decision logic, with a default InternalPortfolioPilot implementation. This is a positive change for modularity. My review focuses on several critical correctness issues in the new implementation, including an interface signature mismatch, an empty error handling block, and incorrect usage of the fn.Either type which would prevent compilation. I've also provided suggestions for a few medium-severity issues like an unused import and a redundant function parameter to improve code quality.
rfq/negotiator.go
Outdated
| result, err := n.cfg.PortfolioPilot.ResolveBuyRequest( | ||
| ctx, request, | ||
| ) | ||
| if err != nil { | ||
| // Send a reject message to the peer. | ||
|
|
||
| } | ||
|
|
||
| if result.IsErr() { | ||
| n.cfg.ErrChan <- fmt.Errorf("resolve buy request: %w", | ||
| result.Err()) | ||
|
|
||
| msg := rfqmsg.NewReject( | ||
| request.Peer, request.ID, | ||
| rfqmsg.ErrUnknownReject, | ||
| request.Peer, request.ID, rfqmsg.ErrUnknownReject, | ||
| ) | ||
| sendOutgoingMsg(msg) | ||
| return | ||
| } | ||
|
|
||
| // Add an error to the error channel and return. | ||
| err = fmt.Errorf("failed to query sell price from "+ | ||
| "oracle: %w", err) | ||
| outcome, unpackErr := result.Unpack() | ||
| if unpackErr != nil { | ||
| n.cfg.ErrChan <- fmt.Errorf("resolve buy request: %w", | ||
| unpackErr) | ||
|
|
||
| msg := rfqmsg.NewReject( | ||
| request.Peer, request.ID, rfqmsg.ErrUnknownReject, | ||
| ) | ||
| sendOutgoingMsg(msg) | ||
| return | ||
| } | ||
|
|
||
| if outcome.IsRight() { | ||
| reason := rfqmsg.ErrUnknownReject | ||
| outcome.WhenRight(func(err rfqmsg.RejectErr) { | ||
| reason = err | ||
| }) | ||
|
|
||
| msg := rfqmsg.NewReject( | ||
| request.Peer, request.ID, reason, | ||
| ) | ||
| sendOutgoingMsg(msg) | ||
| return | ||
| } | ||
|
|
||
| var assetRate *rfqmsg.AssetRate | ||
| outcome.WhenLeft(func(rate rfqmsg.AssetRate) { | ||
| assetRate = &rate | ||
| }) | ||
| if assetRate == nil { | ||
| err := fmt.Errorf("resolve buy request: missing " + | ||
| "asset rate on accept decision") | ||
| n.cfg.ErrChan <- err | ||
|
|
||
| msg := rfqmsg.NewReject( | ||
| request.Peer, request.ID, | ||
| rfqmsg.ErrUnknownReject, | ||
| ) | ||
| sendOutgoingMsg(msg) | ||
| return | ||
| } |
There was a problem hiding this comment.
This block has several critical issues:
- The
if err != nilblock is empty, which means any internal errors fromResolveBuyRequestare silently ignored. - The code attempts to use methods like
IsErr(),Err(), andUnpack()on theresultvariable, which is of typefn.Either. These methods do not exist onfn.Either, which will cause a compilation error.
The logic for handling the (fn.Either, error) return from ResolveBuyRequest should be corrected to properly handle internal errors and correctly unpack the Either type to process accept/reject decisions.
result, err := n.cfg.PortfolioPilot.ResolveBuyRequest(
ctx, request,
)
if err != nil {
// This is an internal error that occurred within the
// portfolio pilot. We'll send a generic rejection to
// the peer and log the actual error.
n.cfg.ErrChan <- fmt.Errorf("resolve buy request: %w", err)
msg := rfqmsg.NewReject(
request.Peer, request.ID, rfqmsg.ErrUnknownReject,
)
sendOutgoingMsg(msg)
return
}
// The portfolio pilot has made a decision. This can either be to
// accept the request with a given asset rate, or to reject it
// with a specific error.
var assetRate *rfqmsg.AssetRate
result.WhenLeft(func(rate rfqmsg.AssetRate) {
assetRate = &rate
})
result.WhenRight(func(rejectErr rfqmsg.RejectErr) {
msg := rfqmsg.NewReject(
request.Peer, request.ID, rejectErr,
)
sendOutgoingMsg(msg)
})
// If we have a rejection, we'll exit now.
if result.IsRight() {
return
}
// If we don't have an asset rate, then something has gone
// wrong.
if assetRate == nil {
err := fmt.Errorf("resolve buy request: missing " +
"asset rate on accept decision")
n.cfg.ErrChan <- err
msg := rfqmsg.NewReject(
request.Peer, request.ID,
rfqmsg.ErrUnknownReject,
)
sendOutgoingMsg(msg)
return
}
rfq/portfolio_pilot.go
Outdated
| type PortfolioPilot interface { | ||
| // ResolveBuyRequest applies the portfolio pilot's decision logic for a | ||
| // BuyRequest. | ||
| ResolveBuyRequest(context.Context, rfqmsg.BuyRequest) ResolveRequest |
There was a problem hiding this comment.
The signature of ResolveBuyRequest in the PortfolioPilot interface does not match its implementation in InternalPortfolioPilot. The implementation returns (ResolveRequest, error), but the interface only specifies ResolveRequest. The caller in negotiator.go expects the (ResolveRequest, error) signature. To fix this, the interface definition should be updated to match the implementation.
| ResolveBuyRequest(context.Context, rfqmsg.BuyRequest) ResolveRequest | |
| ResolveBuyRequest(context.Context, rfqmsg.BuyRequest) (ResolveRequest, error) |
rfq/negotiator.go
Outdated
| "github.com/lightninglabs/taproot-assets/fn" | ||
| "github.com/lightninglabs/taproot-assets/rfqmath" | ||
| "github.com/lightninglabs/taproot-assets/rfqmsg" | ||
| lfn "github.com/lightningnetwork/lnd/fn/v2" |
There was a problem hiding this comment.
The imported package lfn is not used in this file and should be removed.
| cfg.PortfolioPilot = NewInternalPortfolioPilot( | ||
| cfg.PriceOracle, cfgPortfolioPilot, | ||
| ) |
There was a problem hiding this comment.
The NewInternalPortfolioPilot function is being updated to remove a redundant parameter. The call to it here should be updated accordingly by removing the first argument.
| cfg.PortfolioPilot = NewInternalPortfolioPilot( | |
| cfg.PriceOracle, cfgPortfolioPilot, | |
| ) | |
| cfg.PortfolioPilot = NewInternalPortfolioPilot( | |
| cfgPortfolioPilot, | |
| ) |
rfq/portfolio_pilot.go
Outdated
| func NewInternalPortfolioPilot(_ PriceOracle, | ||
| cfg InternalPortfolioPilotConfig) *InternalPortfolioPilot { |
There was a problem hiding this comment.
The first parameter _ PriceOracle is unused within this function. The PriceOracle is already available in the cfg struct. This redundant parameter should be removed for clarity. The call site in negotiator.go will also need to be updated.
func NewInternalPortfolioPilot(cfg InternalPortfolioPilotConfig) *InternalPortfolioPilot {
Replace direct goroutine spawning and WaitGroup usage with a utility wrapper for error handling. This improves code clarity and error propagation.
ffranr
force-pushed
the
wip/portfolio-pilot/add-ResolveBuyRequest
branch
2 times, most recently
from
1ba9497 to
5f52053
Compare
Pull Request Test Coverage Report for Build 20083463873Details
💛 - Coveralls |
- Add InternalPortfolioPilot class. - Support resolving buy requests only. Additional functionality will be implemented later.
- Add PortfolioPilot to Negotiator and Manager structs. - Use internal implementation of PortfolioPilot if not specified.
- Remove HasAssetSellOffer method. - Delegate buy quote request handling to PortfolioPilot. - Improve error management and response handling.
ffranr
force-pushed
the
wip/portfolio-pilot/add-ResolveBuyRequest
branch
from
5f52053 to
7fea652
Compare
github-project-automation
bot
moved this from 🆕 New
to ✅ Done
in Taproot-Assets Project Board
InternalPortfolioPilotwith config (price oracle, tolerance, peer forwarding) andResolveBuyRequest.InternalPortfolioPilotenforces rate tolerance using the configuredAcceptPriceDeviationPpmand optional peer forwarding to oracle.