From 1b353b2e215f651f8300421c426e366847981608 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Tue, 7 May 2024 16:39:34 +0200 Subject: [PATCH 1/5] NNS1-2905: Remove transactions fields from accounts --- rs/backend/src/accounts_store.rs | 82 +++++++++++++++++-- rs/backend/src/accounts_store/schema/tests.rs | 1 - rs/backend/src/accounts_store/tests.rs | 71 +++++++++++++++- rs/backend/src/accounts_store/toy_data.rs | 34 +------- 4 files changed, 147 insertions(+), 41 deletions(-) diff --git a/rs/backend/src/accounts_store.rs b/rs/backend/src/accounts_store.rs index 63d3a425475..f567eb3aee5 100644 --- a/rs/backend/src/accounts_store.rs +++ b/rs/backend/src/accounts_store.rs @@ -153,36 +153,111 @@ pub struct Account { /// Note: The principal was not stored for early users. When early users log in, we discover their principal and set this field. principal: Option, account_identifier: AccountIdentifier, - default_account_transactions: Vec, sub_accounts: HashMap, hardware_wallet_accounts: Vec, canisters: Vec, + // default_account_transactions: Do not reuse this field. There are still accounts in stable memor with this unused field. } impl Storable for Account { const BOUND: Bound = Bound::Unbounded; fn to_bytes(&self) -> Cow<'_, [u8]> { - candid::encode_one(self).expect("Failed to serialize account").into() + // Encode Account in a way that can still be restored if we need to + // roll back the release. + let old_account = OldAccount::from(self.clone()); + candid::encode_one(old_account) + .expect("Failed to serialize account") + .into() + // TODO(NNS1-2905): Restore direct encoding once OldAccount is deleted. + // candid::encode_one(self).expect("Failed to serialize account").into() } fn from_bytes(bytes: Cow<'_, [u8]>) -> Self { candid::decode_one(&bytes).expect("Failed to parse account from store.") } } +// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release. +#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)] +struct OldAccount { + principal: Option, + account_identifier: AccountIdentifier, + default_account_transactions: Vec, + sub_accounts: HashMap, + hardware_wallet_accounts: Vec, + canisters: Vec, +} + +impl From for OldAccount { + fn from(account: Account) -> Self { + OldAccount { + principal: account.principal, + account_identifier: account.account_identifier, + default_account_transactions: Vec::new(), + sub_accounts: account + .sub_accounts + .into_iter() + .map(|(id, sa)| (id, sa.into())) + .collect(), + hardware_wallet_accounts: account + .hardware_wallet_accounts + .into_iter() + .map(NamedHardwareWalletAccount::into) + .collect(), + canisters: account.canisters, + } + } +} + #[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)] struct NamedSubAccount { + name: String, + account_identifier: AccountIdentifier, + // transactions: Do not reuse this field. There are still accounts in stable memor with this unused field. +} + +// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release. +#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)] +struct OldNamedSubAccount { name: String, account_identifier: AccountIdentifier, transactions: Vec, } +impl From for OldNamedSubAccount { + fn from(sub_account: NamedSubAccount) -> Self { + OldNamedSubAccount { + name: sub_account.name, + account_identifier: sub_account.account_identifier, + transactions: Vec::new(), + } + } +} + #[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)] struct NamedHardwareWalletAccount { + name: String, + principal: PrincipalId, + // transactions: Do not reuse this field. There are still accounts in stable memor with this unused field. +} + +// TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release. +#[derive(CandidType, Deserialize, Debug, Eq, PartialEq, Clone)] +struct OldNamedHardwareWalletAccount { name: String, principal: PrincipalId, transactions: Vec, } +impl From for OldNamedHardwareWalletAccount { + fn from(hw_account: NamedHardwareWalletAccount) -> Self { + OldNamedHardwareWalletAccount { + name: hw_account.name, + principal: hw_account.principal, + transactions: Vec::new(), + } + } +} + #[derive(CandidType, Deserialize, Clone, Debug, PartialEq, Eq)] pub struct NamedCanister { name: String, @@ -544,7 +619,6 @@ impl AccountsStore { account.hardware_wallet_accounts.push(NamedHardwareWalletAccount { name: request.name, principal: request.principal, - transactions: Vec::new(), }); account .hardware_wallet_accounts @@ -1204,7 +1278,6 @@ impl Account { Account { principal: Some(principal), account_identifier, - default_account_transactions: Vec::new(), sub_accounts: HashMap::new(), hardware_wallet_accounts: Vec::new(), canisters: Vec::new(), @@ -1217,7 +1290,6 @@ impl NamedSubAccount { NamedSubAccount { name, account_identifier, - transactions: Vec::new(), } } } diff --git a/rs/backend/src/accounts_store/schema/tests.rs b/rs/backend/src/accounts_store/schema/tests.rs index c94bcf13f62..13de7db6dd6 100644 --- a/rs/backend/src/accounts_store/schema/tests.rs +++ b/rs/backend/src/accounts_store/schema/tests.rs @@ -21,7 +21,6 @@ pub fn toy_account(account_index: u64, num_canisters: u64) -> Account { let mut account = Account { principal: Some(principal), account_identifier, - default_account_transactions: Vec::new(), sub_accounts: HashMap::new(), hardware_wallet_accounts: Vec::new(), canisters: Vec::new(), diff --git a/rs/backend/src/accounts_store/tests.rs b/rs/backend/src/accounts_store/tests.rs index 98426c30e9b..e9fe6cf766c 100644 --- a/rs/backend/src/accounts_store/tests.rs +++ b/rs/backend/src/accounts_store/tests.rs @@ -1067,8 +1067,6 @@ fn accounts_should_implement_storable() { ToyAccountSize { sub_accounts: 2, canisters: 3, - default_account_transactions: 4, - sub_account_transactions: 5, hardware_wallets: 6, }, ); @@ -1077,4 +1075,73 @@ fn accounts_should_implement_storable() { assert_eq!(account, parsed); } +fn create_new_test_account_for_encoding() -> Account { + let principal = PrincipalId::from_str(TEST_ACCOUNT_1).unwrap(); + let account_identifier = AccountIdentifier::from(principal); + + let sub_principal = PrincipalId::from_str(TEST_ACCOUNT_2).unwrap(); + let sub_account_identifier = AccountIdentifier::from(sub_principal); + + let hw_principal = PrincipalId::from_str(TEST_ACCOUNT_3).unwrap(); + + let new_sub_account = NamedSubAccount { + name: "Sub1".to_string(), + account_identifier: sub_account_identifier, + }; + + let mut new_sub_accounts = HashMap::new(); + new_sub_accounts.insert(1, new_sub_account); + + let new_hw_account = NamedHardwareWalletAccount { + name: "HW1".to_string(), + principal: hw_principal, + }; + + Account { + principal: Some(principal), + account_identifier, + sub_accounts: new_sub_accounts, + hardware_wallet_accounts: vec![new_hw_account], + canisters: vec![], + } +} + +#[test] +fn old_account_can_be_decoded_to_new_account() { + // Test that after upgrading to the next release, we are able to decode the + // old accounts from stable memory. + let expected_new_account = create_new_test_account_for_encoding(); + let old_account = OldAccount::from(expected_new_account.clone()); + + let bytes = Candid((old_account,)).into_bytes().unwrap(); + let decoded_new_account = Account::from_bytes(Cow::Owned(bytes)); + + assert_eq!(decoded_new_account, expected_new_account); +} + +#[test] +fn new_account_can_be_decoded_to_new_account() { + // Test that after upgrading to the next release after the next release, we + // are able to decode the new accounts that were encoded as old accounts. + let expected_new_account = create_new_test_account_for_encoding(); + + let bytes = expected_new_account.to_bytes(); + let decoded_new_account = Account::from_bytes(bytes); + + assert_eq!(decoded_new_account, expected_new_account); +} + +#[test] +fn new_account_can_be_decoded_to_old_account() { + // Test that, in case we need to roll back after the next release, the + // previous version is able to decode accounts written by the next version. + let new_account = create_new_test_account_for_encoding(); + let expected_old_account = OldAccount::from(new_account.clone()); + + let bytes = new_account.to_bytes(); + let (decoded_old_account,): (OldAccount,) = Candid::from_bytes(bytes.into_owned()).map(|c| c.0).unwrap(); + + assert_eq!(decoded_old_account, expected_old_account); +} + crate::accounts_store::schema::tests::test_accounts_db!(AccountsStore::default()); diff --git a/rs/backend/src/accounts_store/toy_data.rs b/rs/backend/src/accounts_store/toy_data.rs index 5dc0523233c..43f62d2ddb9 100644 --- a/rs/backend/src/accounts_store/toy_data.rs +++ b/rs/backend/src/accounts_store/toy_data.rs @@ -25,11 +25,6 @@ pub struct ToyAccountSize { pub sub_accounts: usize, /// The number of canisters pub canisters: usize, - /// The number of transaction indices for the main account; transactions are stored outside the - /// account. - pub default_account_transactions: usize, - /// The number of transactions per sub-account. - pub sub_account_transactions: usize, /// The number of hardware wallets. pub hardware_wallets: usize, } @@ -38,25 +33,10 @@ impl From<&Account> for ToyAccountSize { fn from(account: &Account) -> Self { let sub_accounts = account.sub_accounts.len(); let canisters = account.canisters.len(); - let default_account_transactions = account.default_account_transactions.len(); - // Average number of transactions per sub-account, rounded down. When creating a toy - // account, all sub-accounts have the same number of transactions however that can change. - let total_sub_account_transactions: usize = account - .sub_accounts - .values() - .map(|sub_account| sub_account.transactions.len()) - .sum(); - let sub_account_transactions: usize = if sub_accounts == 0 { - 0 - } else { - total_sub_account_transactions / sub_accounts - }; let hardware_wallets = account.hardware_wallet_accounts.len(); ToyAccountSize { sub_accounts, canisters, - default_account_transactions, - sub_account_transactions, hardware_wallets, } } @@ -76,7 +56,6 @@ pub fn toy_account(account_index: u64, size: ToyAccountSize) -> Account { let mut account = Account { principal: Some(principal), account_identifier, - default_account_transactions: Vec::new(), sub_accounts: HashMap::new(), hardware_wallet_accounts: Vec::new(), canisters: Vec::new(), @@ -87,8 +66,7 @@ pub fn toy_account(account_index: u64, size: ToyAccountSize) -> Account { let sub_account_name = format!("sub_account_{account_index}_{sub_account_index}"); let sub_account = convert_byte_to_sub_account(sub_account_index); let sub_account_identifier = AccountIdentifier::new(principal, Some(sub_account)); - let mut named_sub_account = NamedSubAccount::new(sub_account_name.clone(), sub_account_identifier); - named_sub_account.transactions = (0..size.sub_account_transactions as u64).collect(); + let named_sub_account = NamedSubAccount::new(sub_account_name.clone(), sub_account_identifier); account.sub_accounts.insert(sub_account_index, named_sub_account); } // Attaches canisters to the account. @@ -100,13 +78,6 @@ pub fn toy_account(account_index: u64, size: ToyAccountSize) -> Account { }; account.canisters.push(canister); } - for transaction_index in 0..size.default_account_transactions as u64 { - // Note: Normally a transaction would be added to the list of transactions in the accounts - // store and the index of that transaction would be stored in the account itself. Given - // that we are creating a standalong account, without an account store, the index is - // meaningless. - account.default_account_transactions.push(transaction_index); - } for hardware_wallet_index in 0..size.hardware_wallets as u64 { // Note: The principal is currently unused but in case it is used in future tests we make a // modest attempt to avoid collisions by: @@ -118,7 +89,6 @@ pub fn toy_account(account_index: u64, size: ToyAccountSize) -> Account { let hardware_wallet = NamedHardwareWalletAccount { name: format!("hw_wallet_{account_index}_{hardware_wallet_index}"), principal, - transactions: Vec::new(), }; account.hardware_wallet_accounts.push(hardware_wallet); } @@ -132,8 +102,6 @@ fn toy_account_should_have_the_requested_size() { let requested_size = ToyAccountSize { sub_accounts: 1, canisters: 2, - default_account_transactions: 3, - sub_account_transactions: 4, hardware_wallets: 5, }; let account = toy_account(9, requested_size); From c797ffef3056a6be97c6a61bb749d1d5994f42c0 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Tue, 7 May 2024 16:54:02 +0200 Subject: [PATCH 2/5] changelog --- CHANGELOG-Nns-Dapp-unreleased.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 3a8c1b61491..e7d9520baa0 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -29,6 +29,7 @@ proposal is successful, the changes it released will be moved from this file to * Stop encoding the accounts map in the `AccountsStore`. * Removed `ENABLE_HIDE_ZERO_BALANCE` feature flag. * Proposal filtering by reward status. +* Removed transactions from accounts stored in nns-dapp. #### Fixed From 639ab3c1ec6da39cff612e360e8cc5a07e079474 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Wed, 8 May 2024 11:06:36 +0200 Subject: [PATCH 3/5] changelog --- CHANGELOG-Nns-Dapp-unreleased.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index e7d9520baa0..cd8e045bcc7 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -29,7 +29,7 @@ proposal is successful, the changes it released will be moved from this file to * Stop encoding the accounts map in the `AccountsStore`. * Removed `ENABLE_HIDE_ZERO_BALANCE` feature flag. * Proposal filtering by reward status. -* Removed transactions from accounts stored in nns-dapp. +* Intermediate step to remove transactions from accounts stored in nns-dapp. #### Fixed From 88e619d7aba86367ec4f6f796be49dc64c7d9e67 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Fri, 10 May 2024 08:25:18 +0200 Subject: [PATCH 4/5] typo --- rs/backend/src/accounts_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/backend/src/accounts_store.rs b/rs/backend/src/accounts_store.rs index f567eb3aee5..39e6d354f2d 100644 --- a/rs/backend/src/accounts_store.rs +++ b/rs/backend/src/accounts_store.rs @@ -212,7 +212,7 @@ impl From for OldAccount { struct NamedSubAccount { name: String, account_identifier: AccountIdentifier, - // transactions: Do not reuse this field. There are still accounts in stable memor with this unused field. + // transactions: Do not reuse this field. There are still accounts in stable memory with this unused field. } // TODO(NNS1-2905): Delete this after it has been on mainnet for at least 1 release. From 70bc688c487fc9164d50b2e4ac4c00bcf392c65a Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Fri, 10 May 2024 08:29:47 +0200 Subject: [PATCH 5/5] encode_one/decode_one --- rs/backend/src/accounts_store/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rs/backend/src/accounts_store/tests.rs b/rs/backend/src/accounts_store/tests.rs index e9fe6cf766c..8780e5d5942 100644 --- a/rs/backend/src/accounts_store/tests.rs +++ b/rs/backend/src/accounts_store/tests.rs @@ -1113,7 +1113,7 @@ fn old_account_can_be_decoded_to_new_account() { let expected_new_account = create_new_test_account_for_encoding(); let old_account = OldAccount::from(expected_new_account.clone()); - let bytes = Candid((old_account,)).into_bytes().unwrap(); + let bytes = candid::encode_one(old_account).unwrap(); let decoded_new_account = Account::from_bytes(Cow::Owned(bytes)); assert_eq!(decoded_new_account, expected_new_account); @@ -1139,7 +1139,7 @@ fn new_account_can_be_decoded_to_old_account() { let expected_old_account = OldAccount::from(new_account.clone()); let bytes = new_account.to_bytes(); - let (decoded_old_account,): (OldAccount,) = Candid::from_bytes(bytes.into_owned()).map(|c| c.0).unwrap(); + let decoded_old_account: OldAccount = candid::decode_one(&bytes.into_owned()).unwrap(); assert_eq!(decoded_old_account, expected_old_account); }