Add simple asset autoloop #886

sputn1ck · 2025-02-17T15:10:53Z

This PR adds a simple asset autoloop mode by reusing the easy autoloop runloop.

Testing instructions:

In order to test you need a docker-regtest setup with an asset channel.

EDIT(hieblmi) Setup with an asset channel with following steps:

regtest init (should build litd)
regtest start
regtest assetmint
regtest openassetchan

Afterwards set up your loop with the extra flags:

  --tapd.activate \
  --tapd.host=localhost:8443 \
  --tapd.macaroonpath=$HOME/docker/mounts/regtest/eve/tapd.macaroon \
  --tapd.tlspath=$HOME/docker/mounts/regtest/eve/tls.cert \

You then need to activate your asset autoloop with

./loop-debug setparams --autoloop --easyautoloop --localbalancesat 500 --asset_id $ASSET_ID

Then you can run
./loop-debug forceautoloop this should dispatch a loop out

AI generated summary:

This pull request introduces significant changes to the assets/client.go and liquidity/liquidity.go files, adding new functionality for handling asset prices and improving the overall asset management. The most important changes include the addition of a new method to get asset prices, modifications to the client configuration to include an asset client, and updates to the liquidity manager to support easy autoloop functionality for assets.

Asset Management Enhancements:

assets/client.go: Added a new method GetAssetPrice to retrieve the price of an asset in satoshis using the RFQ process. Also, added a helper function getSatsFromAssetAmt to convert asset amounts to satoshis.
assets/client_test.go: Added tests for the new getSatsFromAssetAmt function to ensure correct conversion of asset amounts to satoshis.

Client Configuration Updates:

client.go: Updated the Client struct and related methods to use AssetClient instead of assetClient, ensuring consistency and proper initialization. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]
config.go: Added AssetClient to the clientConfig struct to support asset-related operations.

Liquidity Manager Enhancements:

liquidity/liquidity.go: Introduced new functionality for easy autoloop operations for assets, including methods to handle asset prices and dispatch swaps based on asset balances. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]

These changes collectively enhance the system's ability to manage and utilize assets more effectively, providing better support for asset-related operations and improving the overall functionality of the liquidity manager.

hieblmi

Great work as usual @sputn1ck. In my first pass I've left a few suggestions, comments and nits.

liquidity/parameters.go

looprpc/client.proto

liquidity/interface.go

liquidity/liquidity.go

assets/client.go

hieblmi · 2025-02-18T14:38:12Z

assets/client.go

+					AssetIdStr: assetId,
+				},
+			},
+			PaymentMaxAmt:  uint64(satMinAmt),


can we leave a comment here why the minimum swap amount has to match the maximum amount the edge node has to pay?

I renamed the var. this is just to signal the edge node what the maximum payment amount would be, but essentially unused as we just want to gauge the price. This whole function will be changed once lightninglabs/taproot-assets#1397 lands

hieblmi · 2025-02-18T14:39:49Z

assets/client.go

+
+	acceptedRes := rfq.GetAcceptedQuote()
+	if acceptedRes == nil {
+		return 0, fmt.Errorf("no accepted quote")


nit: "quote wasn't accepted", maybe with details?

This actually isn't "quote wasn't accepted" as that would be rfq.GetRejectedQuote(). This would be an internal error in the grpc call so pretty unlikely and a check for the .proto oneof message type.

func (x *AddAssetSellOrderResponse) GetAcceptedQuote() *PeerAcceptedSellQuote { if x, ok := x.GetResponse().(*AddAssetSellOrderResponse_AcceptedQuote); ok { return x.AcceptedQuote } return nil } func (x *AddAssetSellOrderResponse) GetInvalidQuote() *InvalidQuoteResponse { if x, ok := x.GetResponse().(*AddAssetSellOrderResponse_InvalidQuote); ok { return x.InvalidQuote } return nil } func (x *AddAssetSellOrderResponse) GetRejectedQuote() *RejectedQuoteResponse { if x, ok := x.GetResponse().(*AddAssetSellOrderResponse_RejectedQuote); ok { return x.RejectedQuote } return nil }

These are the 3 possibilities

liquidity/parameters.go

assets/client.go

bhandras

Looking quite good! I think some separation of concerns within the easy autoloop where we iterate over channels would be simple to add and would greatly enhance testability of the the code. I'd also recommend covering the new functionality with unit tests.

bhandras · 2025-02-24T17:14:21Z

looprpc/client.proto

+    map<string, EasyAssetParams> easy_asset_params = 25;
+}
+
+message EasyAssetParams {


nit: would be better to call it EasyAssetAutloopParams?

bhandras · 2025-02-24T17:23:43Z

liquidity/parameters.go

+
+	// EasyAutoloopTargetAmount is the target amount of liquidity that we
+	// want to maintain in our channels.
+	EasyAutoloopTargetAmount uint64


nit: why not call this LocalTargetAssetAmt?

liquidity/parameters.go

liquidity/liquidity.go

bhandras · 2025-02-24T17:27:09Z

liquidity/loopout_builder.go

+			return nil, err
+		}
+		assetRfqRequest = &loop.AssetRFQRequest{
+			AssetId:       assetIDBytes,


Not in this PR but I think we need to start using an asset specifier instead (which is a union of asset id and group key).

^ will greatly help with future group key support

bhandras · 2025-02-24T19:11:44Z

liquidity/liquidity.go

 		return nil
 	}

 	log.Debugf("easy autoloop: local_total=%v, target=%v, "+
 		"attempting to loop out %v", localTotal,
-		m.params.EasyAutoloopTarget, amount)
+		localTarget, amount)


Just flagging that we should log the satAmount here iiuc (though if not renamed, the log is fine).

bhandras · 2025-02-24T19:13:44Z

liquidity/liquidity.go

 	localTotal := btcutil.Amount(0)
 	for _, channel := range channels {
 		if channelIsCustom(channel) {
-			continue
+			if assetID == "" {


Ideally we should separate the asset/btc per channel logic here, so it's more readable as well as testable.

bhandras · 2025-02-24T19:15:23Z

assets/client.go

+
+	// We'll allow a short rfq expiry as we'll only use this rfq to
+	// gauge a price.
+	rfqExpiry := time.Now().Add(time.Minute).Unix()


Do we need to specify expiry? Is there a default?

The sell order expiry is actually unused in tapd. I think we should keep this sane value though.

The rfq expiry is checked when this function is called on intercepted incoming HTLCs

assets/client.go

bhandras · 2025-02-24T19:22:41Z

assets/client.go

+		return 0, fmt.Errorf("cannot unmarshal asset rate: %w", err)
+	}
+
+	assetUnits := rfqmath.NewBigIntFixedPoint(assetAmt, 0)


Here we hold the assumption that an asset unit is at least a millisatoshi. What about fractions? Should we scale both asset units and the rate so we can represent fractions correctly?

Why do you consider this to be assumed?

Copilot

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Copilot · 2025-02-26T18:42:05Z

liquidity/loopout_builder.go

+	initatior := getInitiator(params)
+


The variable name 'initatior' appears to be misspelled; consider renaming it to 'initiator' for clarity.

Suggested change

initatior := getInitiator(params)

initiator := getInitiator(params)

lightninglabs-deploy · 2025-03-05T18:59:31Z

@hieblmi: review reminder
@sputn1ck, remember to re-request review from reviewers when ready

hieblmi

Extracting the asset logic from easy autoloop looks good. I've left some more comments.

hieblmi · 2025-03-06T09:07:47Z

liquidity/parameters.go

@@ -413,6 +427,14 @@ func RpcToParameters(req *clientrpc.LiquidityParameters) (*Parameters,
 		addrType = walletrpc.AddressType_TAPROOT_PUBKEY
 	}

+	easyAssetMap := make(map[string]AssetParams)


this reads better if its EasyAssetParams.

liquidity/parameters.go

hieblmi · 2025-03-06T09:22:23Z

liquidity/parameters.go

@@ -528,6 +551,17 @@ func ParametersToRpc(cfg Parameters) (*clientrpc.LiquidityParameters,
 		addrType = clientrpc.AddressType_ADDRESS_TYPE_UNKNOWN
 	}

+	easyAssetMap := make(map[string]*clientrpc.EasyAssetAutoloopParams, len(


nit: formatting

easyAssetMap := make( map[string]*clientrpc.EasyAssetAutoloopParams, len(cfg.AssetAutoloopParams), )

liquidity/loopout_builder.go

liquidity/liquidity.go

hieblmi · 2025-03-06T14:06:31Z

liquidity/liquidity.go

+			assetData.LocalBalance,
+		)
+
+		assetPeerPubkey = channel.PubKeyBytes[:]


if there are multiple asset channels this overwrites the assetPeerPubkey. If only one asset channel is allowed initially we should filter it out from the list of channels first, then apply the logic that's here in the loop.

The peer is only used initially to get a first price check for how many sats we request for in the swap request. This is not the peer that is asked for rfq if we actually swap out.

hieblmi · 2025-03-06T14:07:58Z

liquidity/liquidity.go

+
+		// We'll overwrite the channel local balance in order to
+		// reuse channel selection logic.
+		channel.LocalBalance = btcutil.Amount(localTotal)


we are setting the LocalBalance already above, is it correct to overwrite it here?

good catch, thanks

liquidity/liquidity.go

assets/client.go

Copilot

PR Overview

This PR adds support for a simple asset autoloop mode by extending the asset client and liquidity manager logic. Key changes include adding a new asset pricing method and helper functions in the asset client, extending client and configuration structures to support an asset client, and updating the liquidity manager and swap builder logic for asset-specific autoloop functionality.

Reviewed Changes

File	Description
cmd/loop/debug.go	Adds a new debug command to force an autoloop tick in development builds.
assets/client.go & client_test.go	Introduces new methods for retrieving asset prices and converting asset amounts.
cmd/loop/liquidity.go	Updates command flags and parameter handling to support asset ID input.
looprpc/client.proto	Adds a new message for asset-specific easy autoloop parameters.
liquidity/parameters.go	Adds asset-specific autoloop parameters to liquidity configuration.
liquidity/loopout_builder.go	Integrates asset swap information into the swap builder with new options.
client.go	Updates client construction and asset client field naming for consistency.
loopd/utils.go	Adds support for passing the asset pricing method to the liquidity manager.
liquidity/interface.go & loopin_builder.go	Adjusts function signatures to accept additional swap options.
liquidity/liquidity.go	Implements asset autoloop procedures in the liquidity manager including channel selection and swap dispatch logic.
liquidity/autoloop_test.go	Introduces tests for asset autoloop, ensuring correct channel selection and swap dispatch behavior.

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

liquidity/loopout_builder.go:151

[nitpick] Concatenating the assetID directly to the initiator string may lead to ambiguity or formatting issues. Consider using a more explicit separator or refactoring the initiator composition to clearly distinguish asset-related information.

initiator += "-" + assetSwap.assetID

Copilot · 2025-03-10T18:21:33Z

liquidity/liquidity.go

+func (m *Manager) dispatchBestAssetEasyAutoloopSwap(ctx context.Context,
+	assetID string, localTarget uint64) error {
+
+	if len(assetID) != 32 {


The assetID length check assumes that a valid asset ID has a length of 32 characters; however, if assetIDs are hex-encoded from 32-byte values, the expected length is 64. Please verify the intended format and update the validation accordingly.

Suggested change

if len(assetID) != 32 {

if len(assetID) != 64 {

bhandras

Looks good just a couple of comments and suggestions. Also I think would be nice to add more test coverage with more conditions: multiple asset/non-asset channels, multiple amounts, min/max etc so we hit more code paths.

liquidity/liquidity.go

assets/client.go

assets/client_test.go

cmd/loop/debug.go

bhandras · 2025-03-10T22:22:52Z

liquidity/autoloop_test.go

+	}
+
+	var (
+		channels = []lndclient.ChannelInfo{assetChan}


An easy win would be to add more channels here so we can test that the selection criteria works correctly.

liquidity/autoloop_test.go

liquidity/liquidity.go

GeorgeTsagk

cACK!
most of my feedback is concentrated around the approach and considerations for future groupkey support, but overall LGTM

I'll leave the impl details up to the loop experts

GeorgeTsagk · 2025-03-12T14:28:36Z

looprpc/client.proto

@@ -1207,6 +1207,29 @@ message LiquidityParameters {
    The address type of the account specified in the account field.
    */
    AddressType account_addr_type = 24;
+
+    /*
+    A map of asset parameters to use for swaps. The key is the asset id and the


should think about how this will change in the future to support group keys.

With group channels there's going to be multiple asset IDs in a single channel so I assume the user will want to define the liquidity parameter based on the group key. Not sure if there's any RPC helper to check whether a channel's assets "satisfy" a groupkey, but could be exposed in the future

something like the internal helper rfq.Manager.ChannelCompatible

GeorgeTsagk · 2025-03-12T14:31:55Z

liquidity/loopout_builder.go

+			return nil, err
+		}
+		assetRfqRequest = &loop.AssetRFQRequest{
+			AssetId:       assetIDBytes,


^ will greatly help with future group key support

GeorgeTsagk · 2025-03-12T14:34:53Z

liquidity/liquidity.go

@@ -305,6 +313,15 @@ func (m *Manager) Run(ctx context.Context) error {
 				}
 			}

+			for assetID := range m.params.AssetAutoloopParams {


a global flag like EasyAssetAutoloopActive would help with users switching off all asset swaps, without editing each liquidity parameter individually

just a recommendation

GeorgeTsagk · 2025-03-12T14:39:20Z

liquidity/liquidity.go

+	}
+
+	// Dispatch a sticky loop out.
+	go m.dispatchStickyLoopOut(


may have been discussed in a prior PR, but I wonder if there's extra risks with making this a sticky loop out
IMO if every attempt grabs a new quote then it should be fine

GeorgeTsagk · 2025-03-12T14:46:38Z

liquidity/liquidity.go

+			// If we use a custom channel, the local balance is
+			// denominated in the asset's unit, so we don't need to
+			// check the minimum.
+			!channelIsCustom(channel) {


could provide the rfq / price hint here in order to have a more realistic image of what the assets evaluate to. Is this effectively bypassing the minimum restriction?

GeorgeTsagk · 2025-03-12T14:49:06Z

liquidity/liquidity.go

+		return nil
+	}
+
+	for _, asset := range assetData.Assets {


could add a todo here for group key support, will have to aggregate all the asset balances

GeorgeTsagk · 2025-03-12T14:51:06Z

assets/client.go

+
+	// We'll allow a short rfq expiry as we'll only use this rfq to
+	// gauge a price.
+	rfqExpiry := time.Now().Add(time.Minute).Unix()


The rfq expiry is checked when this function is called on intercepted incoming HTLCs

GeorgeTsagk · 2025-03-12T14:53:05Z

assets/client.go

+		return 0, fmt.Errorf("cannot unmarshal asset rate: %w", err)
+	}
+
+	assetUnits := rfqmath.NewBigIntFixedPoint(assetAmt, 0)


Why do you consider this to be assumed?

sputn1ck force-pushed the asset_autoloop_1 branch from 39fbaf6 to 30bf919 Compare February 17, 2025 15:37

sputn1ck requested a review from bhandras February 17, 2025 16:35

hieblmi self-requested a review February 18, 2025 10:51

hieblmi reviewed Feb 18, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch 4 times, most recently from 60c3a87 to 8c6e77a Compare February 18, 2025 15:48

hieblmi reviewed Feb 19, 2025

View reviewed changes

liquidity/parameters.go Outdated Show resolved Hide resolved

hieblmi reviewed Feb 19, 2025

View reviewed changes

assets/client.go Show resolved Hide resolved

sputn1ck force-pushed the asset_autoloop_1 branch 2 times, most recently from e3dc42f to 5f72083 Compare February 20, 2025 14:39

sputn1ck marked this pull request as ready for review February 20, 2025 14:39

sputn1ck requested a review from hieblmi February 20, 2025 14:39

sputn1ck force-pushed the asset_autoloop_1 branch from 5f72083 to 4a0b1eb Compare February 20, 2025 14:44

levmi assigned sputn1ck Feb 20, 2025

bhandras reviewed Feb 24, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch 3 times, most recently from 6d8bb95 to d4eff31 Compare February 25, 2025 07:44

sputn1ck requested a review from Copilot February 26, 2025 18:39

Copilot AI reviewed Feb 26, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch 2 times, most recently from 4bc6f00 to e2bc1c8 Compare March 4, 2025 08:32

hieblmi reviewed Mar 6, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch 4 times, most recently from 2b227c8 to 4bb5843 Compare March 10, 2025 18:13

sputn1ck requested a review from Copilot March 10, 2025 18:18

Copilot AI reviewed Mar 10, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch from 4bb5843 to 5ecc620 Compare March 10, 2025 18:22

sputn1ck requested review from bhandras and hieblmi March 10, 2025 19:03

sputn1ck force-pushed the asset_autoloop_1 branch from 5ecc620 to 9736660 Compare March 10, 2025 20:45

bhandras reviewed Mar 11, 2025

View reviewed changes

sputn1ck force-pushed the asset_autoloop_1 branch 3 times, most recently from 93bf3aa to 0a12675 Compare March 11, 2025 18:21

sputn1ck requested review from GeorgeTsagk and bhandras March 11, 2025 18:22

GeorgeTsagk reviewed Mar 12, 2025

View reviewed changes

sputn1ck added 11 commits March 12, 2025 11:22

liquidity: add easy asset params

865db66

liquidity: add asset swap info to swap builder

d8ab955

liquidity: add easy asset autoloop out

82d6cac

asset: add function to get asset price

1eb7b95

loopd: move asset client to config

26406bf

loopd: add asset client to autoloop cfg

ca83271

cmd: add easy autoloop asset flags

20d65bb

cmd: add debug forceautoloop cmd

8cf6af9

liquidity: test move p2wkh to const

1478d2e

liquidity: add asset loop out unit test

624d445

liquidity: skip asset channels in easy autoloop

012baa4

sputn1ck force-pushed the asset_autoloop_1 branch from 0a12675 to 012baa4 Compare March 12, 2025 18:47

	initatior := getInitiator(params)
	initiator := getInitiator(params)

Add simple asset autoloop #886

Are you sure you want to change the base?

Add simple asset autoloop #886

Conversation

sputn1ck commented Feb 17, 2025 • edited by hieblmi Loading

Testing instructions:

AI generated summary:

Asset Management Enhancements:

Client Configuration Updates:

Liquidity Manager Enhancements:

hieblmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Feb 26, 2025

Choose a reason for hiding this comment

lightninglabs-deploy commented Mar 5, 2025

hieblmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Mar 10, 2025

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck commented Feb 17, 2025 •

edited by hieblmi

Loading

sputn1ck Feb 18, 2025 •

edited

Loading