From 07810f77c47f4d13f4979aa9dbe6c2dc327bb047 Mon Sep 17 00:00:00 2001
From: Jason Spafford <nullprogrammer@gmail.com>
Date: Mon, 8 Jul 2024 16:59:53 -0400
Subject: [PATCH] Import duplicate spending key says account name (#5115)

When importing an account twice, it would not print out the name of the
account with the duplicate spending key. This will now include that in
the message.

It also improves tests to check the error type instead of error message.
We should avoid testing error messages in tests if there is a proper
typed error to check.
---
 ironfish/src/wallet/errors.ts      |  9 +++++++++
 ironfish/src/wallet/wallet.test.ts | 12 +++++++-----
 ironfish/src/wallet/wallet.ts      | 15 +++++++++------
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/ironfish/src/wallet/errors.ts b/ironfish/src/wallet/errors.ts
index 344c0b2690..342b79c235 100644
--- a/ironfish/src/wallet/errors.ts
+++ b/ironfish/src/wallet/errors.ts
@@ -43,6 +43,15 @@ export class DuplicateAccountNameError extends Error {
   }
 }
 
+export class DuplicateSpendingKeyError extends Error {
+  name = this.constructor.name
+
+  constructor(name: string) {
+    super()
+    this.message = `Account already exists with provided spending key: ${name}`
+  }
+}
+
 export class DuplicateMultisigSecretNameError extends Error {
   name = this.constructor.name
 
diff --git a/ironfish/src/wallet/wallet.test.ts b/ironfish/src/wallet/wallet.test.ts
index 86079dcd63..79202b7135 100644
--- a/ironfish/src/wallet/wallet.test.ts
+++ b/ironfish/src/wallet/wallet.test.ts
@@ -22,7 +22,11 @@ import {
 } from '../testUtilities'
 import { AsyncUtils, BufferUtils, ORE_TO_IRON } from '../utils'
 import { Account, TransactionStatus, TransactionType } from '../wallet'
-import { MaxMemoLengthError } from './errors'
+import {
+  DuplicateAccountNameError,
+  DuplicateSpendingKeyError,
+  MaxMemoLengthError,
+} from './errors'
 import { toAccountImport } from './exporter'
 import { AssetStatus, Wallet } from './wallet'
 
@@ -499,7 +503,7 @@ describe('Wallet', () => {
       expect(node.wallet.accountExists(account.name)).toEqual(true)
 
       await expect(node.wallet.importAccount(account)).rejects.toThrow(
-        'Account already exists with the name',
+        DuplicateAccountNameError,
       )
     })
 
@@ -513,9 +517,7 @@ describe('Wallet', () => {
       const clone = { ...account }
       clone.name = 'Different name'
 
-      await expect(node.wallet.importAccount(clone)).rejects.toThrow(
-        'Account already exists with provided spending key',
-      )
+      await expect(node.wallet.importAccount(clone)).rejects.toThrow(DuplicateSpendingKeyError)
     })
 
     it('should be able to import an account from solely its view keys', async () => {
diff --git a/ironfish/src/wallet/wallet.ts b/ironfish/src/wallet/wallet.ts
index ced8b5e809..d59a68da29 100644
--- a/ironfish/src/wallet/wallet.ts
+++ b/ironfish/src/wallet/wallet.ts
@@ -46,6 +46,7 @@ import { AssetBalances } from './assetBalances'
 import {
   DuplicateAccountNameError,
   DuplicateMultisigSecretNameError,
+  DuplicateSpendingKeyError,
   MaxMemoLengthError,
   NotEnoughFundsError,
 } from './errors'
@@ -1348,12 +1349,14 @@ export class Wallet {
       throw new DuplicateAccountNameError(name)
     }
 
-    const accounts = this.listAccounts()
-    if (
-      accountValue.spendingKey &&
-      accounts.find((a) => accountValue.spendingKey === a.spendingKey)
-    ) {
-      throw new Error(`Account already exists with provided spending key`)
+    if (accountValue.spendingKey) {
+      const duplicateSpendingAccount = this.listAccounts().find(
+        (a) => accountValue.spendingKey === a.spendingKey,
+      )
+
+      if (duplicateSpendingAccount) {
+        throw new DuplicateSpendingKeyError(duplicateSpendingAccount.name)
+      }
     }
 
     validateAccountImport(accountValue)