Skip to content

fix(wallet,deploy): default sequencer URL from scaffold localnet port#84

Merged
vpavlin merged 2 commits into
logos-co:masterfrom
crazywriter1:fix/default-sequencer-url-from-localnet-port
May 22, 2026
Merged

fix(wallet,deploy): default sequencer URL from scaffold localnet port#84
vpavlin merged 2 commits into
logos-co:masterfrom
crazywriter1:fix/default-sequencer-url-from-localnet-port

Conversation

@crazywriter1
Copy link
Copy Markdown
Contributor

@crazywriter1 crazywriter1 commented Apr 23, 2026

Problem

When wallet_config.json omits sequencer_addr, wallet topup and deploy still defaulted to http://127.0.0.1:3040. That ignores [localnet] port in scaffold.toml, so a non-default localnet port and an unset wallet address pointed RPC at the wrong URL.

Solution

  • Add default_sequencer_http_url_for_project in wallet_support to build http://127.0.0.1:{project.config.localnet.port} (same default as localnet when [localnet] is omitted).
  • Use it as the fallback when sequencer_addr is missing in both deploy and wallet topup.
  • Follow-up to fix(config): fail fast on invalid [localnet] port in scaffold.toml #83: once [localnet] port is authoritative, default sequencer_addr for deploy / wallet topup should use the same port instead of hardcoding 3040.

Tests

  • New integration test: wallet_topup_dry_run_planned_network_uses_localnet_port_without_wallet_sequencer_addr asserts dry-run output shows the URL with the configured port when the wallet omits sequencer_addr.
  • Comment on deploy fallback test updated to describe localnet.port-based fallback.

How to verify

cargo test wallet_topup_dry_run_planned_network
cargo test

@crazywriter1 crazywriter1 requested a review from a team April 23, 2026 19:20
Copy link
Copy Markdown

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

Clean extraction of a shared fallback URL helper. Replaces two hardcoded http://127.0.0.1:3040 literals with default_sequencer_http_url_for_project(), which reads from project.config.localnet.port — consistent with how localnet.rs already resolves the port.

Changes verified:

  • wallet topup now falls back to the configured localnet port when sequencer_addr is missing in wallet config ✅
  • `deploy} uses the same fallback logic ✅
  • New integration test `wallet_topup_dry_run_planned_network_uses_localnet_port_without_wallet_sequencer_addr} passes ✅
  • All 76 CLI tests pass, formatting clean ✅

One minor suggestion: the deploy-side fallback isn't covered by a new test (only the wallet topup path is tested). Consider adding a similar dry-run assertion for `deploy} to close the coverage gap. Not blocking.

Approve.

@crazywriter1
Copy link
Copy Markdown
Contributor Author

Thanks for the review and approve.

I’ve added an integration test deploy_preflight_targets_localnet_port_when_wallet_omits_sequencer_addr so the deploy path is covered as well: wallet omits sequencer_addr, scaffold.toml sets a non-default [localnet] port, and we assert the preflight error on stderr includes http://127.0.0.1:{port} together with the existing reachability messaging. Pushed in the latest commit. @vpavlin

Copy link
Copy Markdown
Contributor

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

LGTM. feel free to resolve merge conflcits and merge

@weboko
Copy link
Copy Markdown
Collaborator

weboko commented May 5, 2026

@crazywriter1 can you, please, resolve conflict so that we merge?

@crazywriter1
Copy link
Copy Markdown
Contributor Author

@crazywriter1 can you, please, resolve conflict so that we merge?

Hey, I’ll take a look and resolve the conflicts in a few hours. Thanks.

@crazywriter1 crazywriter1 force-pushed the fix/default-sequencer-url-from-localnet-port branch from 5d12629 to c5843f6 Compare May 7, 2026 18:01
@crazywriter1
Copy link
Copy Markdown
Contributor Author

Conflicts resolved on latest master, and deploy-side fallback coverage has been added (deploy_preflight_targets_localnet_port_when_wallet_omits_sequencer_addr). Full cargo test passes. @weboko

@crazywriter1 crazywriter1 force-pushed the fix/default-sequencer-url-from-localnet-port branch from c5843f6 to c4a6e68 Compare May 20, 2026 18:21
@vpavlin vpavlin merged commit 78e80bb into logos-co:master May 22, 2026
1 check passed
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.

4 participants