feat: lwk esplora concurrency#501
Conversation
leave that to go GC
WalkthroughA builder pattern for constructing an Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet
participant EsploraClientBuilder
participant FFI
participant EsploraClient
Wallet->>EsploraClientBuilder: Create builder with params
Wallet->>FFI: Call EsploraClientFromBuilder(builder)
FFI->>EsploraClient: Construct EsploraClient via builder
EsploraClient-->>FFI: Return instance or error
FFI-->>Wallet: Return EsploraClient or error
Poem
✨ Finishing Touches
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 (
|
|
@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: 0
🧹 Nitpick comments (1)
internal/onchain/liquid-wallet/wallet.go (1)
116-116: Consider making the concurrency value configurable.The hard-coded concurrency value of 32 may not be optimal for all environments. Consider making this configurable through the
EsploraConfigstruct or environment variables to allow tuning based on system resources and network conditions.type EsploraConfig struct { Url string Waterfall bool + Concurrency *uint32 }Then use it in the builder:
- concurrency := uint32(32) + concurrency := uint32(32) + if cfg.Esplora.Concurrency != nil { + concurrency = *cfg.Esplora.Concurrency + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/onchain/liquid-wallet/lwk/lwk.go(3 hunks)internal/onchain/liquid-wallet/lwk/lwk.h(2 hunks)internal/onchain/liquid-wallet/wallet.go(1 hunks)lwk(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/onchain/liquid-wallet/wallet.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 (1)
internal/onchain/liquid-wallet/wallet.go (2)
internal/logger/logger.go (1)
Infof(113-115)internal/onchain/liquid-wallet/lwk/lwk.go (3)
EsploraClientFromBuilder(3196-3206)EsploraClientBuilder(7307-7313)Network(4070-4072)
🔇 Additional comments (9)
lwk (1)
1-1: API Compatibility Verified: No Remaining Old Constructor UsagesRan a full-text scan for
NewEsploraClient(and confirmed:
- Definition only in the lwk submodule:
• internal/onchain/liquid-wallet/lwk/lwk.go:3184- No external call-sites in the Go codebase.
Builder API is already in use where clients are constructed:
• internal/onchain/liquid-wallet/wallet.go:117All call-sites have migrated to
EsploraClientBuilder, so the submodule bump can be merged safely.internal/onchain/liquid-wallet/wallet.go (3)
115-115: Log message unification looks good.The unified log message appropriately indicates Esplora client usage without distinguishing waterfall mode, which aligns with the refactoring to use the builder pattern.
117-122: Builder pattern implementation looks correct.The transition from conditional instantiation to the builder pattern is well-implemented. The builder parameters correctly map to the previous logic with appropriate field mappings.
124-124: Error message consistency improvement.The updated error message format provides better consistency with the "esplora client:" prefix, making it easier to identify the source of configuration errors.
internal/onchain/liquid-wallet/lwk/lwk.h (2)
652-656: New FFI constructor looks correct
- Guard macro follows existing naming scheme; avoids double-definition.
- Parameter list (
RustBuffer builder, RustCallStatus *out_status) is consistent with other builder-style constructors, so ownership semantics remain predictable.- No symbol collision with the originals
..._new/..._new_waterfalls.Nothing blocking here.
3064-3066: No Go-side registration required—checksum is already in the glue codeThe newly added
uniffi_lwk_checksum_constructor_esploraclient_from_builderis referenced ininternal/onchain/liquid-wallet/lwk/lwk.go(lines 1775–1780), verifying its checksum (10195) via the existingrustCallpattern. No further updates to Go glue are necessary.internal/onchain/liquid-wallet/lwk/lwk.go (3)
1774-1782: LGTM: Checksum verification follows established pattern.The checksum verification for the new constructor function correctly follows the existing pattern in the codebase. The hardcoded checksum value (10195) should match the Rust side implementation.
3196-3206: LGTM: Constructor function properly handles FFI and errors.The
EsploraClientFromBuilderfunction correctly:
- Uses the established
rustCallWithErrorpattern for error handling- Properly converts between Go and Rust types using FFI converters
- Returns appropriate error values on failure
- Follows the naming conventions of the codebase
7307-7358: LGTM: Builder struct and FFI converters are well-implemented.The
EsploraClientBuilderstruct and its associated FFI converter methods are correctly implemented:
- Optional fields (
ConcurrencyandTimeout) are properly handled as pointers- The
Destroy()method correctly calls destroy on all fields for proper memory management- FFI converter methods (
Lift,Read,Lower,Write) follow the established UniFFI patterns- Public field naming follows Go conventions
- The implementation is consistent with other FFI converters in the codebase
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores