feat: allow updating LDK chain source via UI #2019
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughAdds an LDK on-chain source configuration feature: validation helpers for Esplora/Electrum, a Config API method, frontend settings UI and route, HTTP/Wails endpoints to persist user overrides, tests and mocks, and LDK selection logic that prefers user overrides over environment fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant FE as Frontend
participant API as HTTP Service
participant Config as Config Layer
participant DB as Database
participant LDK as LDK Client
User->>FE: Open Chain Source settings
FE->>FE: load useInfo()
User->>FE: Select source and enter data
FE->>FE: client-side validation
User->>FE: Click Save
FE->>API: PATCH /api/ldk-onchain-source (UpdateChainConfigRequest)
API->>API: ensure LN backend == LDK
API->>Config: ValidateChainSource(chainSource, url)
Config->>Config: validateEsplora / validateElectrum (HTTP or TCP check)
Config-->>API: Validation success/failure
alt Validation success
API->>DB: SetUpdate() user override fields
DB-->>API: persisted
API-->>FE: 200 OK
FE->>User: success toast
else Validation failed
API-->>FE: 400 error
FE->>User: show error
end
Note over LDK,DB: On next LDK init
LDK->>DB: query UserChainSource
alt user override exists and valid
LDK->>LDK: configure user-selected chain source
else
LDK->>LDK: use environment-based chain source
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@config/models.go`:
- Around line 94-105: The struct comment for UpdateChainConfigRequest's
ChainSource is incorrect and lists "bitcoind" whereas the handler expects
"bitcoind_rpc"; update the inline comment on the ChainSource field in type
UpdateChainConfigRequest to list "bitcoind_rpc" (so it reads something like
`"esplora", "electrum", "bitcoind_rpc", or "default"`) to match the
handler/clients and avoid 400 errors.
In `@config/tests/config_test.go`:
- Around line 365-412: The tests "Esplora - Connection Refused" and "Electrum -
Connection Refused" use a fixed port 127.0.0.1:54321 which can be flaky; replace
the hardcoded port with a dynamically reserved/free port that you immediately
close to simulate refusal. Update the cases in config_test.go to call a helper
(e.g., getFreePort or reserveFreePort) that does net.Listen("tcp",
"127.0.0.1:0"), captures the assigned port, closes the listener, and returns the
host:port string; then use that host:port in the Esplora URL and in the electrum
URL prefixes ("tcp://"+addr or "ssl://"+addr) and keep the same shouldError and
errorContains expectations. Ensure the helper is added to the test file (e.g.,
getFreePort) and used for both offending test cases.
In `@frontend/src/screens/settings/ChainSource.tsx`:
- Around line 61-98: The validateForm function currently allows invalid schemes
and missing ports; update it to require exact schemes and backend-compatible
formats: for esplora (in validateForm, checking formData.chainSource ===
"esplora") require formData.url.startsWith("http://") ||
formData.url.startsWith("https://") (not just "http"), and additionally ensure
the URL includes a host (e.g., non-empty hostname after the scheme). For
electrum (formData.chainSource === "electrum") require formData.url to start
with "ssl://" or "tcp://" and validate that the remainder contains host:port
(i.e., a colon and a numeric port), and that the port is a valid integer in
range 1-65535. For bitcoind_rpc keep the existing presence checks on
formData.host, formData.port, formData.user, formData.pass but tighten port
validation to ensure Number(formData.port) is an integer between 1 and 65535.
Add clear validationError messages referencing the specific field failures.
In `@http/http_service.go`:
- Around line 1600-1652: The code ignores errors returned by
httpSvc.cfg.SetUpdate in each switch case, which can mask DB write failures;
update the handler to check the returned error from every SetUpdate call (e.g.,
in the "default", "esplora", "electrum", and "bitcoind_rpc" branches) and if any
call returns an error, return an HTTP error response (e.g., c.JSON with
http.StatusInternalServerError and an ErrorResponse containing err.Error()) so
the API surfaces config write failures instead of reporting success; keep
existing input validation (ValidateChainSource and the strconv.ParseUint check)
and return early on the first SetUpdate error.
In `@lnclient/ldk/ldk.go`:
- Around line 163-176: The current bitcoind_rpc branch accepts an invalid or
empty UserBitcoindPort because it ignores ParseUint errors and treats a parsed 0
as valid; update the logic around host, portStr, strconv.ParseUint and
sourceConfigured so you only call builder.SetChainSourceBitcoindRpc and set
sourceConfigured = true when ParseUint returns no error and the parsed port is
within valid range (>0 and <=65535); if parsing fails or yields 0, do not mark
the override as configured so env fallback can proceed. Reference the host,
portStr, strconv.ParseUint call, builder.SetChainSourceBitcoindRpc and
sourceConfigured in your changes.
In `@wails/wails_handlers.go`:
- Around line 1210-1218: The JSON unmarshal error logging currently prints the
raw request body (which can contain Bitcoind RPC credentials) in
wails_handlers.go; update the error branch around the
config.UpdateChainConfigRequest decode so you do not log the raw body — replace
the "body" field with a redacted placeholder (e.g. "<redacted>" or a
masked/hashed summary) when calling logger.Logger.WithFields/WithError in that
error case, leaving other fields ("route", "method", error) intact; locate the
payload decode code that constructs payload :=
&config.UpdateChainConfigRequest{} and the subsequent json.Unmarshal error
handling and change the body field used in the log to a redacted value.
- Around line 1223-1275: The SetUpdate calls in the switch cases (e.g.,
cfg.SetUpdate in the "default", "esplora", "electrum", and "bitcoind_rpc"
branches) currently ignore returned errors which can cause a success response
without persisted changes; update each SetUpdate invocation to capture its
error, and if non-nil return a WailsRequestRouterResponse containing the error
(similar to how validation errors are returned), ensuring you short-circuit
further updates on failure so callers receive an accurate error when
cfg.SetUpdate fails.
🧹 Nitpick comments (1)
frontend/src/screens/settings/ChainSource.tsx (1)
103-109: Prevent double-submit while save is in-flight.
handleSubmitcan be triggered multiple times beforeisLoadingflips, resulting in duplicate requests.🛡️ Simple guard against repeat submits
const handleSubmit = async () => { - if (!validateForm()) { + if (isLoading || !validateForm()) { return; } setIsLoading(true);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #1985
Implemented handlers in both the HTTP server and Wails app to allow configuring the LDK chain source. You can now switch between Esplora, Electrum, and Bitcoind RPC, or reset to defaults.
Note: We will be able to display the current chain source in the UI once #2013 is merged.
Alby.Hub.-compressed.mp4
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.