feat: implement transaction.direct status update#497
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces new methods for reverse swap management, including querying by claim address and selecting direct outputs. It refactors fee estimation and amount checking logic for better modularity, enhances direct reverse swap payment handling, and adds related tests. Duplicate claim address prevention is enforced during reverse swap creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPCServer
participant Database
Client->>RPCServer: createReverseSwap(claimAddress)
RPCServer->>Database: Query all pending reverse swaps
Database-->>RPCServer: List of pending swaps
RPCServer->>RPCServer: Check for duplicate claimAddress
alt Duplicate found
RPCServer-->>Client: Error (AlreadyExists)
else No duplicate
RPCServer->>Database: Create new reverse swap
Database-->>RPCServer: Confirmation
RPCServer-->>Client: Success
end
sequenceDiagram
participant Nursery
participant Onchain
participant Database
Nursery->>Onchain: GetFeeEstimations()
Onchain-->>Nursery: FeeEstimations
Nursery->>Database: Fetch ReverseSwaps
loop For each swap
Nursery->>Onchain: Get outputs for swap
Nursery->>Nursery: chooseDirectOutput(swap, fees, outputs)
Nursery->>Nursery: handleReverseSwapDirectPayments(swap, outputs)
Nursery->>Database: Update swap status/claim tx
end
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jackstar12
force-pushed
the
feat/direct-tx-lwk
branch
from
c924032 to
20e87e2
Compare
jackstar12
force-pushed
the
feat/direct-tx-lwk
branch
from
ead465d to
e2c6d65
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
internal/rpcserver/rpcserver_test.go (1)
1910-1910: Consider using event-based waiting instead of sleep.While the 1-second sleep works, it could make tests slower and potentially flaky. Consider waiting for a specific event or condition instead.
internal/nursery/reverse.go (1)
155-199: Consider improving the wallet type handling clarity.The current implementation correctly handles Liquid wallets but silently breaks for non-Liquid wallets (e.g., GDK). While the comment explains this, consider making the intent more explicit.
Consider adding a log message or more explicit handling:
} else { - // gdk handles direct transactions via its own transaction notifier + // gdk handles direct transactions via its own transaction notifier + logger.Debugf("Skipping direct transaction handling for non-LWK wallet type") break }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/database/reverse.go(1 hunks)internal/nursery/nursery.go(1 hunks)internal/nursery/nursery_test.go(2 hunks)internal/nursery/reverse.go(5 hunks)internal/onchain/liquid-wallet/wallet.go(5 hunks)internal/rpcserver/router.go(1 hunks)internal/rpcserver/rpcserver_test.go(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
internal/nursery/nursery_test.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/rpcserver/rpcserver_test.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/nursery/reverse.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
🧬 Code Graph Analysis (4)
internal/nursery/nursery.go (3)
pkg/boltz/fees.go (2)
FeeEstimations(31-31)CheckAmounts(63-72)pkg/boltz/currency.go (2)
CurrencyLiquid(12-12)CurrencyBtc(11-11)pkg/boltz/api.go (1)
Percentage(79-79)
internal/onchain/liquid-wallet/wallet.go (1)
internal/onchain/liquid-wallet/lwk/lwk.go (3)
WolletDescriptor(7173-7175)NewWolletDescriptor(7177-7187)NewAddress(2356-2366)
internal/rpcserver/router.go (2)
internal/database/database.go (2)
SwapQuery(332-341)Id(330-330)pkg/boltzrpc/boltzrpc.pb.go (5)
SwapState(70-70)SwapState(115-117)SwapState(119-121)SwapState(128-130)SwapState_PENDING(73-73)
internal/rpcserver/rpcserver_test.go (2)
internal/test/test.go (2)
GetNewAddress(188-190)LiquidCli(160-162)pkg/boltzrpc/boltzrpc.pb.go (1)
Currency_LBTC(136-136)
🔇 Additional comments (15)
internal/database/reverse.go (1)
269-278: LGTM! Well-implemented database query method.The new
QueryReverseSwapByClaimAddressmethod correctly follows the existing patterns in the codebase and properly filters by both claim address and PENDING state to prevent duplicate claim addresses.internal/nursery/nursery_test.go (1)
111-218: Excellent test coverage for the new functionality.The
TestChooseDirectOutputtest is well-implemented with comprehensive coverage of different scenarios including matching outputs, no matches, and the special case of zero-value outputs for Liquid transactions. The table-driven test structure makes it easy to understand and maintain.internal/nursery/nursery.go (1)
368-388: Good refactoring that improves modularity.The extraction of fee estimation logic into a separate
GetFeeEstimationsmethod improves code organization and reusability. The method properly handles errors for both currency fee estimations and maintains the existing behavior ofCheckAmounts.internal/onchain/liquid-wallet/wallet.go (3)
30-30: LGTM!The descriptor field is properly added to store the wallet descriptor for deriving blinding keys.
191-207: LGTM!The descriptor initialization and usage in the constructor is implemented correctly with proper error handling.
395-406: LGTM!The
DeriveBlindingKeymethod correctly implements blinding key derivation for Liquid addresses with appropriate error handling.internal/rpcserver/rpcserver_test.go (4)
34-34: LGTM!The serializers import is correctly added and used for currency parsing in the test.
1830-1846: LGTM!The test correctly validates that duplicate claim addresses are prevented for pending reverse swaps, which aligns with the new validation logic.
1854-1856: LGTM!Re-enabling the Liquid test cases is appropriate now that the Liquid blinding key derivation is implemented.
1867-1867: LGTM!The mock expectation for
BroadcastTransactionis correctly added to support the transaction broadcasting in the test flow.internal/nursery/reverse.go (5)
9-14: LGTM!The added imports are necessary for string manipulation and Liquid wallet operations.
285-323: LGTM! The output selection logic is well-implemented.The method correctly handles multiple outputs, prevents double-claiming, and has appropriate special handling for unblindable Liquid outputs.
325-377: Excellent implementation of direct payment handling!The method properly handles direct payments with atomic database updates, appropriate confirmation checks, and comprehensive error handling.
388-402: LGTM!The simplified individual swap processing is cleaner and aligns well with the duplicate claim address prevention strategy.
407-424: LGTM!The method signature change to handle a single swap is consistent with the new processing model and simplifies the logic.
jackstar12
force-pushed
the
feat/direct-tx-lwk
branch
2 times, most recently
from
05f3273 to
d9b9d61
Compare
this simplifies the handling of direct payments and gets rid of the problem where we cant verify the value of liquid transactions made to these transactions
jackstar12
force-pushed
the
feat/direct-tx-lwk
branch
from
d9b9d61 to
03c34e6
Compare
jackstar12
force-pushed
the
feat/direct-tx-lwk
branch
from
c76b957 to
13ef84e
Compare
* feat: lwk server integration * feat: lwk `GetOutputs` implementation for MRHs * refactor: improve wallet tests * chore: .gitignore * chore: deprecate subaccount apis * feat: electrum backend * refactor: `getOwnWallet` -> `getGdkWallet` * fix: dont select subaccounts in cli wallet import not needed anymore * feat: `TxProvider` in wallet backend important to not broadcast via boltz * feat: lwk index persister * refactor: move full scan of wallets into seperate `Sync` function this way, we can create the wallet inside of a transaction with the other db operations and have the heavier sync not block the db * feat: transactions pagination * docs: clarify persister comments * fix: correct insufficient funds error want to be consistent with gdk * fix: encrypt wallets after logging in * feat: derive default descriptor before creating wallet in db users will see descriptor in credentials of hot wallets this way * fix: use `GetSendFee` from wallet interface in `checkBalance` * feat: implement `transaction.direct` status update (#497) * feat: implement `transaction.direct` status update * refactor: dont set mrh for external claim addresses this simplifies the handling of direct payments and gets rid of the problem where we cant verify the value of liquid transactions made to these transactions * fix: flaky gdk bump test * fix: remove manual GC run debugging leftover
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests