Skip to content

[cli] Fix windows faucet wallet issue #24384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Nov 26, 2025
Merged

[cli] Fix windows faucet wallet issue #24384

merged 11 commits into from
Nov 26, 2025
+35 −39

Conversation

@stefan-mysten
Copy link
Contributor

@stefan-mysten stefan-mysten commented Nov 21, 2025 •
edited
Loading

Description

This PR fixes an issue flagged in #24041. Unfortunately, Windows does not allow clients to connect to 0.0.0.0, which was the case here, and led to an error when the faucet had to make an RPC client to node.

Test plan

Existing tests. Manually tested on Windows!

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • Indexing Framework:

@stefan-mysten stefan-mysten self-assigned this Nov 21, 2025
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 21, 2025 01:41 — with GitHub Actions Inactive
@vercel
Copy link

vercel bot commented Nov 21, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
sui-docs Ready Ready Preview Comment Nov 25, 2025 9:33pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
multisig-toolkit Ignored Ignored Preview Nov 25, 2025 9:33pm
sui-kiosk Ignored Ignored Preview Nov 25, 2025 9:33pm

stefan-mysten
stefan-mysten commented Nov 21, 2025
View reviewed changes
damirka
damirka approved these changes Nov 21, 2025
View reviewed changes
amnn
amnn reviewed Nov 21, 2025
View reviewed changes
Scetrov
Scetrov reviewed Nov 21, 2025
View reviewed changes
Copy link

@Scetrov Scetrov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Feel free to @ me if you want me to to test this locally once you have settled the review.

@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 24, 2025 18:54 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten requested a review from a team as a code owner November 25, 2025 00:05
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 00:05 — with GitHub Actions Inactive
stefan-mysten
stefan-mysten commented Nov 25, 2025
View reviewed changes
crates/sui-indexer-alt-reader/src/consistent_reader.rs
.connect()
.await
.context("Failed to connect to gRPC endpoint")?;
let channel = endpoint.connect_lazy();
Copy link
Contributor Author

@stefan-mysten stefan-mysten Nov 25, 2025

Choose a reason for hiding this comment

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

We need to connect lazily, as otherwise the grpc server might not be ready and the whole system errors due to being unable to connect to server.

@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 00:23 — with GitHub Actions Inactive
stefan-mysten
stefan-mysten commented Nov 25, 2025
View reviewed changes
stefan-mysten
stefan-mysten commented Nov 25, 2025
View reviewed changes
@stefan-mysten
Copy link
Contributor Author

stefan-mysten commented Nov 25, 2025 •
edited
Loading

Feel free to @ me if you want me to to test this locally once you have settled the review.

@Scetrov I think the fix is now ready. I am also planning to add some tests.

On my Windows machine, I built from source and then ran
sui start --force-regenesis --with-faucet --with-graphql --> with-graphql requires a PostgresDB.

@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 17:21 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 17:28 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 17:29 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 17:30 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten force-pushed the fix-windows-faucet-wallet-rpc branch from 087c1f0 to 8e1aef2 Compare November 25, 2025 17:53
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 17:53 — with GitHub Actions Inactive
@stefan-mysten
Copy link
Contributor Author

stefan-mysten commented Nov 25, 2025

@amnn I touched the indexing side in three places and moved from connect to connect_lazy. I figure tests should catch any issues if these changes were to mess up with the grpc connections.

amnn
amnn approved these changes Nov 25, 2025
View reviewed changes
Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Yep, tests should have you covered! Couple more clean up questions, but this looks good to go, thanks @stefan-mysten

@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 25, 2025 21:31 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten merged commit 63f36a8 into MystenLabs:main Nov 26, 2025
68 of 69 checks passed
@stefan-mysten stefan-mysten deleted the fix-windows-faucet-wallet-rpc branch November 26, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amnn amnn amnn approved these changes

@damirka damirka damirka approved these changes

@jordanjennings-mysten jordanjennings-mysten Awaiting requested review from jordanjennings-mysten

@joyqvq joyqvq Awaiting requested review from joyqvq joyqvq is a code owner

@henryachen henryachen Awaiting requested review from henryachen henryachen is a code owner automatically assigned from MystenLabs/devx-indexers-and-rpcs

@emmazzz emmazzz Awaiting requested review from emmazzz emmazzz is a code owner automatically assigned from MystenLabs/devx-indexers-and-rpcs

@wlmyng wlmyng Awaiting requested review from wlmyng wlmyng is a code owner automatically assigned from MystenLabs/devx-indexers-and-rpcs

@tpham-mysten tpham-mysten Awaiting requested review from tpham-mysten tpham-mysten is a code owner automatically assigned from MystenLabs/devx-indexers-and-rpcs

@evan-wall-mysten evan-wall-mysten Awaiting requested review from evan-wall-mysten evan-wall-mysten is a code owner automatically assigned from MystenLabs/devx-indexers-and-rpcs

+2 more reviewers

@imamnugrh imamnugrh imamnugrh left review comments

@Scetrov Scetrov Scetrov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@stefan-mysten stefan-mysten

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@stefan-mysten @amnn @damirka @imamnugrh @Scetrov