Suggestions

[PreetamSing]

• Assuming that all the uncommented console.log statements are for dev purpose only and will be disabled in prod.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/readme.md?plain=1#L14-L26 Update Usage section to only include a start-backend command.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/services/walletService.js#L23-L28 Use short-hand notation for this instead. E.g.:

  return {
  mnemonicPhrase,
  address,
  // ...
  }

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/services/walletService.js#L40 Rather than exporting an object for named exports, you can also use the newer (probably ES6) syntax as follows. Even though it's arguable that having a single place to list all named exports is more readable, but hopefully the guys must have added this feature for a reason, and I've seen most people use newer syntax. Just making sure that you're aware of this syntax, changing won't necessarily be an improvement.

  export async function createKeyPair(mnemonicPhrase, index = 0) {

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/services/walletService.js#L6-L29 Function is async even though it doesn't use await statement inside it.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L8-L13 Can rather use following syntax.

  const { address, publicKey, privateKey, mnemonicPhrase } = await createKeyPair();

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L18-L24 model.create() function call already saves the data, so don't need to call .save() method after that. Also, assigning return value of .save() method to a variable isn't necessary if that variable won't be used. I, personally, use following syntax for saving document to db.

  const account = new accountModel({
    address,
    publickey,
    privatekey
  });
  // account.someOtherProp = (some complex logic value)
  await account.save();
  // Use values returned by saving doc, e.g. "_id" or on save hook generated value, to update dependent fields.
  account.dependentProp = account.expiry + account.x;
  await account.save();

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L37 It's a good practice to use return keyword while sending response at the end of code block, just so you don't accidentally try to send response again below in the function, e.g. in a finally block, as that'd throw error even though linter might not highlight so.

• Rather than using try-catch block in every controller for generic error handling, you can do something like following. Though, ofc you'll have to handle specific expected error scenarios in the controller itself using try-catch or some other mechanism.

  // utils.js
  export function Wrap(controller) {
  return async (req, res, next) => {
    try {
      await controller(req, res, next)
    } catch (error) {
      console.log(error);
      return res.status(500).json({
          status: 'fail',
          message: error.message // Normally we would give a generic 'Something went wrong.' kind of message, and not exact info of our internal error, for security purposes.
      });
    }
  }
  }
  
  // router.js
  yourRouterInstance.get('/some/path', Wrap(SomePathController));

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L57 Index mnemonicPhrase field in model, if findOne() is used frequently with it. How? Based on a google search, by setting index prop in schema definition of that field to true.

• Rather than having an array of accountModel's ObjectIds in walletModel. Have a wallet foreign key in accountModel referencing it's parent wallet. Because MongoDB document has a limit on it's size, 16 MB i guess, and even though chances are we're never going to hit that, still it's a good practice to have child pointing to parent rather than arrays in pretty much every database schema design, no matter if it's MongoDB or not.

• Passing mnemonicPhrase (sensitive info AFAIK) in URL is really bad practice. If you can replace that with wallet's mongo id, that'd be great. But I'm guessing your business logic dictates you can't, in which case you'd have to pass it in request body, and it is discouraged to use request body in GET API. Consequently, will have to switch to a POST API, even though it's a read request. That's the best I can think of for this case.

• I'm guessing you already know, but just to re-emphasize, sensitive info like privateKey isn't directly saved in DB without encryption ( if saved in DB at all ).

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L28 Does this line really work? As account._id is of type ObjectId, whereas accounts field needs an array of it. Might be doing some internal conversion ig, if it does work.

• You can add a .env.example file.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/controllers/walletController.js#L135-L138 Better way to do the same operation using previously fetched wallet instance.

  // Using wallet fetched in the beginning of this controller.
  wallet.numberOfAccounts++;
  wallet.accounts.push(account._id);
  await wallet.save();

• In Delete Wallet API, you can choose to soft delete the document rather than hard deletion, if the business logic asks for it.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/routes/walletRoutes.js#L7 Create Wallet should be a POST API.

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/routes/walletRoutes.js#L9-L13 validateMnemonicPhrase middleware is supposed to always throw error as it is picking up value from request body, whereas in this API you're passing mnemonicPhrase in path params. Though, I'm not sure if isValidMnemonic is correct function as I couldn't find it. Maybe something wrong on my end. Referring to following line. https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/middlewares/validateMnemonicPhrase.js#L7

• https://github.com/axatbhardwaj/Multichain-wallet/blob/0c9742d0b633aa83d4b7d90a12aadd0d7086458a/backend/routes/walletRoutes.js#L24 Add Account could be a PUT/PATCH API.

[axatbhardwaj]

  `model.create()` function call already saves the data, so don't need to call `.save()` method after that. Also, assigning return value of `.save()` method to a variable isn't necessary if that variable won't be used. I, personally, use following syntax for saving document to db.
  ```js
  const account = new accountModel({
      address,
      publickey,
      privatekey
  });
  // account.someOtherProp = (some complex logic value)
  await account.save();
  // Use values returned by saving doc, e.g. "_id" or on save hook generated value, to update dependent fields.
  account.dependentProp = account.expiry + account.x;
  await account.save();
  ```

would love to know what is it doing

specifically this part

  account.dependentProp = account.expiry + account.x;

[PreetamSing]

specifically this part

  account.dependentProp = account.expiry + account.x;

That, is just a representation of a fairly rare scenario, where you need database generated value of some fields to calculate the dependent field. Hence, you have to call save method twice. In your exact API that I've referenced, this isn't required.

Hope, that clears it up.