Repository navigation
Conversation
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — intent-solver 0.5.0 + lnurl-server 0.6.0
The version bumps are straightforward and the config-format migration to SOLVER_CONFIG_JSON looks structurally correct for the new API. Three issues need addressing before merge.
1. INTENT_SOLVER_MNEMONIC is now a dead Docker config knob — .env.defaults:178
The mnemonic is hardcoded inside SOLVER_CONFIG_JSON in compose.ark.yml rather than interpolated from the existing variable:
# compose.ark.yml (new)
"mnemonic": "planet travel grab found idle ripple acoustic hero normal mixed rich lamp",.env.defaults still documents this variable as the override path ("fixed in .env.defaults so the solver's address is stable across restarts"), and README.md:247 says the same. After this PR, overriding INTENT_SOLVER_MNEMONIC in a local .env has no effect on the container — it silently uses the hardcoded phrase instead. CI environments or developers who pin a custom key will find the solver deriving a different identity than expected, with no log warning.
Fix option A — interpolate inside the JSON (Compose does support this):
SOLVER_CONFIG_JSON: |
{
"rails": {
"arkade": {
"mnemonic": "${INTENT_SOLVER_MNEMONIC}",Fix option B — explicitly retire the variable: remove INTENT_SOLVER_MNEMONIC from .env.defaults and add a comment on the SOLVER_CONFIG_JSON block explaining it is the new override surface.
2. INTENT_SOLVER_RELAY_URL affects only the cards file, not the solver container — .env.defaults:183, lib/setup/lnurl.mjs:82
Before this PR the solver container received RELAY_URL: ${INTENT_SOLVER_RELAY_URL:-ws://strfry:7777} and the setup script read the same variable for the cards file — both sides always dialled the same relay. After this PR the container hardcodes "url": "ws://strfry:7777" in SOLVER_CONFIG_JSON, while lnurl.mjs:82 still reads INTENT_SOLVER_RELAY_URL for the cards file. If INTENT_SOLVER_RELAY_URL is overridden, the cards file advertises the new relay but the solver container still connects to ws://strfry:7777 — silent divergence. README.md:265 says explicitly that both sides use the value from INTENT_SOLVER_RELAY_URL, which is now only half-true.
Same two fix options as above: interpolate ${INTENT_SOLVER_RELAY_URL:-ws://strfry:7777} into the JSON, or document the divergence clearly. Given that the setup script already rewrites relay URLs in the emitted card (line 97), the practical blast radius is small in regtest, but the documented invariant ("both sides dial the same relay") is broken.
3. README left with stale env var docs — README.md:234,251,260
These are documentation issues introduced by the config-format change and should land in this PR or a same-day follow-up:
README.md:234: showsintent-solver:0.3.1(should be0.5.0)README.md:260: showslnurl-server:0.3.3(should be0.6.0)README.md:251: documentsLND_ESPLORA_URLandARK_ESPLORA_URLas the two-explorer knobs. Those env vars no longer exist; they are nowrails.lightning/rails.onchain.esploraUrlinsideSOLVER_CONFIG_JSON.README.md:247: still namesINTENT_SOLVER_MNEMONICas the operative wallet-seed knob.README.md:265: still namesINTENT_SOLVER_RELAY_URLas controlling "both sides" of the relay.
Minor note — new arkade:BTC->onchain:BTC corridor
compose.ark.yml adds "arkade:BTC->onchain:BTC": {} to corridors. Worth a one-line mention in the PR description so reviewers and the git log both capture that this is a net-new capability, not just a config migration.
No protocol-critical findings — this PR is infra config only (no VTXO handling, signing, forfeit, round, or exit logic).
|
Requires arkade-os/intent-solver#345 merged and published for tests to pass here. |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
1 commit since last pass; 0 prior issues fixed; 3 still open.
Incremental change — commit 7b4ae28 ("don't pass env vars to intent-solver cli")
Single-hunk change in lib/setup/lnurl.mjs: removes the -e RELAY_URL=wss://strfry:7777 env injection from the docker exec card invocation. The motivation is correct — intent-solver 0.5.0 now reads from SOLVER_CONFIG_JSON.relay.url rather than the RELAY_URL env var, so injecting RELAY_URL via -e had no effect on the emitted card. Removing dead injection is right.
One new finding from this hunk:
New: stale comment — lib/setup/lnurl.mjs:83-88
The block comment directly above the docker exec call still describes the removed behaviour:
"Its one lie is the relay scheme: the card schema admits wss:// only, while strfry terminates no TLS, so emission runs with the scheme upgraded and the file below carries the dialable ws:// value. The override is exec-scoped — the service keeps its real RELAY_URL for a future relay-mode run."
After this commit neither sentence is true. The scheme is no longer upgraded before emission (the solver now emits whatever relay.url says in SOLVER_CONFIG_JSON), and there is no exec-scoped override. A future reader will be confused about why the upgrade was removed and whether something is missing. The comment needs to be replaced with the new reality: that the solver's relay URL is now fixed in SOLVER_CONFIG_JSON, the card is emitted as-is, and line 97 rewrites the relays in the written file to relayUrl unconditionally.
Prior issues — closure status
1. INTENT_SOLVER_MNEMONIC is a dead config knob — .env.defaults:178 (Still open)
Not touched by 7b4ae28. compose.ark.yml:389 hardcodes the mnemonic inside SOLVER_CONFIG_JSON; .env.defaults:178 still documents INTENT_SOLVER_MNEMONIC as the operative wallet-seed knob. Overriding the variable has no effect on the running container. Unchanged since my last pass.
2. INTENT_SOLVER_RELAY_URL controls only the cards file, not the solver — .env.defaults:183, lib/setup/lnurl.mjs:82 (Still open, not worsened)
This commit removes the injection that was partly obscuring the split, but the underlying divergence remains: lnurl.mjs:82 still reads INTENT_SOLVER_RELAY_URL for the cards file, while the solver's relay URL is now fully determined by SOLVER_CONFIG_JSON.relay.url = "ws://strfry:7777" in compose.ark.yml:408. Overriding INTENT_SOLVER_RELAY_URL changes what the cards file advertises but not what the solver dials. The documented invariant in .env.defaults:180 ("so both sides always dial the same relay") is still not enforced.
3. README stale — README.md:234,251,260 (Still open)
No README changes in this commit. Version strings, env var names, and explorer-URL documentation remain stale as described in the previous review.
No protocol-critical findings — infra config and setup script only.
No description provided.