Fix fetchActions

[Comdex]

• Fix the issue mentioned in Fix Fetch.fetchActions and add new fetchActions method for external use #843
  This problem is because the actionData array returned by the archive node is in descending order according to accoutUpdateId, which will cause problems in the order of using actions to calculate actionHash, and the actionData array needs to be reversed.

• Removed redundant code

[mitschabaude]

are we sure we want to remove this? if yes, why? in this else branch, we don't have accountUpdateId === currentAccountUpdateId so the line is not redundant

[Comdex]

Yes, sure, because the code after the end of this branch will also perform this operation.
You can see the code comments below:

fetchedActions.forEach((fetchedAction) => {
    const { actionData } = fetchedAction;
    let latestActionsHash = Field(fetchedAction.actionState.actionStateTwo);
    let actionState = Field(fetchedAction.actionState.actionStateOne);

    if (actionData.length === 0)
      throw new Error(
        `No action data was found for the account ${publicKey} with the latest action state ${actionState}`
      );

    let { accountUpdateId: currentAccountUpdateId } = actionData[0];
    let currentActionList: string[][] = [];

    actionData.forEach((action, i) => {
      const { accountUpdateId, data } = action;
      const isLastAction = i === actionData.length - 1;
      const isSameAccountUpdate = accountUpdateId === currentAccountUpdateId;

      if (isSameAccountUpdate && !isLastAction) {
        currentActionList.push(data);
        currentAccountUpdateId = accountUpdateId;
        return;
      } else if (isSameAccountUpdate && isLastAction) {
        currentActionList.push(data);
      } else if (!isSameAccountUpdate && isLastAction) {
        // branch 3
        latestActionsHash = processActionData(
          currentActionList,
          latestActionsHash
        );
        actionsList.push({
          actions: currentActionList,
          hash: Ledger.fieldToBase58(Field(latestActionsHash)),
        });
        // The following line of code is redundant
        currentAccountUpdateId = accountUpdateId;
        currentActionList = [data];
      }

      latestActionsHash = processActionData(
        currentActionList,
        latestActionsHash
      );
      actionsList.push({
        actions: currentActionList,
        hash: Ledger.fieldToBase58(Field(latestActionsHash)),
      });
      // After the above branch 3 is executed, the currentAccoutUpdateId will also be updated here
      currentAccountUpdateId = accountUpdateId;
      currentActionList = [data];
    });

[mitschabaude]

thanks!

[mitschabaude]

The fix makes sense to me -- previously, we reversed the blocks of fetched actions, now we're also reversing them within each block. But would be nice @MartinMinkov if you could also approve this

[Comdex]

I just found another issue. The behavior of zkapp.reducer.getActions under LocalBlockChain and Berkeley is different. Under the Berkeley network, executing the getActions method will also get the action corresponding to fromActionState, but it does not under LocalBlockChain. In my personal opinion, the handling of LocalBlockChain is more user-friendly because the action that corresponds to fromActionState is typically already processed. If we want to solve this problem, there are two feasible solutions: handling it in the 'fetchActions' or processing it on the archive node side. What do you think about this? @mitschabaude @MartinMinkov

[mitschabaude]

I just found another issue. The behavior of zkapp.reducer.getActions under LocalBlockChain and Berkeley is different. Under the Berkeley network, executing the getActions method will also get the action corresponding to fromActionState, but it does not under LocalBlockChain. In my personal opinion, the handling of LocalBlockChain is more user-friendly because the action that corresponds to fromActionState is typically already processed. If we want to solve this problem, there are two feasible solutions: handling it in the 'fetchActions' or processing it on the archive node side. What do you think about this? @mitschabaude @MartinMinkov

Thanks for digging into this @Comdex. I agree - the actionState usually refers to the state after hashing some particular action. We don't want to include that action when we fetch fromActionState -- we want to start with the next one