api: Move BTC positive balance filtering to API-level #7515
Conversation
PR bisq-network#7483 added the --only-funded flag to the getfundingaddresses method. This flag was added to bisq-cli and the filtering happened client-side. This change moves the filtering to the server-side, so that all API users can use the 'only-funded' option.
|
Tests:
|
WalkthroughAdds an --only-funded flag to the getfundingaddresses command across CLI, gRPC, Core, and Daemon. Updates proto to carry the flag, introduces a dedicated option parser, adjusts method signatures to accept a boolean, and updates help and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as CliMain
participant P as GetFundingAddressesOptionParser
participant GC as GrpcClient
participant D as Daemon GrpcWalletsService
participant A as CoreApi
participant W as CoreWalletsService
U->>C: bisq-cli getfundingaddresses --only-funded[=true|false]
C->>P: parse(args)
P-->>C: onlyFunded flag
C->>GC: getFundingAddresses(onlyFunded)
GC->>D: GetFundingAddressesRequest{ only_funded=onlyFunded }
D->>A: getFundingAddresses(onlyFunded)
A->>W: getFundingAddresses(onlyFunded)
alt onlyFunded == true
W->>W: filter addresses with balance > 0
else onlyFunded == false
W->>W: include all funding addresses
end
W-->>A: List<AddressBalanceInfo>
A-->>D: List<AddressBalanceInfo>
D-->>GC: GetFundingAddressesResponse
GC-->>C: List<AddressBalanceInfo>
C-->>U: Print addresses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/main/java/bisq/core/api/CoreWalletsService.java (1)
240-245: Performance concern: O(N²) complexity from repeated lookups.Line 244 calls
getAddressEntry(address)for each address in the stream. ThegetAddressEntrymethod (lines 640-650) performs a linear search through the address entry list. For N addresses, this results in O(N²) complexity.Consider memoizing
getAddressEntrysimilar to howgetAddressBalanceis memoized (line 225):// getAddressBalance is memoized, because we'll map it over addresses twice. // To get the balances, we'll be using .getUnchecked, because we know that // this::getAddressBalance cannot return null. var balances = memoize(this::getAddressBalance); +var addressEntries = memoize(this::getAddressEntry); boolean noAddressHasZeroBalance = addressStrings.stream() .allMatch(addressString -> balances.getUnchecked(addressString) != 0); if (noAddressHasZeroBalance) { var newZeroBalanceAddress = btcWalletService.getFreshAddressEntry(); addressStrings.add(newZeroBalanceAddress.getAddressString()); } Stream<String> resultStream = addressStrings.stream(); if (onlyFunded) { resultStream = resultStream.filter(address -> balances.getUnchecked(address) > 0); } return resultStream.map(address -> new AddressBalanceInfo(address, balances.getUnchecked(address), getNumConfirmationsForMostRecentTransaction(address), - btcWalletService.isAddressUnused(getAddressEntry(address).getAddress()))) + btcWalletService.isAddressUnused(addressEntries.getUnchecked(address).getAddress()))) .collect(Collectors.toList());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cli/src/main/java/bisq/cli/CliMain.java(3 hunks)cli/src/main/java/bisq/cli/GrpcClient.java(1 hunks)cli/src/main/java/bisq/cli/opts/GetFundingAddressesOptionParser.java(1 hunks)cli/src/main/java/bisq/cli/opts/OptLabel.java(1 hunks)cli/src/main/java/bisq/cli/request/WalletsServiceRequest.java(1 hunks)cli/src/test/java/bisq/cli/table/AddressCliOutputDiffTest.java(2 hunks)core/src/main/java/bisq/core/api/CoreApi.java(1 hunks)core/src/main/java/bisq/core/api/CoreWalletsService.java(3 hunks)core/src/main/resources/help/getfundingaddresses-help.txt(1 hunks)daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java(1 hunks)proto/src/main/proto/grpc.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/src/main/java/bisq/cli/CliMain.java (1)
cli/src/main/java/bisq/cli/opts/GetFundingAddressesOptionParser.java (1)
GetFundingAddressesOptionParser(7-26)
cli/src/main/java/bisq/cli/opts/GetFundingAddressesOptionParser.java (1)
cli/src/main/java/bisq/cli/opts/OptLabel.java (1)
OptLabel(23-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Java 11, windows-latest
- GitHub Check: Test Java 11, ubuntu-latest
- GitHub Check: Test Java 11, macOS-latest
🔇 Additional comments (14)
proto/src/main/proto/grpc.proto (1)
930-930: LGTM! Proto field definition is correct.The optional boolean field
only_fundedis properly defined with tag 1 and follows proto3 conventions. The default value offalseensures backward compatibility with existing clients.core/src/main/resources/help/getfundingaddresses-help.txt (1)
10-21: LGTM! Clear and helpful documentation.The documentation for the
--only-fundedoption is concise and clearly explains its purpose. The addition of the OPTIONS section follows standard help text conventions.cli/src/main/java/bisq/cli/opts/OptLabel.java (1)
41-41: LGTM! Constant follows established conventions.The
OPT_ONLY_FUNDEDconstant is correctly defined and maintains alphabetical ordering with other option labels.cli/src/test/java/bisq/cli/table/AddressCliOutputDiffTest.java (2)
30-30: LGTM! Test updated to use new API signature.The test correctly passes
falseto maintain the previous behavior of fetching all funding addresses without filtering.
45-45: LGTM! Consistent test update.Both usages of
getFundingAddressesare consistently updated to use the new boolean parameter.core/src/main/java/bisq/core/api/CoreApi.java (1)
395-397: LGTM! API signature correctly updated.The method signature properly accepts the
onlyFundedboolean parameter and delegates to the wallet service. This centralizes the filtering logic on the server side as intended.cli/src/main/java/bisq/cli/CliMain.java (3)
70-70: LGTM! Appropriate import added.The import of
GetFundingAddressesOptionParseris necessary for parsing the new option.
253-262: LGTM! Option parsing and integration are correct.The implementation properly:
- Parses the
--only-fundedoption using the dedicated parser- Checks for help request before processing
- Passes the boolean flag to the client
- Maintains consistent error handling and output formatting
904-904: LGTM! Help text correctly documents the option.The help output clearly shows the
--only-funded=<true|false>option format and aligns with the new functionality.daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java (1)
164-165: LGTM! gRPC service correctly bridges to Core API.The implementation properly extracts the
onlyFundedflag from the gRPC request and passes it to the Core API, completing the server-side filtering integration.cli/src/main/java/bisq/cli/request/WalletsServiceRequest.java (1)
102-108: LGTM! Request construction is correct.The method signature properly accepts the
onlyFundedboolean parameter and correctly constructs the gRPC request usingsetOnlyFunded(onlyFunded). This completes the client-side integration of the server-side filtering feature.cli/src/main/java/bisq/cli/GrpcClient.java (1)
118-120: LGTM!The method signature correctly adds the
boolean onlyFundedparameter and properly delegates to the underlying service request.core/src/main/java/bisq/core/api/CoreWalletsService.java (2)
81-81: LGTM!The import for
java.util.stream.Streamis correctly added to support the new filtering logic.
235-239: LGTM!The filtering logic correctly filters addresses to only those with positive balance when
onlyFundedis true. The implementation is clear and correct.
alvasw
dismissed
alejandrogarcia83’s stale review
The merge-base changed after approval.
3 participants
PR #7483 added the --only-funded flag to the getfundingaddresses method.
This flag was added to bisq-cli and the filtering happened client-side.
This change moves the filtering to the server-side, so that all API
users can use the 'only-funded' option.
Summary by CodeRabbit