Skip to content

Vaults#747

Open
Rigidity wants to merge 10 commits intomainfrom
vaults
Open

Vaults#747
Rigidity wants to merge 10 commits intomainfrom
vaults

Conversation

@Rigidity
Copy link
Collaborator

@Rigidity Rigidity commented Feb 19, 2026

Note

High Risk
Touches core wallet identity/storage, database schema, and coin/puzzle handling while adding new wallet types (vault/watch), increasing the chance of migration issues or subtle spend/sync regressions. Vault spending paths are partially unimplemented (todo!) and new selection restrictions may surface runtime errors in edge cases.

Overview
Adds multi-wallet types across the stack by replacing key-centric APIs with wallet-centric ones and introducing WalletRecord/WalletKind (BLS, Vault, Watch) in sage-api, updating OpenAPI/tauri bindings, and renaming endpoints (import_wallet, import_addresses, get_wallet(s), delete_wallet, rename_wallet).

Introduces initial vault and watch-only plumbing: database migration 0006_vaults.sql adds vault-related tables, the DB layer gains Vault asset/coin/p2-puzzle kinds plus "external" puzzles, the keychain can now store vault launcher IDs and watch puzzle hashes, and wallet sync/derivation logic is gated so only BLS wallets auto-derive addresses.

Frontend updates the login flow and wallet switcher to handle the new wallet types, adds screens for Watch Address import, a Keys management page, and stubbed Mint Vault/Recover Vault wizards; also refactors repeated duration and mnemonic UI into reusable DurationInput and MnemonicDisplay components.

Written by Cursor Bugbot for commit 021c349. This will update automatically on new commits. Configure here.

@Rigidity
Copy link
Collaborator Author

bugbot run

@Rigidity Rigidity marked this pull request as ready for review February 25, 2026 03:56
@Rigidity Rigidity marked this pull request as draft February 25, 2026 03:56
@Rigidity

This comment was marked as outdated.

@Rigidity

This comment was marked as outdated.

@Rigidity

This comment was marked as outdated.

@Rigidity Rigidity marked this pull request as ready for review February 25, 2026 21:00
@Rigidity
Copy link
Collaborator Author

@cursor review

let fingerprint = u32::from_be_bytes(self.rng.r#gen::<[u8; 4]>());

if self.contains(fingerprint) {
return Err(KeychainError::KeyExists);
Copy link

Choose a reason for hiding this comment

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

Random fingerprint collision returns error instead of retrying

Low Severity

add_watch_p2_puzzle_hashes generates a random u32 fingerprint and returns KeychainError::KeyExists if it collides with an existing fingerprint. Since the fingerprint is random and the collision check doesn't retry, users could get an unexplained "Key already exists" error when importing a watch-only wallet, even though the addresses are unique. The add_vault method has the same issue deriving a fingerprint from launcher_id bytes.

Fix in Cursor Fix in Web

}
}
} else {
info!("Wallet is a vault, skipping automatic key derivation");
Copy link

Choose a reason for hiding this comment

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

Log message says "vault" for watch wallets too

Low Severity

The else branch on the matches!(wallet.info, WalletInfo::Bls { .. }) check covers both Vault and Watch wallet types, but the log message says "Wallet is a vault." Watch wallets would produce a misleading log entry, making debugging harder.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.


const handleCreate = () => {
toast.success(t`Vault creation is not yet implemented.`);
};
Copy link

Choose a reason for hiding this comment

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

Success toast used for unimplemented vault features

Medium Severity

toast.success is used to display "not yet implemented" messages in both MintVault and RecoverVault. After completing a multi-step wizard (generating and potentially saving mnemonics), users see a green success notification saying the feature doesn't work. This is actively misleading — toast.info or toast.warning would be appropriate for stub functionality.

Additional Locations (1)

Fix in Cursor Fix in Web

</h3>
<p className='break-all text-sm text-muted-foreground'>
{info.public_key}
{info.type === 'bls' && info.public_key}
Copy link

Choose a reason for hiding this comment

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

Details dialog shows empty Public Key for non-BLS

Low Severity

The "Wallet Details" dialog unconditionally renders the "Public Key" heading for all wallet types. For vault and watch wallets, info.type === 'bls' && info.public_key evaluates to false, leaving the heading visible with no content below it. The entire block needs to be conditional on info.type === 'bls'.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant