-
Notifications
You must be signed in to change notification settings - Fork 108
[custom channels]: Use new tapchannelrpc
RPC methods
#807
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
Conversation
9140cc0
to
3fa4a1d
Compare
81bb790
to
a19321f
Compare
I tested all commands manually, everything seems to work as expected. Ready for full review now. |
return m.invoiceChan, m.invoiceErrChan, nil | ||
} | ||
|
||
type mockRouter struct { |
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.
I think we should add a bit more to this commit message. I'm not certain what's happening here. I think you're removing functionality from the LND mock and putting it into a new mock called mockRouter
.
Is that split related to the lndclient version bump?
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.
+1, fully understand the diff but missing rationale/context
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.
Added some rationale to the commit message.
itest/assets_test.go
Outdated
t, src, dst, btcutil.Amount(htlcCarrierAmt), | ||
encodeResp.CustomRecords, | ||
) | ||
time.Sleep(time.Second) |
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.
Unnecessary sleep? Shouldn't getAssetPaymentResult
block on the recv?
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.
Unfortunately and strangely the tests fail without it... It shouldn't make a difference, but somehow it does. Moved it to a single place and added an explanatory comment.
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! 🥕
return m.invoiceChan, m.invoiceErrChan, nil | ||
} | ||
|
||
type mockRouter struct { |
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.
+1, fully understand the diff but missing rationale/context
Because each lndclient service mock now needs to implement the RawClientWithMacAuth method but with a different type, we can't implement two lndclient service mocks within a single struct, as that would require us to have two methods with the same name but different types. So we split the lnd and rounter mocks into two separate structs.
aeedc66
to
734627b
Compare
ed56dd9
to
d8c4d4d
Compare
With this commit we bump to the latest version of taproot assets and lnd (staging branch), so we can use the new SendPayment RPC that handles the RFQ part inline.
d8c4d4d
to
8853d2c
Compare
Depends on lightninglabs/taproot-assets#1048.
Depends on lightningnetwork/lnd#8947.Uses the new
tapchannelrpc
RPC methodsSendPayment
andAddInvoice
that do the RFQ part inline, so we don't have to keep that logic in thelitcli
code.