NONEVM-4290: improve stellar support - #2818
Merged
Merged
Conversation
|
👋 FelixFan1992, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A source-compatible API break and unresolved test robustness and validation issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Stellar quickstart configuration with architecture-aware platform selection, RPC readiness checks, and integration coverage.
Changes:
- Updates Stellar container startup and health validation.
- Adds Stellar RPC, network, and Friendbot integration tests.
File summaries
| File | Summary and review findings |
|---|---|
framework/components/blockchain/stellar.go |
Updates container configuration and readiness handling. Critical (2 votes): preserve DefaultStellarFriendbotPort as a deprecated alias. Nits (2, 1 votes): update platform-default documentation and describe the RPC flag as deprecated rather than invalid. |
framework/components/blockchain/blockchain_test.go |
Adds end-to-end Stellar tests. Moderate (1 vote each): restrict Friendbot success handling, bound Friendbot requests, and bound RPC requests with timeouts. |
Review details
Suppressed comments (4)
framework/components/blockchain/blockchain_test.go:155
- This treats every HTTP 400 as a successful funding attempt, so an invalid address/query or a regression to the Friendbot route can make the test pass without verifying that the account was funded. Restrict this to HTTP 200, or inspect the 400 response and accept it only when it is the documented already-funded response (and otherwise fail).
return resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusBadRequest
framework/components/blockchain/blockchain_test.go:150
require.Eventuallycannot enforce its two-minute deadline while this callback is blocked inhttp.Get: the default HTTP client has no request timeout, so a stalled Friendbot connection can hang the test indefinitely. Use a boundedhttp.Client(or a context derived from the retry deadline) for each attempt.
resp, gerr := http.Get(fmt.Sprintf("%s?addr=%s", netInfo.FriendbotURL, addr)) //nolint:gosec
framework/components/blockchain/blockchain_test.go:172
- This helper uses
http.Postwith the package default client, which has no timeout. If the container accepts the connection but stops responding, the new health/network assertions can hang indefinitely instead of failing the test; use a boundedhttp.Clientfor the RPC call.
resp, err := http.Post(rpcURL, "application/json", strings.NewReader(string(body))) //nolint:gosec
framework/components/blockchain/stellar.go:79
- The comment says
--enable-soroban-rpcis no longer valid, but the quickstart entrypoint still accepts that spelling as a deprecated compatibility alias. Removing it from the default command is fine because all services run by default, but this explanation should say deprecated rather than invalid.
// The older "--enable-soroban-rpc" flag is no longer valid; the current form is "--enable"
// with a comma-separated service list, which is only used to run a subset.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
68
to
+71
| imagePlatform := "linux/amd64" | ||
| if runtime.GOARCH == "arm64" { | ||
| imagePlatform = "linux/arm64" | ||
| } |
FelixFan1992
changed the title
FelixFan1992
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Friendbot probes need bounded timeouts and must distinguish funding failures from already-funded responses.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
framework/components/blockchain/blockchain_test.go:172
- The new RPC helper calls
http.Postthrough the default client, which has no request timeout. If the local gateway hangs after startup, either the health/network assertion or this helper can block forever and bypass the test's failure timeout; use a bounded HTTP client or request context here.
resp, err := http.Post(rpcURL, "application/json", strings.NewReader(string(body))) //nolint:gosec
framework/components/blockchain/blockchain_test.go:156
- A Friendbot 400 can represent an invalid request or a funding failure, not only an already-funded account. Treating every 400 as success means this new integration test can pass even when the faucet never funds the account; inspect the error detail or verify the account before accepting the retry.
return resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusBadRequest
}, 2*time.Minute, 5*time.Second, "friendbot never became ready at %s", netInfo.FriendbotURL)
framework/components/blockchain/stellar.go:88
- This comment does not match the quickstart service defaults:
--localdefaults to core, horizon, and RPC, while Lab is optional, and the upstream parser still accepts--enable-soroban-rpcfor compatibility. Please describe the actual defaults so future callers do not assume Lab is running or that the legacy flag is invalid.
// Build the command arguments. In --local mode the quickstart image runs all services by
// default (core, horizon, Soroban RPC, Friendbot, Lab), so no service-enable flag is needed.
// The older "--enable-soroban-rpc" flag is no longer valid; the current form is "--enable"
// with a comma-separated service list, which is only used to run a subset.
// https://github.com/stellar/quickstart#usage
framework/examples/myproject/smoke_stellar_test.go:89
- A Friendbot 400 is not synonymous with an already-funded account; it is also used for invalid requests and funding failures. Returning true for every 400 can make this smoke test pass without funding the account. Only accept a 400 after checking the response detail for the already-funded error, or verify the account through RPC.
// 200 = funded, 400 = already funded on a reused/cached network — both are success.
return resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusBadRequest
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
| // cached/reused network — both are success. | ||
| addr := "GAAZI4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWN7" | ||
| require.Eventually(t, func() bool { | ||
| resp, gerr := http.Get(fmt.Sprintf("%s?addr=%s", netInfo.FriendbotURL, addr)) //nolint:gosec |
| // until ready), so retry until it actually accepts the funding request instead of | ||
| // silently passing on a "still initializing" response. | ||
| require.Eventually(t, func() bool { | ||
| resp, err := http.Get(friendbotURL) //nolint:gosec |
skudasov
approved these changes
Sep 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.