fix: tron derivation path - #1003
Conversation
zerodevblock-cyber
left a comment
zerodevblock-cyber
left a comment
There was a problem hiding this comment.
Reviewed CR-001, CR-004, and CR-005 against PR head 66eb9ad1d1b898eaa2e355fcf160752d48280071 (base 3960316e0df4a384376d0ebe5d07ee9eca8e25ca). All three concern ordinary pre-upgrade HD accounts and recovery guidance.
| Finding | Severity | Result |
|---|---|---|
| CR-001 | P2 | The short TRON export/import/delete sequence removes the original EVM slot without including its restoration step. The surviving seed permits recovery. |
| CR-004 | P3 | Native backup overstates a limitation of this version's default recovery flow as a limitation of the mnemonic in every wallet. |
| CR-005 | P3 | Reselecting an existing legacy slot succeeds, but its text receipt omits the default-recovery limitation and still gives unconditional backup guidance. |
Suggested order: make the short migration procedure preserve both original family identities, then align the native warning and derive receipt with the actual recovery behavior. The ability to recover keys from the mnemonic plus the original path should remain explicit.
Validation: static review of the current PR diff and relevant call chains, including the unchanged deletion, private-key import, and receipt code. The latest derive-selector and interactive backup changes were checked for their effect on these findings. No full test suite, build, or end-to-end wallet/cryptographic tests were run for this review.
Three inline comments provide the triggers, impact boundaries, and suggested regression coverage.
| ` $ wallet-cli backup ${selector} --keystore --network tron:728126428 --password-stdin\n` + | ||
| ` $ wallet-cli import keystore <file>\n` + | ||
| ` $ wallet-cli delete ${selector} --yes\n` + |
There was a problem hiding this comment.
[P2] Preserve the original EVM account in the short recovery flow (CR-001)
For a normal pre-upgrade HD slot at index 1, these commands export only its TRON child key (m/44'/195'/1'/0/0) and re-import that key as a standalone account. The EVM address generated from this imported key is different from the original EVM address derived at m/44'/60'/0'/0/1. Keystore.delete() then removes the entire index entry, including both families, so following just these three commands makes the original EVM account disappear from the local account list.
The root account and seed survive this non-root deletion, so the EVM account can be recovered by deriving the original index again once the legacy-slot gate permits it. This is an incomplete recovery procedure, not permanent key loss. The full recovery guide already includes rebuilding the EVM indexes, but the actionable short sequence here omits that step.
Please include EVM preservation/restoration and its required ordering in this message, or explicitly require the complete recovery procedure before deleting the slot. A regression should verify that recovery retains access to both original family addresses, rather than checking only the imported TRON address.
There was a problem hiding this comment.
Fixed in a681c7d. The shared legacy error no longer inlines the TRON-only export/import/delete sequence. It keeps the affected label and verified path, then directs the user to the complete versioned recovery guide and explicitly says to follow it before deleting anything. The guide preserves the legacy TRON keys first, then rebuilds the mnemonic wallet and restores every original EVM index. Updated the domain, signer, and keystore assertions. Full suite: 193 files, 2368 passed, 2 skipped.
| `this recovery phrase does NOT back up ${stranded | ||
| .map((a) => `${wallet.id}.${a.index} (${a.path})`) | ||
| .join(", ")} — ` + | ||
| `${stranded.length === 1 ? "that account was" : "those accounts were"} derived at a TRON path this version no longer produces, ` + | ||
| `and the phrase re-derives the current one in every wallet, this one included. ` + |
There was a problem hiding this comment.
[P3] Limit the backup warning to this version's default recovery behavior (CR-004)
When a seed wallet contains a legitimate legacy TRON child, native backup says the phrase "does NOT back up" that account and will select the current path "in every wallet". The same mnemonic with the original path still derives the original private key: resolveDerivation() in this PR does exactly that, and the repository's Java MnemonicUtils.getPrivateKeyFromMnemonicByCustomPath() also supports the old account-level path.
The limitation is that this TypeScript version's default mnemonic import/derive flow no longer rediscovers those legacy addresses. The broader wording can mislead users into believing their saved mnemonic has lost its recovery value.
Please state the default-recovery limitation explicitly while keeping the affected indexes, paths, and separate-keystore advice. A warning assertion should check this distinction; neither key loss nor inability to recover in every wallet follows from the path change.
There was a problem hiding this comment.
Fixed in a681c7d. The native-backup warning now says that this version's default mnemonic recovery will not recreate the listed legacy TRON accounts; it also states explicitly that the recovery phrase can still derive those keys at the listed paths. The warning keeps the separate-keystore action and now says to do it before deleting anything, with a link to the full recovery guide. Documentation, comments, and warning assertions were updated to avoid the broader key-loss claim.
| return { | ||
| status: mutationStatus(result.created), | ||
| ...this.#describeWithVerifiedDerivation(result.accountId), | ||
| }; |
There was a problem hiding this comment.
[P3] Explain the recovery limitation when reselecting a legacy slot (CR-005)
After upgrading while retaining the old wallet directory, derive --seed-id <id> --index 1 succeeds when index 1 already exists: addAccount() verifies the legacy address and reactivates the slot without deriving a new key. This result includes the verified old path, but the command emits no legacy warning and the text formatter still unconditionally prints shares master mnemonic; no separate backup needed without displaying that path.
If the user later restores the mnemonic into a fresh directory and derives the same index with this version's defaults, the TRON address differs. The mnemonic plus the old path can still recover the key; the missing information is that default recovery will not automatically recreate this account.
Please carry the legacy recovery limitation into the successful text output or a shared warning, while preserving the intended ability to select existing slots. JSON already contains the verified path; the unconditional backup note is specific to text output. Cover legacy-existing, current-existing, and current-created receipts. The pre-existing Derived sub-account title is not the issue being reported here.
The downstream formatter is unchanged by this PR: walletDerive, line 58. This comment is anchored at the changed result construction that now supplies the verified path.
There was a problem hiding this comment.
Fixed in a681c7d. Reselecting an existing legacy slot still succeeds, but WalletService now detects its verified legacy TRON path and emits a recovery warning. JSON carries it in meta.warnings, while text output prints it before the receipt. The unconditional no-separate-backup note is now the neutral shares-the-wallet-recovery-phrase note. Coverage distinguishes current-created, current-existing with no warning, and legacy-existing with a warning, including a black-box CLI assertion for meta.warnings.
3 participants
No description provided.