Skip to content

fix(localnet): make [localnet].port actually take effect - #264

Open
erhnysr wants to merge 1 commit into
logos-co:masterfrom
erhnysr:fix/localnet-port-config-is-a-noop
Open

fix(localnet): make [localnet].port actually take effect#264
erhnysr wants to merge 1 commit into
logos-co:masterfrom
erhnysr:fix/localnet-port-config-is-a-noop

Conversation

@erhnysr

@erhnysr erhnysr commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #263.

[localnet].port in scaffold.toml did nothing — two bugs canceled out in opposite directions.

Sequencer side: the pinned sequencer_service binds the port given by its --port CLI flag (clap default 3040) — confirmed by reading sequencer/service/src/main.rs and lib.rs::run() directly at the pinned SHA (logos-execution-zone @ cf3639d8). It never reads the sequencer_config.json port key that prepare_sequencer_config patches for this purpose. start_localnet never passed --port, so the sequencer always bound 3040 regardless of the configured port. (test_node already does this correctly — a separate code path.)

Wallet side: prepare_wallet_home seeds a fresh wallet home by copying the vendored LEZ debug wallet_config.json verbatim. That file hardcodes sequencer_addr: http://127.0.0.1:3040. Since the key is present, load_wallet_runtime's fallback to default_sequencer_http_url_for_project (which does follow [localnet].port) never triggers.

As the issue notes, fixing only one side breaks the other — moving the sequencer without fixing the wallet breaks deploy with Connection refused. This PR does both:

  • start_localnet now passes --port <configured>, mirroring test_node's existing pattern.
  • prepare_wallet_home now takes the project's configured sequencer address and writes it into the freshly-seeded wallet config, instead of leaving the vendored 3040 default.
  • Fixed a stale doc comment on prepare_sequencer_config that incorrectly claimed the pinned sequencer doesn't accept --port.

tests/cli.rs::localnet_start_patches_config_and_uses_configured_port previously locked in the old behavior (asserted --port must not be passed, per the issue's own note); inverted here, with the fake sequencer now binding --port like the real one. Added two regression tests for the wallet-seeding half in state.rs.

  • Full suite: 587/587 lib tests pass, 186/186 cli integration tests pass.
  • No behavior change when [localnet].port is left at the default 3040.

Fixes logos-co#263. [localnet].port did nothing: two bugs canceled out.

The sequencer binds the port given by its --port CLI flag (clap
default 3040), never the sequencer_config.json 'port' key that
prepare_sequencer_config patches — that key was written but never
read for binding. start_localnet never passed --port, so the
sequencer always bound 3040 regardless of the configured port.

Separately, prepare_wallet_home seeds a fresh wallet home by copying
the vendored LEZ debug wallet_config.json verbatim, which hardcodes
sequencer_addr to http://127.0.0.1:3040. Because the key is present
(not absent), load_wallet_runtime's fallback to
default_sequencer_http_url_for_project never triggers, so the wallet
always targeted 3040 too.

Fixing only one side breaks the other: move the sequencer alone and
deploy/wallet flows fail with connection refused; leave the sequencer
alone and no wallet fix does anything. This does both:
  - start_localnet now passes --port <configured>, mirroring test_node.
  - prepare_wallet_home now overrides the seeded wallet's
    sequencer_addr with the project's configured address.

Root cause verified against the pinned sequencer_service source
(logos-execution-zone @ cf3639d8, the DEFAULT_LEZ pin) rather than
assumed from the issue text alone; this also caught a stale doc
comment on prepare_sequencer_config claiming --port isn't accepted,
now corrected.

tests/cli.rs's localnet_start_patches_config_and_uses_configured_port
previously locked in the buggy behavior (asserted --port must NOT be
passed); inverted to assert it is, with a fake sequencer that binds
--port like the real one. Added two new state.rs regression tests for
the wallet-seeding half.
@erhnysr
erhnysr requested a review from a team September 1, 2026 20:17
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@weboko weboko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix end-to-end on a non-default port, and confirmed the premise against the pinned LEZ source.

Premise checks (LEZ cf3639d8): sequencer/service/src/main.rs parses #[clap(short, long, default_value = "3040")] port and hands it to sequencer_service::run(config, port), which binds SocketAddr::from(([0,0,0,0], port)) in run_server. Nothing reads the config file's port key for RPC binding. Both halves of the PR description hold.

What I ran (fresh default-template project, [localnet].port = 3141, real sequencer + wallet):

  • setup → seeded .scaffold/wallet/wallet_config.json with "sequencer_addr": "http://127.0.0.1:3141", all four initial_accounts and the other keys intact.
  • localnet startsequencer_service …/sequencer_config.json --port 3141; 3141 open, 3040 closed.
  • doctorPASS | sequencer port 3141, PASS | wallet network config | wallet points to local sequencer, PASS | wallet usability. That row is a standing WARN on master at any non-default port, so this is the check becoming meaningful, exactly as #263 predicted.
  • wallet topupNetwork: local sequencer (http://127.0.0.1:3141); deploy hello_worldSucceeded: 1.
  • Full suite green: 587 lib / 186 CLI / 3 test-node / 5 doctests, including the inverted localnet_start_patches_config_and_uses_configured_port.

The gap: projects that already ran setup are left broken, and the error sends them the wrong way

prepare_wallet_home only writes sequencer_addr when it is seeding a fresh wallet home — and prepare_wallet_home_does_not_touch_existing_wallet_config deliberately locks that in. But the population this bug affects is precisely the projects that already ran setup with [localnet].port != 3040. Today those work by accident (both halves wrong in the same direction). After this change the sequencer moves and the wallet does not, which is the regression #263 warned about — just relocated from "everyone" to "everyone who upgrades".

Reproduced on the same project by restoring the wallet config to the pre-PR on-disk state (sequencer_addr = http://127.0.0.1:3040) while localnet ran on 3141:

$ lgs deploy hello_world_with_move_function
error: cannot deploy programs: http://127.0.0.1:3040/: Connection Failed: Connection refused (os error 111)
sequencer appears unavailable at http://127.0.0.1:3040
Run `logos-scaffold localnet start`.
Another project's sequencer may already be running and may not match this project.

Localnet is running, and the advice is to start it again. doctor does say the real thing (WARN | wallet network config with a remediation naming the file and the value), but nothing in the failing command points there, so the natural next step is a loop of localnet start / localnet status that never mentions the port mismatch.

Either fix would close it:

  1. Detect and say so — in load_wallet_runtime (or the deploy preflight), when sequencer_addr is a loopback URL whose port disagrees with [localnet].port, replace the hint with that fact: "wallet is configured for 127.0.0.1:3040 but [localnet].port is 3141 — update .scaffold/wallet/wallet_config.json or run lgs doctor". Lowest risk, and keeps the PR's "don't silently rewrite a value the user can see" stance.
  2. Repair on setup — rewrite an existing sequencer_addr when it is loopback with a disagreeing port (leave any non-loopback address alone). Fixes it without the user reading anything, at the cost of the invariant the new test asserts.

I'd take (1); (2) alone would still leave anyone who never re-runs setup stuck. Whichever you pick, it seems worth a line in #263 about the upgrade path, since the issue explicitly framed "fix one and a previously-working project breaks" as the thing to avoid.

Minor: seeding now round-trips the config through serde_json, so the written file is re-indented (4→2 spaces) and its keys come out alphabetically reordered (initial_accounts first, sequencer_addr last). Harmless, but this is a file doctor tells people to hand-edit and that some will have diffed before; serde_json's preserve_order feature would keep it recognisable if that matters to you.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[localnet].port is a no-op, and fixing only the sequencer half breaks deploy

2 participants