Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Stellar CLI identity/secret model so --hd-path is persisted for plain seed-phrase identities (non-secure-store, non---as-secret) and then reused as the default derivation index in later commands.
Changes:
- Extend
Secret::SeedPhrasewith an optional persistedhd_pathand make key derivation fall back to it when the caller doesn’t pass an index. - Persist
--hd-pathforstellar keys generateandstellar keys addwhen storing a plain seed phrase; reject--hd-pathfor incompatiblekeys addmodes (--public-key, raw secret key). - Add/update unit tests and refresh help docs to reflect the new behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/config/secret.rs | Add hd_path to SeedPhrase secrets; default derivation uses persisted index; add TOML/back-compat tests. |
| cmd/soroban-cli/src/config/key.rs | Update key round-trip test construction for the new SeedPhrase shape. |
| cmd/soroban-cli/src/commands/keys/secret.rs | Update pattern match to tolerate the new SeedPhrase field. |
| cmd/soroban-cli/src/commands/keys/generate.rs | Persist hd_path for plain seed-phrase identities; update help text and add a test. |
| cmd/soroban-cli/src/commands/keys/add.rs | Persist hd_path when parsing seed phrases; reject hd_path for public key / secret key inputs; add tests. |
| cmd/crates/soroban-test/tests/it/util.rs | Update helper to construct SeedPhrase with hd_path: None. |
| FULL_HELP_DOCS.md | Update CLI help output text for --hd-path. |
24c7ccd to
7ad0fe1
Compare
7ad0fe1 to
008c002
Compare
Plain seed-phrase identities now persist --hd-path and honor it as a default for later derivations. Secure Store identities did the same on disk but ignored the persisted hd_path at derivation time, falling back to index 0 unless the user re-passed --hd-path; Secret::SecureStore now also falls back to the persisted value when the caller passes None, matching the behavior of Secret::SeedPhrase. Caller-supplied indices still take precedence.
d51c3ec to
c07d2c8
Compare
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.
What
Persist
--hd-pathon seed-phrase identities (both plain seed-phrase and Secure Store) forstellar keys generateandstellar keys add, and honor it as a default in later derivations:Secret::SeedPhrasegains an optionalhd_pathfield that travels with the identity.Secret::public_key/private_key/signerfall back to the persistedhd_pathwhen the caller passesNone.Secret::SecureStorealready persistedhd_pathon disk (see Cache secure-store public key in identity files. #2533), but ignored it at derivation time. It now also falls back to the persisted value when the caller passesNone, matchingSecret::SeedPhrase. Caller-supplied indices still take precedence.keys addrejects--hd-pathagainst incompatible inputs (raw secret keys,--public-key).Closes #2538.
Why
--hd-pathwas accepted but silently dropped on the plain seed-phrase path, and was persisted-but-ignored for Secure Store, so later commands derived index0unless the user re-passed the flag every time. Approach (3) from the issue (persist on the identity, schema-bumped with#[serde(default)]) keeps behavior consistent across all storage modes — without breaking identity files written before this field existed.The Secure Store fallback was added during review after empirical testing showed
keys public-key <name>returned the index-0 key even thoughhd_pathwas already on disk.Known limitations
hd_pathfield; they parse fine and behave as before (derivation index defaults to0). Re-runningkeys add/keys generatewith--hd-pathis the way to migrate them, but this is not required.--hd-pathagainst a raw secret key or--public-keyis now a hard error rather than being silently ignored. This is intentional but is a behavior change for anyone who was passing the flag in those modes.