-
Notifications
You must be signed in to change notification settings - Fork 106
Multi rfq send itest #1097
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
base: master
Are you sure you want to change the base?
Multi rfq send itest #1097
Conversation
6165322
to
c7dbe80
Compare
Seems like you should update to mention the send test, possibly renaming the function name? |
Do we see any value in adding tests where Charlie has more or less channels than Fabia? Like this route seems a bit too simple because both sender and receiver kind of need the same sharding. I'm also wondering if we should test where Charlie-Dave have asset channels and not sats channels. I don't think this test should be removed, but I think there should be some more complex network that tests more than what this test can do in another function. |
1e79938
to
533a39f
Compare
Linter / unit tests fail because of this
which is not related to this PR, the itests should normally run & pass |
The last commit adds a new node to the topology, which always rejects quotes, causing a reliable RFQ negotiation failure this way The tests pass, but they do now expose some potential bug in the RFQ negotiation code where we hang until timeout if the rfq negotiation is not successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending a couple of nits.
itest/assets_test.go
Outdated
@@ -1733,8 +1737,8 @@ func payInvoiceWithAssets(t *testing.T, payer, rfqPeer *HarnessNode, | |||
acceptedQuote := quoteMsg.GetAcceptedSellOrder() | |||
require.NotNil(t, acceptedQuote) | |||
|
|||
peerPubKey := acceptedQuote.Peer | |||
require.Equal(t, peerPubKey, rfqPeer.PubKeyStr) | |||
// peerPubKey := acceptedQuote.Peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: commented out code.
itest/assets_test.go
Outdated
@@ -264,6 +307,19 @@ func createTestMultiRFQAssetNetwork(t *harnessTest, net *NetworkHarness, | |||
require.NoError(t.t, err) | |||
t.Logf("Funded channel between Yara and Fabia: %v", fundRespYF) | |||
|
|||
// We fund the Yara->Fabia channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment mismatch.
itest/litd_custom_channels_test.go
Outdated
"--taproot-assets.allow-public-uni-proof-courier", | ||
"--taproot-assets.universe.public-access=rw", | ||
"--taproot-assets.universe.sync-all-assets", | ||
"--taproot-assets.universerpccourier.skipinitdelay", | ||
"--taproot-assets.universerpccourier.backoffresetwait=1s", | ||
"--taproot-assets.universerpccourier.numtries=5", | ||
"--taproot-assets.universerpccourier.initialbackoff=300ms", | ||
"--taproot-assets.universerpccourier.maxbackoff=600ms", | ||
"--taproot-assets.universerpccourier.skipinitdelay", | ||
"--taproot-assets.universerpccourier.backoffresetwait=100ms", | ||
"--taproot-assets.universerpccourier.initialbackoff=300ms", | ||
"--taproot-assets.universerpccourier.maxbackoff=600ms", | ||
"--taproot-assets.custodianproofretrievaldelay=500ms", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can use append(litdArgsTemplateNoOracle, []string{
instead of duplicating all these values (since they look identical at first glance).
|
||
// Let's make another round-trip involving multi-rfq functionality. | ||
// Let's have Fabia receive another large payment and send it back | ||
// again, this time with a greater amount. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and you are also using a group key too instead of an asset id? does/should this test ensure that it shards and multiple asset ID are used in the group key or is it not trying to check for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do more extensive tests around assetID/groupkey in diff itests (mostly the liquidity edge cases) here I'm just switching things up a bit just as an extra sanity check
@ffranr: review reminder |
go.mod
Outdated
@@ -247,3 +247,7 @@ replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-d | |||
// it is a replace in the tapd repository, it doesn't get propagated here | |||
// automatically, so we need to add it manually. | |||
replace github.com/golang-migrate/migrate/v4 => github.com/lightninglabs/migrate/v4 v4.18.2-9023d66a-fork-pr-2 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove these lines now that lightningnetwork/lnd#9980 has been merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to remove the replace
in go.mod
, and some other outstanding comments, otherwise good to go.
FYI: I made a branch where I rebased and then updated all the packages. Not sure if you want to use it or re-do it another way. https://github.com/ZZiigguurraatt/taproot-assets/tree/group-key-addr-part-2%2Bmulti-rfq-send |
sorry, got this PR mixed up with the other one. |
33aa187
to
b60e3e4
Compare
b60e3e4
to
7e35182
Compare
@GeorgeTsagk can you please rebase this to the |
do we want to prio lightninglabs/loop#968 now so that this can be done more cleanly? |
Description
Enhances the multi-rfq itest to cover multi-rfq send functionality. In the following topology
we now also get Fabia to pay Charlie back, with amounts that exceed the capacity of each individual channel. We also use a hold invoice to validate that multiple HTLCs were added in different channels.
Related PRs (in order of merge sequence):