fix: resolve dependency addresses from the bridge address, not assignedPort - #4
Conversation
d68aaf9 to
4e902f3
Compare
MattDHill
left a comment
There was a problem hiding this comment.
Reviewed: resolves dependency addresses via sdk.host.getBridgeAddress rather than the raw net.assignedPort / assignedSslPort fields, which are only populated depending on how the dependency bound the port. Verified live on a 0.4.0-beta.10 box across lnd, cln, fulcrum and lnbits.
`net.assignedPort` and `net.assignedSslPort` are raw metadata, and which of them is populated depends on how the dependency bound the port: a binding with addSsl frees `assignedPort` entirely. Reading either field directly breaks the moment a dependency changes its TLS arrangement, as LND just did (Start9Labs/lnd-startos#171). start-sdk 2.0.9 adds `sdk.host.getBridgeAddress`, which resolves the binding's own derived address — correct under either arrangement, and computed per binding so it also covers bridge-only bindings such as tor's SOCKS proxy. Adopt it and delete the local copy of the helper this package was carrying. `ssl` is passed only where the target publishes both a plaintext and a TLS address; elsewhere pinning it would assert a fact about how the dependency binds.
4e902f3 to
68a53fa
Compare
|
Thanks for the review. One change since the sha you approved, plus what I checked before treating this as shippable. Delta --- a/startos/utils.ts
+++ b/startos/utils.ts
@@ -1,5 +1,3 @@
-import { sdk } from './sdk'
-
export const uiPort = 80
Verified beyond CI
One nit I left alone. The bump splits the SDK against the git dep, so the bundle goes 2.66 MB → 4.51 MB uncompressed. Compressed in the s9pk that's ~+336 KB on ~41.8 MB (+0.8%), against a bitcoind dependency measured in hundreds of GB. I left it because the house call is to fix skew at the source rather than pin an Merge and ordering are yours — merging publishes to the community registry, and this is one of 44 in the pass. |
The docs still described the local helper this package no longer has, and in places asserted its internals — reading `net.assignedPort`, "never `addressInfo` hostnames" — which is the opposite of what `sdk.host.getBridgeAddress` does. Also drops imports left dead by the helper's removal.
MattDHill
left a comment
There was a problem hiding this comment.
Re-approving after the doc-sync commit. Includes Helix's fixes on this branch, which correctly caught the dead sdk import and the stale AGENTS.md reference — both turned out to be fleet-wide and are now fixed everywhere.
Why
This package resolved its dependencies' addresses by reading
bindings[<port>].net.assignedPort. That field is raw metadata, and which port field is populated depends on how the dependency bound the port — a binding withaddSslfreesassignedPortentirely and carries onlyassignedSslPort.LND hit exactly this when it moved REST behind the OS reverse proxy (Start9Labs/lnd-startos#171): every dependent silently resolved
null.What changed
Adopts
sdk.host.getBridgeAddress, added in start-sdk 2.0.9 (Start9Labs/start-technologies#3560, #3561), and deletes the local copy of the helper this package was carrying — one of 44 near-identical copies across the fleet.The helper resolves the binding's own derived bridge address, which is correct whether the dependency terminates its own TLS or hands the port to the OS proxy. It is computed per binding rather than per exported interface, so it also resolves bridge-only bindings such as tor's SOCKS proxy.
ssl:is passed only where the target binding publishes two bridge addresses —protocol: 'http'/'ws', orsecure: nullwithaddSsl. bitcoind's RPC is the clearest case (10.0.3.1:8332 ssl=falsealongside10.0.3.1:54404 ssl=true), so an undiscriminated lookup there is order-dependent. Where a binding publishes one address no discriminator is passed — pinning one would assert a fact about how the dependency binds, the coupling this change removes.On the lockfile diff
Larger than expected, and expected to stay that way for now. This package pins git dependencies that track
#nextbranches, which still resolve start-sdk 2.0.7, so npm nests a second SDK copy rather than hoisting one. It collapses once thosenextbranches carry 2.0.9. The lockfile has to be committed regardless —s9pk.mkand the reusable CI both runnpm ci, which fails on a lockfile out of sync withpackage.json. Functionally inert: the git deps are imported for types, and the s9pk build tree-shakes.Verification
tscand prettier clean against the published start-sdk 2.0.9. The resolution logic was verified live on a StartOS 0.4.0-beta.10 box: with LND onaddSsl,net.assignedPortreadsnullwhile the binding's bridge entry resolves to10.0.3.1:8080; LNbits connected through it (✔️ Backend LndRestWallet connected), and Fulcrum picked bitcoind's plaintext:8332leg over the TLS:54404one.Not exercised against a running instance of this service — compile-checked only.
Test plan
.const()should settle on one value and stay there.