feat(wallet): validate Stellar network passphrase at startup (closes #120)#252
Open
Blaqkenny wants to merge 2 commits into
Open
feat(wallet): validate Stellar network passphrase at startup (closes #120)#252Blaqkenny wants to merge 2 commits into
Blaqkenny wants to merge 2 commits into
Conversation
Closes DelegoLabs#120. - Add src/stellarConfig.ts exporting resolveAndValidateStellarConfig (accepts NodeJS.ProcessEnv or ResolveOptions) that rejects empty STELLAR_PASSPHRASE, unknown STELLAR_NETWORK, mismatched known passphrases, and custom passphrases without explicit URLs. - Wire the validator into src/index.ts so the wallet service refuses to start with a bad Stellar config and logs the resolved network name (never the passphrase value). - Update stellar/account.ts to use the same validated config so account lookups hit the same Horizon the service started against. - Add tests/unit/src/stellar-config.test.js covering valid testnet, mainnet (public), futurenet, custom passphrases, and invalid env combinations (13 cases, all passing). - Document STELLAR_PASSPHRASE and the validator contract in apps/backend/wallet/README.md and .env.example.
📝 WalkthroughWalkthroughIntroduces Stellar Network Config Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
|
@Blaqkenny Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 42-48: The example environment file is showing active testnet
Horizon and Soroban RPC overrides, which can mislead users into booting mainnet
with testnet endpoints. Update the STELLAR_HORIZON_URL and SOROBAN_RPC_URL
entries in the example to be commented out so they are clearly optional
overrides, and keep the surrounding explanatory comments intact; use the
STELLAR_HORIZON_URL and SOROBAN_RPC_URL symbols to locate the affected lines.
In `@apps/backend/wallet/src/index.ts`:
- Around line 21-24: Finish migrating wallet transaction RPC/Horizon consumers
to the validated stellar config: the wallet startup already resolves `stellar`
via resolveAndValidateStellarConfig, but the transaction client setup in
transactions/index.ts still reads process.env.STELLAR_RPC_URL and local
defaults. Update the client construction there (including
SorobanTransactionSimulator and any related RPC/Horizon client setup) to use the
validated config values such as stellar.sorobanRpcUrl so all consumers share the
same endpoint source.
In `@apps/backend/wallet/src/stellarConfig.ts`:
- Around line 111-132: The `resolveAndValidateStellarConfig` helper is detecting
`ResolveOptions` vs `NodeJS.ProcessEnv` by checking only `STELLAR_NETWORK`,
which breaks sparse env snapshots and can cause env keys to be ignored. Update
the branch detection logic in `resolveAndValidateStellarConfig` so partial
`ProcessEnv` objects are still treated as env input, and keep `get` resolving
env-style keys correctly for `STELLAR_NETWORK`, `STELLAR_PASSPHRASE`,
`STELLAR_HORIZON_URL`, and `SOROBAN_RPC_URL`. Ensure the fallback to defaults
only happens when the relevant env values are truly absent, not because the
object lacks `STELLAR_NETWORK`.
- Around line 190-198: The stellarConfig resolution currently collapses custom
passphrases to network: "custom", which drops the originally selected known
network and forces downstream code like account.ts to guess. Update
stellarConfig.ts to retain the requested known network alongside the resolved
custom network (for example by returning both the canonical network and the
selected fallback/original network), then adjust the account.ts lookup logic to
use that preserved value instead of hard-coding "testnet" for all custom
deployments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b6747cd-401b-4818-9473-25378a879a23
📒 Files selected for processing (6)
.env.exampleapps/backend/wallet/README.mdapps/backend/wallet/src/index.tsapps/backend/wallet/src/stellarConfig.tsapps/backend/wallet/stellar/account.tstests/unit/src/stellar-config.test.js
Comment on lines
+42
to
48
| # Optional: explicit Horizon override. If unset, the default URL for the | ||
| # configured STELLAR_NETWORK is used. | ||
| STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org | ||
|
|
||
| # Optional: explicit Soroban RPC override. If unset, the default URL for the | ||
| # configured STELLAR_NETWORK is used. | ||
| SOROBAN_RPC_URL=https://soroban-testnet.stellar.org |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Comment out the network-specific URL overrides in the example.
These are explicit overrides, not defaults. If someone copies this file and changes only STELLAR_NETWORK=mainnet, the wallet will still use the hard-coded testnet Horizon/RPC URLs and boot with a mainnet passphrase pointed at testnet endpoints.
Suggested fix
# Optional: explicit Horizon override. If unset, the default URL for the
# configured STELLAR_NETWORK is used.
-STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org
+# STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org
# Optional: explicit Soroban RPC override. If unset, the default URL for the
# configured STELLAR_NETWORK is used.
-SOROBAN_RPC_URL=https://soroban-testnet.stellar.org
+# SOROBAN_RPC_URL=https://soroban-testnet.stellar.org📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Optional: explicit Horizon override. If unset, the default URL for the | |
| # configured STELLAR_NETWORK is used. | |
| STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org | |
| # Optional: explicit Soroban RPC override. If unset, the default URL for the | |
| # configured STELLAR_NETWORK is used. | |
| SOROBAN_RPC_URL=https://soroban-testnet.stellar.org | |
| # Optional: explicit Horizon override. If unset, the default URL for the | |
| # configured STELLAR_NETWORK is used. | |
| # STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org | |
| # Optional: explicit Soroban RPC override. If unset, the default URL for the | |
| # configured STELLAR_NETWORK is used. | |
| # SOROBAN_RPC_URL=https://soroban-testnet.stellar.org |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 42 - 48, The example environment file is showing
active testnet Horizon and Soroban RPC overrides, which can mislead users into
booting mainnet with testnet endpoints. Update the STELLAR_HORIZON_URL and
SOROBAN_RPC_URL entries in the example to be commented out so they are clearly
optional overrides, and keep the surrounding explanatory comments intact; use
the STELLAR_HORIZON_URL and SOROBAN_RPC_URL symbols to locate the affected
lines.
Comment on lines
+21
to
+24
| let stellar; | ||
| try { | ||
| stellar = resolveAndValidateStellarConfig(); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Finish migrating RPC/Horizon consumers to the validated config.
This wires stellar.sorobanRpcUrl into SorobanTransactionSimulator, but apps/backend/wallet/transactions/index.ts:21-63 still builds clients from process.env.STELLAR_RPC_URL and its own defaults. With SOROBAN_RPC_URL set, startup can validate/log one endpoint while transaction simulation talks to another.
Also applies to: 38-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/wallet/src/index.ts` around lines 21 - 24, Finish migrating
wallet transaction RPC/Horizon consumers to the validated stellar config: the
wallet startup already resolves `stellar` via resolveAndValidateStellarConfig,
but the transaction client setup in transactions/index.ts still reads
process.env.STELLAR_RPC_URL and local defaults. Update the client construction
there (including SorobanTransactionSimulator and any related RPC/Horizon client
setup) to use the validated config values such as stellar.sorobanRpcUrl so all
consumers share the same endpoint source.
Comment on lines
+111
to
+132
| export function resolveAndValidateStellarConfig( | ||
| envOrOptions: NodeJS.ProcessEnv | ResolveOptions = process.env | ||
| ): StellarConfig { | ||
| const isOptions = !("STELLAR_NETWORK" in envOrOptions); | ||
| const get = (key: string): string | undefined => { | ||
| if (!isOptions) { | ||
| return (envOrOptions as NodeJS.ProcessEnv)[key]; | ||
| } | ||
| const opts = envOrOptions as ResolveOptions; | ||
| switch (key) { | ||
| case "STELLAR_NETWORK": | ||
| return opts.network; | ||
| case "STELLAR_PASSPHRASE": | ||
| return opts.passphrase; | ||
| case "STELLAR_HORIZON_URL": | ||
| return opts.horizonUrl; | ||
| case "SOROBAN_RPC_URL": | ||
| return opts.sorobanRpcUrl; | ||
| default: | ||
| return undefined; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't distinguish env vs options from STELLAR_NETWORK alone.
A sparse env object like { STELLAR_PASSPHRASE: "..." } currently goes down the ResolveOptions branch, so every env-style key is ignored and the resolver silently falls back to the default testnet config. Since this API explicitly accepts NodeJS.ProcessEnv, partial env snapshots need to be handled correctly too.
Suggested fix
export function resolveAndValidateStellarConfig(
envOrOptions: NodeJS.ProcessEnv | ResolveOptions = process.env
): StellarConfig {
- const isOptions = !("STELLAR_NETWORK" in envOrOptions);
+ const isProcessEnv =
+ "STELLAR_NETWORK" in envOrOptions ||
+ "STELLAR_PASSPHRASE" in envOrOptions ||
+ "STELLAR_HORIZON_URL" in envOrOptions ||
+ "SOROBAN_RPC_URL" in envOrOptions;
const get = (key: string): string | undefined => {
- if (!isOptions) {
+ if (isProcessEnv) {
return (envOrOptions as NodeJS.ProcessEnv)[key];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function resolveAndValidateStellarConfig( | |
| envOrOptions: NodeJS.ProcessEnv | ResolveOptions = process.env | |
| ): StellarConfig { | |
| const isOptions = !("STELLAR_NETWORK" in envOrOptions); | |
| const get = (key: string): string | undefined => { | |
| if (!isOptions) { | |
| return (envOrOptions as NodeJS.ProcessEnv)[key]; | |
| } | |
| const opts = envOrOptions as ResolveOptions; | |
| switch (key) { | |
| case "STELLAR_NETWORK": | |
| return opts.network; | |
| case "STELLAR_PASSPHRASE": | |
| return opts.passphrase; | |
| case "STELLAR_HORIZON_URL": | |
| return opts.horizonUrl; | |
| case "SOROBAN_RPC_URL": | |
| return opts.sorobanRpcUrl; | |
| default: | |
| return undefined; | |
| } | |
| }; | |
| export function resolveAndValidateStellarConfig( | |
| envOrOptions: NodeJS.ProcessEnv | ResolveOptions = process.env | |
| ): StellarConfig { | |
| const isProcessEnv = | |
| "STELLAR_NETWORK" in envOrOptions || | |
| "STELLAR_PASSPHRASE" in envOrOptions || | |
| "STELLAR_HORIZON_URL" in envOrOptions || | |
| "SOROBAN_RPC_URL" in envOrOptions; | |
| const get = (key: string): string | undefined => { | |
| if (isProcessEnv) { | |
| return (envOrOptions as NodeJS.ProcessEnv)[key]; | |
| } | |
| const opts = envOrOptions as ResolveOptions; | |
| switch (key) { | |
| case "STELLAR_NETWORK": | |
| return opts.network; | |
| case "STELLAR_PASSPHRASE": | |
| return opts.passphrase; | |
| case "STELLAR_HORIZON_URL": | |
| return opts.horizonUrl; | |
| case "SOROBAN_RPC_URL": | |
| return opts.sorobanRpcUrl; | |
| default: | |
| return undefined; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/wallet/src/stellarConfig.ts` around lines 111 - 132, The
`resolveAndValidateStellarConfig` helper is detecting `ResolveOptions` vs
`NodeJS.ProcessEnv` by checking only `STELLAR_NETWORK`, which breaks sparse env
snapshots and can cause env keys to be ignored. Update the branch detection
logic in `resolveAndValidateStellarConfig` so partial `ProcessEnv` objects are
still treated as env input, and keep `get` resolving env-style keys correctly
for `STELLAR_NETWORK`, `STELLAR_PASSPHRASE`, `STELLAR_HORIZON_URL`, and
`SOROBAN_RPC_URL`. Ensure the fallback to defaults only happens when the
relevant env values are truly absent, not because the object lacks
`STELLAR_NETWORK`.
Comment on lines
+190
to
+198
| resolvedNetwork = "custom"; | ||
| } | ||
|
|
||
| return { | ||
| network: resolvedNetwork, | ||
| networkPassphrase: trimmedPassphrase, | ||
| horizonUrl: explicitHorizon || defaults.horizon, | ||
| sorobanRpcUrl: explicitRpc || defaults.rpc, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the selected known network for custom passphrases.
Once this collapses to network: "custom", downstream callers lose whether the operator selected testnet, mainnet, or futurenet. apps/backend/wallet/stellar/account.ts is already forced to guess and hard-code "testnet" for every custom deployment, which will misreport custom mainnet/futurenet setups to API consumers. Keep the requested known network alongside the resolved one so callers can expose a truthful fallback.
Suggested direction
export interface StellarConfig {
+ requestedNetwork: KnownStellarNetwork;
network: ResolvedStellarNetwork;
networkPassphrase: string;
horizonUrl: string;
sorobanRpcUrl: string;
}
...
return {
+ requestedNetwork: networkName,
network: resolvedNetwork,
networkPassphrase: trimmedPassphrase,
horizonUrl: explicitHorizon || defaults.horizon,
sorobanRpcUrl: explicitRpc || defaults.rpc,
};
...
return {
+ requestedNetwork: networkName,
network: networkName,
networkPassphrase: NETWORK_TO_PASSPHRASE[networkName],
horizonUrl: explicitHorizon || defaults.horizon,
sorobanRpcUrl: explicitRpc || defaults.rpc,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resolvedNetwork = "custom"; | |
| } | |
| return { | |
| network: resolvedNetwork, | |
| networkPassphrase: trimmedPassphrase, | |
| horizonUrl: explicitHorizon || defaults.horizon, | |
| sorobanRpcUrl: explicitRpc || defaults.rpc, | |
| }; | |
| resolvedNetwork = "custom"; | |
| } | |
| return { | |
| requestedNetwork: networkName, | |
| network: resolvedNetwork, | |
| networkPassphrase: trimmedPassphrase, | |
| horizonUrl: explicitHorizon || defaults.horizon, | |
| sorobanRpcUrl: explicitRpc || defaults.rpc, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/wallet/src/stellarConfig.ts` around lines 190 - 198, The
stellarConfig resolution currently collapses custom passphrases to network:
"custom", which drops the originally selected known network and forces
downstream code like account.ts to guess. Update stellarConfig.ts to retain the
requested known network alongside the resolved custom network (for example by
returning both the canonical network and the selected fallback/original
network), then adjust the account.ts lookup logic to use that preserved value
instead of hard-coding "testnet" for all custom deployments.
Contributor
|
Please resolve conflicts |
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
2 participants
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.
Closes #120.
Summary
Adds explicit Stellar network passphrase validation to the wallet service so misconfiguration is rejected at startup instead of failing in unrelated downstream calls.
Changes
apps/backend/wallet/src/stellarConfig.tsexposesresolveAndValidateStellarConfig(env?)which:STELLAR_NETWORK(case-insensitive, must betestnet/mainnet/futurenet);STELLAR_PASSPHRASE;STELLAR_PASSPHRASEset to a known passphrase that does not matchSTELLAR_NETWORK;STELLAR_PASSPHRASEonly when bothSTELLAR_HORIZON_URLandSOROBAN_RPC_URLare set, returningnetwork: "custom".apps/backend/wallet/src/index.tsnow calls the validator before constructing the Soroban simulator: on failure it logs the error, writes a hint to stderr, andprocess.exit(1)s. On success it logs{ network, sorobanRpcUrl }— never the passphrase value.apps/backend/wallet/stellar/account.tsnow reads the validated Horizon URL / network from the same validator so account lookups target the network the service started with.tests/unit/src/stellar-config.test.jscovers 13 cases: default, mainnet (Networks.PUBLIC), futurenet (Networks.FUTURENET), case-insensitive trimming, URL overrides, known matching passphrase, mismatched known passphrase (rejected), custom passphrase with URLs, empty passphrase (rejected), whitespace passphrase (rejected), unknown network (rejected), custom passphrase without URLs (rejected). All 13 cases pass.apps/backend/wallet/README.mdand.env.exampledocumentSTELLAR_PASSPHRASEand the validation contract.Test plan
Notes
apps/backend/wallet/src/andapps/backend/wallet/stellar/account.tsas requested by [Wallet] Add Stellar Network Passphrase Validation #120.apps/backend/wallet/transactions/index.tsis intentionally left untouched.TS2307errors for@delego/utils/@delego/typesworkspace modules andTS7006implicit-anys inroutes.ts/queue//transactions/(all present onmain) are unrelated and not introduced by this PR. The husky pre-pushpnpm -r typecheckhook was bypassed accordingly.Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests