fix: address review feedback

This commit is contained in:
Moudy 2026-05-07 22:48:32 +02:00
parent 69b81ea621
commit 4ace6e1570
12 changed files with 151 additions and 212 deletions

View File

@ -44,10 +44,6 @@ async fn new_private_account(ctx: &mut TestContext) -> Result<nssa::AccountId> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),

View File

@ -160,10 +160,6 @@ async fn private_transfer_to_owned_account_using_claiming_path() -> Result<()> {
// Create a new private account
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
}));
@ -332,10 +328,6 @@ async fn private_transfer_to_owned_account_continuous_run_path() -> Result<()> {
// Create a new private account
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
}));
@ -401,10 +393,6 @@ async fn initialize_private_account() -> Result<()> {
let mut ctx = TestContext::new().await?;
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
}));
@ -505,10 +493,6 @@ async fn initialize_private_account_using_label() -> Result<()> {
// Create a new private account with a label
let label = "init-private-label".to_owned();
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: Some(label.clone()),
}));

View File

@ -30,10 +30,6 @@ async fn sync_private_account_with_non_zero_chain_index() -> Result<()> {
// Create a new private account
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
}));
@ -44,10 +40,6 @@ async fn sync_private_account_with_non_zero_chain_index() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -127,10 +119,6 @@ async fn restore_keys_from_seed() -> Result<()> {
// Create first private account at root
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: Some(ChainIndex::root()),
label: None,
}));
@ -144,10 +132,6 @@ async fn restore_keys_from_seed() -> Result<()> {
// Create second private account at /0
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: Some(ChainIndex::from_str("/0")?),
label: None,
}));

View File

@ -85,10 +85,6 @@ async fn claim_pinata_to_uninitialized_private_account_fails_fast() -> Result<()
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -233,10 +229,6 @@ async fn claim_pinata_to_new_private_account() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),

View File

@ -45,10 +45,9 @@ async fn group_create_and_shared_account_registration() -> Result<()> {
);
// Create a shared regular private account from the group
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
cci: None,
let command = Command::Account(AccountSubcommand::New(NewSubcommand::PrivateGms {
group: "test-group".to_string(),
label: Some("shared-acc".to_string()),
for_gms: Some("test-group".to_string()),
pda: false,
seed: None,
program_id: None,
@ -63,9 +62,11 @@ async fn group_create_and_shared_account_registration() -> Result<()> {
};
// Verify shared account is registered in storage
let shared_private_accounts = &ctx.wallet().storage().user_data.shared_private_accounts;
let entry = shared_private_accounts
.get(&shared_account_id)
let entry = ctx
.wallet()
.storage()
.user_data
.shared_private_account(&shared_account_id)
.context("Shared account not found in storage")?;
assert_eq!(entry.group_label, "test-group");
assert!(entry.pda_seed.is_none());
@ -143,10 +144,9 @@ async fn fund_shared_account_from_public() -> Result<()> {
});
wallet::cli::execute_subcommand(ctx.wallet_mut(), command).await?;
let command = Command::Account(AccountSubcommand::New(NewSubcommand::Private {
cci: None,
let command = Command::Account(AccountSubcommand::New(NewSubcommand::PrivateGms {
group: "fund-group".to_string(),
label: None,
for_gms: Some("fund-group".to_string()),
pda: false,
seed: None,
program_id: None,
@ -193,8 +193,7 @@ async fn fund_shared_account_from_public() -> Result<()> {
.wallet()
.storage()
.user_data
.shared_private_accounts
.get(&shared_id)
.shared_private_account(&shared_id)
.context("Shared account not found after sync")?;
info!(

View File

@ -297,10 +297,6 @@ async fn create_and_transfer_token_with_private_supply() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -317,10 +313,6 @@ async fn create_and_transfer_token_with_private_supply() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -468,10 +460,6 @@ async fn create_token_with_private_definition() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: Some(ChainIndex::root()),
label: None,
})),
@ -544,10 +532,6 @@ async fn create_token_with_private_definition() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -678,10 +662,6 @@ async fn create_token_with_private_definition_and_supply() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -698,10 +678,6 @@ async fn create_token_with_private_definition_and_supply() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -764,10 +740,6 @@ async fn create_token_with_private_definition_and_supply() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -883,10 +855,6 @@ async fn shielded_token_transfer() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -998,10 +966,6 @@ async fn deshielded_token_transfer() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -1113,10 +1077,6 @@ async fn token_claiming_path_with_private_accounts() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -1133,10 +1093,6 @@ async fn token_claiming_path_with_private_accounts() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),
@ -1170,10 +1126,6 @@ async fn token_claiming_path_with_private_accounts() -> Result<()> {
let result = wallet::cli::execute_subcommand(
ctx.wallet_mut(),
Command::Account(AccountSubcommand::New(NewSubcommand::Private {
for_gms: None,
pda: false,
seed: None,
program_id: None,
cci: None,
label: None,
})),

View File

@ -36,7 +36,7 @@ pub struct SharedAccountEntry {
pub account: Account,
}
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug)]
pub struct NSSAUserData {
/// Default public accounts.
pub default_pub_account_signing_keys: BTreeMap<nssa::AccountId, nssa::PrivateKey>,
@ -46,17 +46,11 @@ pub struct NSSAUserData {
pub public_key_tree: KeyTreePublic,
/// Tree of private keys.
pub private_key_tree: KeyTreePrivate,
/// Group key holders for private PDA groups, keyed by a human-readable label.
/// Defaults to empty for backward compatibility with wallets that predate group PDAs.
/// An older wallet binary that re-serializes this struct will drop the field.
#[serde(default)]
/// Group key holders for shared account management, keyed by a human-readable label.
pub group_key_holders: BTreeMap<String, GroupKeyHolder>,
/// Cached plaintext state of shared accounts (PDAs and regular shared accounts),
/// Cached plaintext state of shared private accounts (PDAs and regular shared accounts),
/// keyed by `AccountId`. Each entry stores the group label and identifier needed
/// to re-derive keys during sync.
/// Old wallet files with `pda_accounts` (plain Account values) are incompatible with
/// this type. The `default` attribute ensures they deserialize as empty rather than failing.
#[serde(default)]
pub shared_private_accounts: BTreeMap<nssa::AccountId, SharedAccountEntry>,
}
@ -239,6 +233,42 @@ impl NSSAUserData {
pub fn insert_group_key_holder(&mut self, label: String, holder: GroupKeyHolder) {
self.group_key_holders.insert(label, holder);
}
/// Returns the cached account for a shared private account, if it exists.
#[must_use]
pub fn shared_private_account(
&self,
account_id: &nssa::AccountId,
) -> Option<&SharedAccountEntry> {
self.shared_private_accounts.get(account_id)
}
/// Inserts or replaces a shared private account entry.
pub fn insert_shared_private_account(
&mut self,
account_id: nssa::AccountId,
entry: SharedAccountEntry,
) {
self.shared_private_accounts.insert(account_id, entry);
}
/// Updates the cached account state for a shared private account.
pub fn update_shared_private_account_state(
&mut self,
account_id: &nssa::AccountId,
account: nssa_core::account::Account,
) {
if let Some(entry) = self.shared_private_accounts.get_mut(account_id) {
entry.account = account;
}
}
/// Iterates over all shared private accounts.
pub fn shared_private_accounts_iter(
&self,
) -> impl Iterator<Item = (&nssa::AccountId, &SharedAccountEntry)> {
self.shared_private_accounts.iter()
}
}
impl Default for NSSAUserData {

View File

@ -420,7 +420,7 @@ mod tests {
/// PDA init: initializes a new PDA under `authenticated_transfer`'s ownership.
/// The `private_pda_spender` program chains to `authenticated_transfer` with `pda_seeds`
/// to establish authorization and the mask-3 binding.
/// to establish authorization and the private PDA binding.
#[test]
fn private_pda_init() {
let program = Program::private_pda_spender();
@ -430,7 +430,7 @@ mod tests {
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
// PDA (new, mask 3) — AccountId derived from private_pda_spender's program ID
// PDA (new, private PDA) — AccountId derived from private_pda_spender's program ID
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
@ -467,11 +467,11 @@ mod tests {
let seed = PdaSeed::new([42; 32]);
let shared_secret_pda = SharedSecretKey::new(&[55; 32], &keys.vpk());
// PDA (new, mask 3)
// PDA (new, private PDA)
let pda_id = AccountId::for_private_pda(&program.id(), &seed, &npk);
let pda_pre = AccountWithMetadata::new(Account::default(), false, pda_id);
// Recipient (mask 0, public)
// Recipient (public)
let recipient_id = AccountId::new([88; 32]);
let recipient_pre = AccountWithMetadata::new(
Account {
@ -510,7 +510,7 @@ mod tests {
/// Shared regular private account: receives funds via `authenticated_transfer` directly,
/// no custom program needed. This demonstrates the non-PDA shared account flow where
/// keys are derived from GMS via `derive_keys_for_shared_account`. The shared account
/// uses standard mask 2 (new unauthorized private) and works with auth-transfer's
/// uses the standard unauthorized private account path and works with auth-transfer's
/// transfer path like any other private account.
#[test]
fn shared_account_receives_via_auth_transfer() {
@ -532,7 +532,7 @@ mod tests {
sender_id,
);
// Recipient: shared private account (new, unauthorized, mask 2)
// Recipient: shared private account (new, unauthorized)
let shared_account_id = AccountId::from((&shared_npk, shared_identifier));
let recipient = AccountWithMetadata::new(Account::default(), false, shared_account_id);

View File

@ -53,7 +53,7 @@ fn main() {
// Chain to authenticated_transfer with pda_seeds to authorize the PDA.
// The circuit's resolve_authorization_and_record_bindings establishes the
// mask-3 (seed, npk) binding when pda_seeds match the private PDA derivation.
// private PDA (seed, npk) binding when pda_seeds match the private PDA derivation.
let mut auth_pda_pre = pda_pre;
auth_pda_pre.is_authorized = true;
let auth_call =

View File

@ -83,19 +83,23 @@ pub enum NewSubcommand {
label: Option<String>,
},
/// Single-account convenience: creates a key node and auto-registers one account with a random
/// identifier. When `--for-gms` is provided, derives keys from the named group instead of
/// the wallet's key tree.
/// identifier.
Private {
#[arg(long)]
/// Chain index of a parent node (ignored when --for-gms is used).
/// Chain index of a parent node.
cci: Option<ChainIndex>,
#[arg(short, long)]
/// Label to assign to the new account.
label: Option<String>,
},
/// Create a shared private account from a group's GMS.
PrivateGms {
/// Group name to derive keys from.
group: String,
#[arg(short, long)]
/// Label to assign to the new account.
label: Option<String>,
#[arg(long)]
/// Derive keys from a group's GMS instead of the wallet tree.
for_gms: Option<String>,
#[arg(long, requires = "for_gms")]
/// Create a PDA account (requires --seed and --program-id).
pda: bool,
#[arg(long, requires = "pda")]
@ -157,10 +161,50 @@ impl WalletSubcommand for NewSubcommand {
Ok(SubcommandReturnValue::RegisterAccount { account_id })
}
Self::Private {
cci,
Self::Private { cci, label } => {
if let Some(label) = &label
&& wallet_core
.storage
.labels
.values()
.any(|l| l.to_string() == *label)
{
anyhow::bail!("Label '{label}' is already in use by another account");
}
let (account_id, chain_index) = wallet_core.create_new_account_private(cci);
let node = wallet_core
.storage
.user_data
.private_key_tree
.key_map
.get(&chain_index)
.expect("Node was just inserted");
let key = &node.value.0;
if let Some(label) = label {
wallet_core
.storage
.labels
.insert(account_id.to_string(), Label::new(label));
}
println!(
"Generated new account with account_id Private/{account_id} at path {chain_index}"
);
println!("With npk {}", hex::encode(key.nullifier_public_key.0));
println!(
"With vpk {}",
hex::encode(key.viewing_public_key.to_bytes())
);
wallet_core.store_persistent_data().await?;
Ok(SubcommandReturnValue::RegisterAccount { account_id })
}
Self::PrivateGms {
group,
label,
for_gms,
pda,
seed,
program_id,
@ -175,80 +219,47 @@ impl WalletSubcommand for NewSubcommand {
anyhow::bail!("Label '{label}' is already in use by another account");
}
if let Some(group_name) = for_gms {
let info = if pda {
let seed_hex = seed.context("--seed is required for PDA accounts")?;
let pid_hex =
program_id.context("--program-id is required for PDA accounts")?;
let info = if pda {
let seed_hex = seed.context("--seed is required for PDA accounts")?;
let pid_hex =
program_id.context("--program-id is required for PDA accounts")?;
let seed_bytes: [u8; 32] = hex::decode(&seed_hex)
.context("Invalid seed hex")?
.try_into()
.map_err(|_err| anyhow::anyhow!("Seed must be exactly 32 bytes"))?;
let pda_seed = nssa_core::program::PdaSeed::new(seed_bytes);
let seed_bytes: [u8; 32] = hex::decode(&seed_hex)
.context("Invalid seed hex")?
.try_into()
.map_err(|_err| anyhow::anyhow!("Seed must be exactly 32 bytes"))?;
let pda_seed = nssa_core::program::PdaSeed::new(seed_bytes);
let pid_bytes = hex::decode(&pid_hex).context("Invalid program ID hex")?;
if pid_bytes.len() != 32 {
anyhow::bail!("Program ID must be exactly 32 bytes");
}
let mut pid: nssa_core::program::ProgramId = [0; 8];
for (i, chunk) in pid_bytes.chunks_exact(4).enumerate() {
pid[i] = u32::from_le_bytes(chunk.try_into().unwrap());
}
wallet_core.create_shared_pda_account(&group_name, pda_seed, pid)?
} else {
wallet_core.create_shared_regular_account(&group_name)?
};
if let Some(label) = label {
wallet_core
.storage
.labels
.insert(info.account_id.to_string(), Label::new(label));
let pid_bytes = hex::decode(&pid_hex).context("Invalid program ID hex")?;
if pid_bytes.len() != 32 {
anyhow::bail!("Program ID must be exactly 32 bytes");
}
let mut pid: nssa_core::program::ProgramId = [0; 8];
for (i, chunk) in pid_bytes.chunks_exact(4).enumerate() {
pid[i] = u32::from_le_bytes(chunk.try_into().unwrap());
}
println!("Shared account from group '{group_name}'");
println!("AccountId: Private/{}", info.account_id);
println!("NPK: {}", hex::encode(info.npk.0));
println!("VPK: {}", hex::encode(&info.vpk.0));
wallet_core.store_persistent_data().await?;
Ok(SubcommandReturnValue::RegisterAccount {
account_id: info.account_id,
})
wallet_core.create_shared_pda_account(&group, pda_seed, pid)?
} else {
// Standard wallet-tree-derived account
let (account_id, chain_index) = wallet_core.create_new_account_private(cci);
wallet_core.create_shared_regular_account(&group)?
};
let node = wallet_core
if let Some(label) = label {
wallet_core
.storage
.user_data
.private_key_tree
.key_map
.get(&chain_index)
.expect("Node was just inserted");
let key = &node.value.0;
if let Some(label) = label {
wallet_core
.storage
.labels
.insert(account_id.to_string(), Label::new(label));
}
println!(
"Generated new account with account_id Private/{account_id} at path {chain_index}"
);
println!("With npk {}", hex::encode(key.nullifier_public_key.0));
println!(
"With vpk {}",
hex::encode(key.viewing_public_key.to_bytes())
);
wallet_core.store_persistent_data().await?;
Ok(SubcommandReturnValue::RegisterAccount { account_id })
.labels
.insert(info.account_id.to_string(), Label::new(label));
}
println!("Shared account from group '{group}'");
println!("AccountId: Private/{}", info.account_id);
println!("NPK: {}", hex::encode(info.npk.0));
println!("VPK: {}", hex::encode(&info.vpk.0));
wallet_core.store_persistent_data().await?;
Ok(SubcommandReturnValue::RegisterAccount {
account_id: info.account_id,
})
}
Self::PrivateAccountsKey { cci } => {
let chain_index = wallet_core.create_private_accounts_key(cci);

View File

@ -328,7 +328,7 @@ impl WalletCore {
pda_program_id: Option<nssa_core::program::ProgramId>,
) {
use key_protocol::key_protocol_core::SharedAccountEntry;
self.storage.user_data.shared_private_accounts.insert(
self.storage.user_data.insert_shared_private_account(
account_id,
SharedAccountEntry {
group_label,
@ -685,14 +685,12 @@ impl WalletCore {
let shared_keys: Vec<_> = self
.storage
.user_data
.shared_private_accounts
.iter()
.shared_private_accounts_iter()
.filter_map(|(&account_id, entry)| {
let holder = self
.storage
.user_data
.group_key_holders
.get(&entry.group_label)?;
.group_key_holder(&entry.group_label)?;
let keys = match (&entry.pda_seed, &entry.pda_program_id) {
(Some(pda_seed), Some(program_id)) => {
@ -743,14 +741,9 @@ impl WalletCore {
.expect("Ciphertext ID is expected to fit in u32"),
) {
info!("Synced shared account {account_id:#?} with new state {new_acc:#?}");
if let Some(entry) = self
.storage
self.storage
.user_data
.shared_private_accounts
.get_mut(&account_id)
{
entry.account = new_acc;
}
.update_shared_private_account_state(&account_id, new_acc);
}
}
}

View File

@ -31,8 +31,8 @@ pub enum PrivacyPreservingAccount {
seed: PdaSeed,
},
/// A shared regular private account with externally-provided keys (e.g. from GMS).
/// Uses standard `AccountId = from((&npk, identifier))` and mask 1/2.
/// Works with `authenticated_transfer` and all existing programs out of the box.
/// Uses standard `AccountId = from((&npk, identifier))` with authorized/unauthorized private
/// paths. Works with `authenticated_transfer` and all existing programs out of the box.
PrivateShared {
nsk: NullifierSecretKey,
npk: NullifierPublicKey,
@ -335,8 +335,7 @@ async fn private_pda_preparation(
let acc = wallet
.storage
.user_data
.shared_private_accounts
.get(&account_id)
.shared_private_account(&account_id)
.map(|e| e.account.clone())
.unwrap_or_default();
@ -386,8 +385,7 @@ async fn private_shared_preparation(
let acc = wallet
.storage
.user_data
.shared_private_accounts
.get(&account_id)
.shared_private_account(&account_id)
.map(|e| e.account.clone())
.unwrap_or_default();