Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support user-generated RFQ on tchrpc.SendPayment #1224

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

GeorgeTsagk
Copy link
Member

Closes #1215

@GeorgeTsagk GeorgeTsagk self-assigned this Nov 28, 2024
@coveralls
Copy link

coveralls commented Nov 28, 2024

Pull Request Test Coverage Report for Build 12074134958

Details

  • 0 of 16 (0.0%) changed or added relevant lines in 1 file are covered.
  • 17 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.01%) to 40.869%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 16 0.0%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
asset/mock.go 3 92.2%
universe/interface.go 4 51.95%
commitment/tap.go 4 83.64%
asset/asset.go 4 80.96%
Totals Coverage Status
Change from base Build 12057455142: -0.01%
Covered Lines: 25787
Relevant Lines: 63096

💛 - Coveralls

rpcserver.go Outdated
Comment on lines 7038 to 7044
var quote rfqmsg.BuyAccept
for id, q := range r.cfg.RfqManager.PeerAcceptedBuyQuotes() {
if id == rfqmsg.SerialisedScid(req.Scid) {
quote = q
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the quote isn't found?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case we don't want to protect the user, we're assuming they know what they're doing

code wise we should fail the RPC

Comment on lines +125 to +128
// The rfq scid to use for this payment. If the user sets this value then
// the payment will immediately be dispatched, skipping the rfq negotiation
// phase, and using the following rfq scid.
uint64 scid = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this skip RFQ completely or just override scid? Where does the asset rate come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's supposed to skip the entire RFQ negotiation phase that is happening in the case statement below

if the user is using this option then they have negotiated a quote already, outside of the RPC

so all that's needed now is for us to map the scid to the full quote and use that in the outgoing htlc wire records

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll instead want the full 32-byte RFQ ID to be passed in. Then we can validate it exists and hasn't expired yet. If all is okay, we can easily derive the SCID from it.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Simple enough approach. Have two suggestions though, otherwise looks good, thank you 🎉

Comment on lines +125 to +128
// The rfq scid to use for this payment. If the user sets this value then
// the payment will immediately be dispatched, skipping the rfq negotiation
// phase, and using the following rfq scid.
uint64 scid = 5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll instead want the full 32-byte RFQ ID to be passed in. Then we can validate it exists and hasn't expired yet. If all is okay, we can easily derive the SCID from it.

}
}

htlc := rfqmsg.NewHtlc(nil, fn.Some(quote.ID))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do this part as well to remain RPC compatible:

		// Send out the information about the quote on the stream.
		err = stream.Send(&tchrpc.SendPaymentResponse{
			Result: &tchrpc.SendPaymentResponse_AcceptedSellOrder{
				AcceptedSellOrder: quote,
			},
		})
		if err != nil {
			return err
		}

		// Unmarshall the accepted quote's asset rate.
		assetRate, err := rfqrpc.UnmarshalFixedPoint(
			acceptedQuote.BidAssetRate,
		)
		if err != nil {
			return fmt.Errorf("error unmarshalling asset rate: %w",
				err)
		}

		// Calculate the equivalent asset units for the given invoice
		// amount based on the asset-to-BTC conversion rate.
		numAssetUnits := rfqmath.MilliSatoshiToUnits(
			*invoice.MilliSat, *assetRate,
		)

		rpcsLog.Infof("Using quote for %v asset units at %v asset/BTC "+
			"from peer %x with SCID %d", numAssetUnits, assetRate,
			peerPubKey, acceptedQuote.Scid)

		var rfqID rfqmsg.ID
		copy(rfqID[:], acceptedQuote.Id)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

[feature]: Allow specifying a custom RFQ scid on tchrpc.SendPayment
4 participants