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

lnrpc+lnwallet: adds listaddresses RPC #6596

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

priyanshiiit
Copy link
Contributor

@priyanshiiit priyanshiiit commented May 31, 2022

Change Description

Related: #2498

Command: lncli wallet addresses list --account_name "default"

Screenshot from 2022-06-12 17-48-54

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@priyanshiiit priyanshiiit changed the title lnrpc: adds listaddresses RPC [WIP] lnrpc: adds listaddresses RPC May 31, 2022
@guggero guggero self-requested a review June 1, 2022 15:03
Copy link
Collaborator

@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.

This looks very promising! I think we can re-use the code from ListAccounts to avoid some code duplication.

cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 6 times, most recently from 424318d to 4fdaffe Compare June 13, 2022 05:54
@priyanshiiit priyanshiiit changed the title [WIP] lnrpc: adds listaddresses RPC lnrpc+lnwallet: adds listaddresses RPC Jun 13, 2022
@priyanshiiit priyanshiiit marked this pull request as ready for review June 13, 2022 05:55
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 2 times, most recently from df1a8f7 to eb69b90 Compare June 18, 2022 12:53
@guggero guggero added this to the v0.16.0 milestone Jun 20, 2022
@guggero guggero added rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses labels Jun 20, 2022
Copy link
Collaborator

@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.

Did a round of manual testing and found a few things that can be optimized. But I really like this feature, it makes debugging things around wallet balances so much easier! So great work thus far!

cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 4 times, most recently from c28345e to 5d06c22 Compare June 27, 2022 03:17
@guggero
Copy link
Collaborator

guggero commented Jun 27, 2022

Here's the diff I'm proposing:

diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go
index 14f9e3d7d..58db4322e 100644
--- a/lnwallet/btcwallet/btcwallet.go
+++ b/lnwallet/btcwallet/btcwallet.go
@@ -739,29 +739,48 @@ func (b *BtcWallet) ListAddresses(name string) (
        }
 
        for _, accntDetails := range accounts {
+               scopedMgr, err := b.wallet.Manager.FetchScopedKeyManager(
+                       accntDetails.KeyScope,
+               )
+               if err != nil {
+                       return nil, err
+               }
+
                // Retrieve all addresses of an account.
-               addresses, err := b.wallet.AccountAddresses(accntDetails.AccountNumber)
+               var managedAddrs []waddrmgr.ManagedAddress
+               err = walletdb.View(
+                       b.wallet.Database(), func(tx walletdb.ReadTx) error {
+                               addrmgrNs := tx.ReadBucket(waddrmgrNamespaceKey)
+                               return scopedMgr.ForEachAccountAddress(
+                                       addrmgrNs, accntDetails.AccountNumber,
+                                       func(a waddrmgr.ManagedAddress) error {
+                                               managedAddrs = append(
+                                                       managedAddrs, a,
+                                               )
+
+                                               return nil
+                                       },
+                               )
+                       },
+               )
                if err != nil {
                        return nil, err
                }
 
                // Only consider those accounts which have addresses.
-               if len(addresses) == 0 {
+               if len(managedAddrs) == 0 {
                        continue
                }
 
-               addressProperties := make([]lnwallet.AddressProperty, len(addresses))
-
-               for idx, address := range addresses {
-                       addressInfo, err := b.wallet.AddressInfo(address)
-                       if err != nil {
-                               return nil, err
-                       }
-
-                       addressString := address.String()
+               addressProperties := make(
+                       []lnwallet.AddressProperty, len(managedAddrs),
+               )
+               for idx, managedAddr := range managedAddrs {
+                       addr := managedAddr.Address()
+                       addressString := addr.String()
                        addressProperties[idx] = lnwallet.AddressProperty{
                                Address:  addressString,
-                               Internal: addressInfo.Internal(),
+                               Internal: managedAddr.Internal(),
                                Balance:  addressBalance[addressString],
                        }
                }

@priyanshiiit priyanshiiit changed the title lnrpc+lnwallet: adds listaddresses RPC [WIP]lnrpc+lnwallet: adds listaddresses RPC Jun 29, 2022
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 3 times, most recently from a487103 to 1231806 Compare June 30, 2022 10:29
@priyanshiiit priyanshiiit changed the title [WIP]lnrpc+lnwallet: adds listaddresses RPC lnrpc+lnwallet: adds listaddresses RPC Jun 30, 2022
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 3 times, most recently from 36234e8 to 7d81bce Compare July 4, 2022 04:54
@priyanshiiit priyanshiiit requested a review from guggero July 4, 2022 04:59
@priyanshiiit priyanshiiit force-pushed the addressbalance branch 2 times, most recently from 35d6dac to b9a1ff7 Compare July 8, 2022 10:26
Copy link
Collaborator

@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.

Looking very good, I like the new flag!
Mostly style nits and the integration test that needs some adjustment. But we're getting very close!

lnwallet/btcwallet/btcwallet.go Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit.proto Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
lntest/itest/lnd_misc_test.go Outdated Show resolved Hide resolved
lntest/itest/lnd_misc_test.go Show resolved Hide resolved
Copy link
Collaborator

@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.

tACK, LGTM 🎉
Very nice work!

cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
@guggero guggero requested a review from bhandras July 12, 2022 07:55
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Nice work! Just a few nits.

lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Show resolved Hide resolved
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
lntest/itest/lnd_misc_test.go Show resolved Hide resolved
lntest/itest/lnd_misc_test.go Show resolved Hide resolved
@bhandras
Copy link
Collaborator

needs rebase

@lightninglabs-deploy
Copy link

@bhandras: review reminder

Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @priyanshiiit 🎉

My only comment here is to perhaps change the loop order in the test case to make sure we don't pass on skipped addresses.

lnwallet/btcwallet/btcwallet.go Outdated Show resolved Hide resolved
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
lntest/itest/lnd_misc_test.go Show resolved Hide resolved
Copy link
Collaborator

@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.

LGTM, just one tiny last comment 🎉

docs/release-notes/release-notes-0.15.1.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants