Merge pull request #1322 from lightninglabs/conditional-records

rfqmsg: detect asset related custom records correctly

itest/channels_test.go

@@ -35,7 +35,7 @@ func testChannelRPCs(t *harnessTest) {
 		},
 	)
 	require.NoError(t.t, err)
-	require.Len(t.t, encodeResp.CustomRecords, 2)
+	require.Len(t.t, encodeResp.CustomRecords, 1)
 
 	var rfqIdType rfqmsg.HtlcRfqIDType
 	rfqIdTlvTypeNumber := uint64(rfqIdType.TypeVal())

rfqmsg/records.go

@@ -110,8 +110,11 @@ func (h *Htlc) SumAssetBalance(assetSpecifier asset.Specifier) (rfqmath.BigInt,
 
 // Records returns the records that make up the Htlc.
 func (h *Htlc) Records() []tlv.Record {
-	records := []tlv.Record{
-		h.Amounts.Record(),
+	var records []tlv.Record
+
+	// Don't encode the asset amounts if there are none.
+	if len(h.Amounts.Val.Balances) > 0 {
+		records = append(records, h.Amounts.Record())
 	}
 
 	h.RfqID.WhenSome(func(r tlv.RecordT[HtlcRfqIDType, ID]) {
@@ -214,6 +217,26 @@ func HtlcFromCustomRecords(records lnwire.CustomRecords) (*Htlc, error) {
 	return DecodeHtlc(encoded)
 }
 
+// HasAssetHTLCCustomRecords returns true if the given custom records contain
+// the custom records that we'd expect an asset HTLC to carry.
+func HasAssetHTLCCustomRecords(records lnwire.CustomRecords) bool {
+	var (
+		amountType HtlcAmountRecordType
+		rfqIDType  HtlcRfqIDType
+	)
+	for key := range records {
+		if key == uint64(amountType.TypeVal()) {
+			return true
+		}
+
+		if key == uint64(rfqIDType.TypeVal()) {
+			return true
+		}
+	}
+
+	return false
+}
+
 // AssetBalance is a record that represents the amount of an asset that is
 // being transferred or is available to be spent.
 type AssetBalance struct {

rfqmsg/records_test.go

@@ -8,13 +8,15 @@ import (
 	"github.com/lightninglabs/taproot-assets/asset"
 	"github.com/lightninglabs/taproot-assets/fn"
 	"github.com/lightninglabs/taproot-assets/rfqmath"
+	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/stretchr/testify/require"
 )
 
 type htlcTestCase struct {
-	name         string
-	htlc         *Htlc
-	expectedJSON string
+	name          string
+	htlc          *Htlc
+	expectedJSON  string
+	expectRecords bool
 
 	// sumBalances is a map of asset ID to the expected sum of balances for
 	// that asset in the HTLC.
@@ -29,6 +31,14 @@ func assetHtlcTestCase(t *testing.T, tc htlcTestCase) {
 	err := tc.htlc.Encode(&b)
 	require.NoError(t, err)
 
+	asCustomRecords, err := lnwire.ParseCustomRecords(b.Bytes())
+	require.NoError(t, err)
+
+	require.Equal(
+		t, tc.expectRecords,
+		HasAssetHTLCCustomRecords(asCustomRecords),
+	)
+
 	deserializedHtlc := &Htlc{}
 	err = deserializedHtlc.Decode(&b)
 	require.NoError(t, err)
@@ -78,6 +88,7 @@ func TestHtlc(t *testing.T) {
 			htlc: NewHtlc([]*AssetBalance{
 				NewAssetBalance([32]byte{1}, 1000),
 			}, fn.None[ID]()),
+			expectRecords: true,
 			//nolint:lll
 			expectedJSON: `{
   "balances": [
@@ -100,13 +111,15 @@ func TestHtlc(t *testing.T) {
 				[32]byte{1}: rfqmath.NewBigIntFromUint64(3000),
 				[32]byte{2}: rfqmath.NewBigIntFromUint64(5000),
 			},
+			expectRecords: true,
 		},
 		{
 			name: "channel with multiple balance assets",
 			htlc: NewHtlc([]*AssetBalance{
 				NewAssetBalance([32]byte{1}, 1000),
 				NewAssetBalance([32]byte{2}, 2000),
 			}, fn.Some(ID{0, 1, 2, 3, 4, 5, 6, 7})),
+			expectRecords: true,
 			//nolint:lll
 			expectedJSON: `{
   "balances": [

tapchannel/aux_invoice_manager.go

@@ -151,37 +151,41 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(_ context.Context,
 	log.Debugf("Received wire custom records: %v",
 		limitSpewer.Sdump(req.WireCustomRecords))
 
-	// No custom record on the HTLC, so we have nothing to do.
-	if len(req.WireCustomRecords) == 0 {
-		// If there's no wire custom records and the invoice is an asset
-		// invoice do not settle the invoice. Since we are asking for
-		// assets in the invoice, we may not let this HTLC go through
-		// as it is not carrying assets. This could lead to undesired
-		// behavior where the asset invoice may be settled by accepting
-		// sats instead of assets.
+	// Check if any strict forwarding rules need to be applied. Strict
+	// forwarding means that we want assets for asset invoices and sats for
+	// BTC invoices, and no mixing of the two.
+	switch {
+	// No asset custom records on the HTLC, check that we're not expecting
+	// assets.
+	case !rfqmsg.HasAssetHTLCCustomRecords(req.WireCustomRecords):
+		// If there's no asset wire custom records but the invoice is an
+		// asset invoice, do not settle the invoice. Since we are asking
+		// for assets in the invoice, we may not let this HTLC go
+		// through as it is not carrying assets. This could lead to
+		// undesired behavior where the asset invoice may be settled by
+		// accepting sats instead of assets.
 		//
 		// TODO(george): Strict-forwarding could be configurable?
 		if isAssetInvoice(req.Invoice, s) {
 			resp.CancelSet = true
 		}
 
 		return resp, nil
-	} else if !isAssetInvoice(req.Invoice, s) && !req.Invoice.IsKeysend {
+
+	// We have custom records, but the invoice is not an asset invoice.
+	case !isAssetInvoice(req.Invoice, s) && !req.Invoice.IsKeysend:
 		// If we do have custom records, but the invoice does not
 		// correspond to an asset invoice, we do not settle the invoice.
 		// Since we requested btc we should be receiving btc.
 		resp.CancelSet = true
 
 		return resp, nil
-	}
 
-	htlcBlob, err := req.WireCustomRecords.Serialize()
-	if err != nil {
-		return nil, fmt.Errorf("error serializing custom records: %w",
-			err)
+	default:
+		// No strict forwarding rule violation, continue below.
 	}
 
-	htlc, err := rfqmsg.DecodeHtlc(htlcBlob)
+	htlc, err := rfqmsg.HtlcFromCustomRecords(req.WireCustomRecords)
 	if err != nil {
 		return nil, fmt.Errorf("unable to decode htlc: %w", err)
 	}

tapchannel/aux_invoice_manager_test.go

@@ -178,7 +178,7 @@ func (m *mockHtlcModifierProperty) HtlcModifier(ctx context.Context,
 			continue
 		}
 
-		if len(r.WireCustomRecords) == 0 {
+		if !rfqmsg.HasAssetHTLCCustomRecords(r.WireCustomRecords) {
 			if isAssetInvoice(r.Invoice, m) {
 				if !res.CancelSet {
 					m.t.Errorf("expected cancel set flag")
@@ -196,10 +196,7 @@ func (m *mockHtlcModifierProperty) HtlcModifier(ctx context.Context,
 			continue
 		}
 
-		htlcBlob, err := r.WireCustomRecords.Serialize()
-		require.NoError(m.t, err)
-
-		htlc, err := rfqmsg.DecodeHtlc(htlcBlob)
+		htlc, err := rfqmsg.HtlcFromCustomRecords(r.WireCustomRecords)
 		require.NoError(m.t, err)
 
 		if htlc.RfqID.ValOpt().IsNone() {