refactor: fetchHistory on sagas/nanoContract to avoid iteration suspension

[alexruzenhack]

Motivation

This PR closes #514 to add concurrency on isAddressMine calls without mess with history ordering.

Acceptance Criteria

• Refactor fetchHistory to avoid iteration suspension while hydrating the NcTxHistory item with isMine flag
• As the isAddressMine is an O(1) call and don't require a http request, we can use a plain Promise.all to collect the tasks

fetchHistory still working properly

issue-fetch-history-concurrent.mp4

Security Checklist

• Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

[alexruzenhack]

[note] Receive it from the saga effect is more efficient. More efficient than that would be a flag in the wallet instance to indicate the kind of wallet, wither fullnode or walletService. Do you think should add this flag in the wallet interface?

[andreabadesso]

Suggested change

	// As isAddressMine call is async we collect the tasks to avoid
	// suspend the iteration with an await.
	// As isAddressMine call is async, we should collect the tasks to avoid
	// suspending the iteration with an await.

[andreabadesso]

Maybe this comment should also die if you accept my suggestion below

[andreabadesso]

Since every iteration of this loop will create a "task" to be awaited later, why don't you just return promises in this map and await them? something like:

const historyTransformPromises = rawHistory.map(async (rawTx) => {
  const caller = addressUtils.getAddressFromPubkey(rawTx.nc_pubkey, network).base58;
  const actions = rawTx.nc_context.actions.map((each) => ({
    type: each.type, // 'deposit' or 'withdrawal'
    uid: each.token_uid,
    amount: each.amount,
  }));

  const isMine = await utils.isAddressMine(wallet, caller, useWalletService);

  const tx = {
    txId: rawTx.hash,
    timestamp: rawTx.timestamp,
    tokens: rawTx.tokens,
    isVoided: rawTx.is_voided,
    ncId: rawTx.nc_id,
    ncMethod: rawTx.nc_method,
    blueprintId: rawTx.nc_blueprint_id,
    firstBlock: rawTx.first_block,
    caller,
    actions,
    isMine,
  };
  return tx;
});

const historyNewestToOldest = await Promise.all(historyTransformPromises);

This would also avoid iterating the loop twice

[alexruzenhack]

I believe we should start to measure performance to really answer this kind of question. But in the lack of data there is a catch in your proposal, the await provokes the block code to be suspended and add other operations as a cost. In my proposal the iteration runs from start to end without any suspension in both iterations despite to be two of them, there is only one suspension in the script Promise.all and of course the suspensions that happens in the utils.isAddressMine calls, but they happen out of the iteration context, which probably make it quicker. But this mental model requires a proof by a performance test. Maybe it didn't deliver any benefit at all, but doesn't it makes sense?

[andreabadesso]

I don't believe there will be any noticeable difference in performance, and even if there is, it will most likely be irrelevant.

Regarding the 'suspensions' you mentioned, I'm not sure I fully understand what you mean. JavaScript execution isn't blocked at all when awaiting promises. When the utils.isAddressMine promise is awaited, the promise returned by the lambda passed to map is paused until it resolves, but execution continues elsewhere in the code, including other iterations of the map. Once the promise resolves, everything after the await is placed back into the event loop and executed.

The JavaScript event loop itself isn't suspended by awaiting promises. The JavaScript engine efficiently manages asynchronous operations.

Given that, I think the main focus here should be on code clarity. In my view, the version I suggested is clearer, as it handles everything in one go without the need for additional mutations in a separate loop

[alexruzenhack]

I don't believe there will be any noticeable difference in performance, and even if there is, it will most likely be irrelevant.

I've tested it and in fact there aren't a significant difference, despite mine proposition be "slightly better", but almost not noticeable.

❯ node benchmark.js
Function 1 { avg: 602.44054917, ops: 1.6599148270775086 }
Function 2 { avg: 602.5024887400015, ops: 1.6597441814576985 }

I will not post here the benchmark ugly code, but if you want I share with you.

I'm adopting your proposition because of the clarity gain.

[andreabadesso]

Suggested change

	const historyTransformer = rawHistory.map(async (rawTx) => {
	const transformedTxHistory = rawHistory.map(async (rawTx) => {