FOLLOW-244: Stop refreshing neurons in nns-dapp canister (#5706)

# Motivation

In #5697 we added fallback
neuron refresh to the frontend in order to be able to remove it from the
backend.

This PR removes fallback neuron refresh from the nns-dapp backend
canister.

# Changes

1. Remove neuron top-up code from the backend canister.
2. Add cleanup TODOs for things that can be cleaned up after one or more
releases.

# Tests

1. Rust unit tests have been removed and updated.
2. The end-to-end test was updated because it now relies on the frontend
mechanism. I also removed a comment which was there to explain that a
test could be removed after ICRC-2 because now the test can be kept.

# Todos

- [x] Add entry to changelog (if necessary).
existing entry updated

CHANGELOG-Nns-Dapp-unreleased.md

@@ -16,7 +16,7 @@ proposal is successful, the changes it released will be moved from this file to
 
 * Provide better error messages when the transaction timestamp is off.
 * A link to the imported tokens documentation page.
-* Refresh NNS neurons from neuron details page if needed.
+* Refresh NNS neurons from neuron details page if needed, instead of from the nns-dapp canister.
 * Inform the user in the staking modal that the minimum dissolve delay for NNS neurons is 7 days.
 
 #### Changed

frontend/src/tests/e2e/neuron-increase-stake.spec.ts

@@ -58,17 +58,6 @@ test("Test neuron increase stake", async ({ page, context }) => {
 
   expect(neuronStake2).toBe(initialStake + increase1);
 
-  // Increasing neuron stake is a 2-step process:
-  // 1. Transfer ICP to the neuron account.
-  // 2. Refresh the neuron.
-  // These steps are normally performed by the frontend code, but it's
-  // possible the process gets interrupted between steps 1 and 2.
-  // To recover from this, the nns-dapp canister watches all
-  // transactions to see if it needs to refresh any neurons.
-  // This is why transferring ICP to the neuron account works to
-  // increase the neuron stake.
-  // When we switch to using ICRC-2 this is no longer necessary and
-  // this test can be removed.
   step("Increase neuron stake with transfer to neuron account");
   const increase2 = 3;
   const neuronAccount = await appPo
@@ -85,8 +74,16 @@ test("Test neuron increase stake", async ({ page, context }) => {
   });
 
   await appPo.goToNeuronDetails(neuronId);
-  // Reload the page to bypass the governance.api-service.ts cache.
+  // Reload the page to bypass checkedNeuronSubaccountsStore preventing
+  // checking a neuron more than once per session.
   await page.reload();
+
+  const toast = appPo.getToastsPo().getToastPo();
+  await toast.waitFor();
+  expect(await toast.getMessage()).toBe(
+    "Your neuron's stake was refreshed successfully."
+  );
+
   const neuronStake3 = Number(
     await appPo
       .getNeuronDetailPo()

frontend/src/tests/page-objects/Toast.page-object.ts

@@ -4,6 +4,10 @@ import type { PageObjectElement } from "$tests/types/page-object.types";
 export class ToastPo extends BasePageObject {
   private static readonly TID = "toast-component";
 
+  static under(element: PageObjectElement): ToastPo {
+    return new ToastPo(element.byTestId(ToastPo.TID));
+  }
+
   static async allUnder(element: PageObjectElement): Promise<ToastPo[]> {
     return Array.from(await element.allByTestId(ToastPo.TID)).map(
       (el) => new ToastPo(el)

frontend/src/tests/page-objects/Toasts.page-object.ts

@@ -9,6 +9,10 @@ export class ToastsPo extends BasePageObject {
     return new ToastsPo(element.byTestId(ToastsPo.TID));
   }
 
+  getToastPo(): ToastPo {
+    return ToastPo.under(this.root);
+  }
+
   getToastPos(): Promise<ToastPo[]> {
     return ToastPo.allUnder(this.root);
   }

rs/backend/src/accounts_store.rs

@@ -53,6 +53,10 @@ pub struct AccountsStore {
     // pending_transactions: HashMap<(from, to), (TransactionType, timestamp_ms_since_epoch)>
     pending_transactions: HashMap<(AccountIdentifier, AccountIdentifier), (TransactionType, u64)>,
 
+    // TODO: Remove neuron_accounts once not topping up neurons has been
+    //       released for some time. Removing this field will have to be done in
+    //       multiple steps because it is stored in stable memory during
+    //       upgrades.
     neuron_accounts: HashMap<AccountIdentifier, NeuronDetails>,
     block_height_synced_up_to: Option<BlockIndex>,
     multi_part_transactions_processor: MultiPartTransactionsProcessor,
@@ -249,7 +253,6 @@ pub enum TransactionType {
     TransferFrom,
     StakeNeuron,
     StakeNeuronNotification,
-    TopUpNeuron,
     CreateCanister,
     TopUpCanister(CanisterId),
     ParticipateSwap(CanisterId),
@@ -588,7 +591,7 @@ impl AccountsStore {
                     if let Some(principal) = self.try_get_principal(&from) {
                         let canister_ids: Vec<CanisterId> =
                             self.get_canisters(principal).iter().map(|c| c.canister_id).collect();
-                        let transaction_type = self.get_transaction_type(
+                        let transaction_type = Self::get_transaction_type(
                             from,
                             to,
                             memo,
@@ -598,12 +601,6 @@ impl AccountsStore {
                         );
                         self.process_transaction_type(transaction_type, principal, from, to, memo, block_height);
                     }
-                } else if let Some(neuron_details) = self.neuron_accounts.get(&to) {
-                    // Handle the case where people top up their neuron from an external account
-                    self.multi_part_transactions_processor.push(
-                        block_height,
-                        MultiPartTransactionToBeProcessed::TopUpNeuron(neuron_details.principal, neuron_details.memo),
-                    );
                 }
             }
         }
@@ -814,10 +811,6 @@ impl AccountsStore {
         self.neuron_accounts.get_mut(&account_identifier).unwrap_or_else(|| panic!("Failed to mark neuron created for account {account_identifier} as the account has no neuron_accounts entry.")).neuron_id = Some(neuron_id);
     }
 
-    pub fn mark_neuron_topped_up(&mut self) {
-        self.neurons_topped_up_count += 1;
-    }
-
     pub fn enqueue_multi_part_transaction(
         &mut self,
         block_height: BlockIndex,
@@ -920,7 +913,6 @@ impl AccountsStore {
 
     #[allow(clippy::too_many_arguments)]
     fn get_transaction_type(
-        &self,
         from: AccountIdentifier,
         to: AccountIdentifier,
         memo: Memo,
@@ -932,8 +924,6 @@ impl AccountsStore {
         // use the default value passed when the function is called
         if from == to {
             default_transaction_type
-        } else if self.neuron_accounts.contains_key(&to) {
-            TransactionType::TopUpNeuron
         } else if memo.0 > 0 {
             if Self::is_create_canister_transaction(memo, &to, principal) {
                 TransactionType::CreateCanister
@@ -1038,15 +1028,6 @@ impl AccountsStore {
                     MultiPartTransactionToBeProcessed::StakeNeuron(principal, memo),
                 );
             }
-            TransactionType::TopUpNeuron => {
-                if let Some(neuron_account) = self.neuron_accounts.get(&to) {
-                    // We need to use the memo from the original stake neuron transaction
-                    self.multi_part_transactions_processor.push(
-                        block_height,
-                        MultiPartTransactionToBeProcessed::TopUpNeuron(neuron_account.principal, neuron_account.memo),
-                    );
-                }
-            }
             TransactionType::CreateCanister => {
                 self.multi_part_transactions_processor.push(
                     block_height,

rs/backend/src/accounts_store/tests.rs

@@ -164,150 +164,6 @@ fn maybe_process_transaction_detects_neuron_transactions() {
     } else {
         panic!();
     }
-
-    let topup1 = Transfer {
-        from: AccountIdentifier::new(neuron_principal, None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(2).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&topup1, Memo(0), block_height + 1)
-        .unwrap();
-
-    // The neuron should be queued for refreshing
-    if let Some((_, MultiPartTransactionToBeProcessed::TopUpNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
-
-    let topup2 = Transfer {
-        from: AccountIdentifier::new(neuron_principal, None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(3).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&topup2, Memo(0), block_height + 2)
-        .unwrap();
-
-    // The neuron should be queued for refreshing
-    if let Some((_, MultiPartTransactionToBeProcessed::TopUpNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
-}
-
-#[test]
-fn maybe_process_transaction_detects_neuron_transactions_from_external_accounts() {
-    let mut store = setup_test_store();
-
-    let block_height = store.get_block_height_synced_up_to().unwrap_or(0) + 1;
-    let neuron_principal = PrincipalId::from_str(TEST_ACCOUNT_1).unwrap();
-    let neuron_memo = Memo(16656605094239839590);
-    let neuron_account = AccountsStore::generate_stake_neuron_address(&neuron_principal, neuron_memo);
-
-    let transfer = Transfer {
-        from: AccountIdentifier::new(neuron_principal, None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(1).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&transfer, neuron_memo, block_height)
-        .unwrap();
-
-    let topup = Transfer {
-        from: AccountIdentifier::new(PrincipalId::from_str(TEST_ACCOUNT_4).unwrap(), None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(2).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&topup, Memo(0), block_height + 1)
-        .unwrap();
-
-    // The neuron should be queued for refreshing
-    if let Some((_, MultiPartTransactionToBeProcessed::StakeNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
-
-    if let Some((_, MultiPartTransactionToBeProcessed::TopUpNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
-}
-
-#[test]
-fn topup_neuron_owned_by_other_principal_refreshes_balance_using_neurons_principal() {
-    let mut store = setup_test_store();
-
-    let neuron_principal = PrincipalId::from_str(TEST_ACCOUNT_1).unwrap();
-    let neuron_memo = Memo(16656605094239839590);
-    let neuron_account = AccountsStore::generate_stake_neuron_address(&neuron_principal, neuron_memo);
-    let other_principal = PrincipalId::from_str(TEST_ACCOUNT_2).unwrap();
-
-    let block_height = store.get_block_height_synced_up_to().unwrap_or(0) + 1;
-    let stake_neuron_transfer = Transfer {
-        from: AccountIdentifier::new(neuron_principal, None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(1).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&stake_neuron_transfer, neuron_memo, block_height)
-        .unwrap();
-
-    let topup = Transfer {
-        from: AccountIdentifier::new(other_principal, None),
-        to: neuron_account,
-        spender: None,
-        amount: Tokens::from_tokens(2).unwrap(),
-        fee: Tokens::from_e8s(10000),
-    };
-    store
-        .maybe_process_transaction(&topup, Memo(0), block_height + 1)
-        .unwrap();
-
-    if let Some((_, MultiPartTransactionToBeProcessed::StakeNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
-
-    if let Some((_, MultiPartTransactionToBeProcessed::TopUpNeuron(principal, memo))) =
-        store.multi_part_transactions_processor.take_next()
-    {
-        assert_eq!(principal, neuron_principal);
-        assert_eq!(memo, neuron_memo);
-    } else {
-        panic!();
-    }
 }
 
 #[test]

rs/backend/src/multi_part_transactions_processor.rs

@@ -13,6 +13,8 @@ pub struct MultiPartTransactionsProcessor {
 #[derive(Clone, CandidType, Deserialize, Debug, Eq, PartialEq)]
 pub enum MultiPartTransactionToBeProcessed {
     StakeNeuron(PrincipalId, Memo),
+    // TODO: Remove TopUpNeuron after a version has been released that does not
+    //       add TopUpNeuron to the multi-part transaction queue anymore.
     TopUpNeuron(PrincipalId, Memo),
     CreateCanisterV2(PrincipalId),
     TopUpCanisterV2(PrincipalId, CanisterId),

rs/backend/src/periodic_tasks_runner.rs

@@ -26,9 +26,10 @@ pub async fn run_periodic_tasks() {
             MultiPartTransactionToBeProcessed::StakeNeuron(principal, memo) => {
                 handle_stake_neuron(principal, memo).await;
             }
-            MultiPartTransactionToBeProcessed::TopUpNeuron(principal, memo) => {
-                handle_top_up_neuron(principal, memo).await;
-            }
+            // TODO: Remove TopUpNeuron after a version has been released that
+            //       does not add TopUpNeuron to the multi-part transaction
+            //       queue anymore.
+            MultiPartTransactionToBeProcessed::TopUpNeuron(_principal, _memo) => {}
             MultiPartTransactionToBeProcessed::CreateCanisterV2(controller) => {
                 handle_create_canister_v2(block_height, controller).await;
             }
@@ -48,13 +49,6 @@ async fn handle_stake_neuron(principal: PrincipalId, memo: Memo) {
     }
 }
 
-async fn handle_top_up_neuron(principal: PrincipalId, memo: Memo) {
-    match claim_or_refresh_neuron(principal, memo).await {
-        Ok(_) => with_state_mut(|s| s.accounts_store.mark_neuron_topped_up()),
-        Err(_error) => (),
-    }
-}
-
 async fn handle_create_canister_v2(block_height: BlockIndex, controller: PrincipalId) {
     match create_canister_v2(block_height, controller).await {
         Ok(Ok(canister_id)) => with_state_mut(|s| {