HathorNetwork / hathor-wallet-service-old

MIT License
4 stars 4 forks source link

design: asynchronously load tokens on both wallets #267

Open andreabadesso opened 2 years ago

andreabadesso commented 2 years ago

Summary

We should download token information asynchronously on the wallets, changing the UX accordingly to inform the user that the balances may not yet have been downloaded

One point note mentioning is that the changes proposed on this document are specifically for the new wallet-service facade, so no changes should be made when the wallets are using the old facade

Motivation

Currently, while using the wallet-service facade, our wallets download the token balances and history synchronously, downloading one token a time, as we can see on this wallet-desktop example:

  async fetchWalletData(wallet) {
    // First we get the tokens in the wallet
    const tokens = await wallet.getTokens();

    const tokensHistory = {};
    const tokensBalance = {};
    // Then for each token we get the balance and history
    for (const token of tokens) {
      /* eslint-disable no-await-in-loop */
      // We fetch history count of 5 pages and then we fetch a new page each 'Next' button clicked
      const history = await wallet.getTxHistory({ token_id: token, count: 5 * WALLET_HISTORY_COUNT });
      tokensBalance[token] = await this.fetchTokenBalance(wallet, token);
      tokensHistory[token] = history.map((element) => this.mapTokenHistory(element, token));
      /* eslint-enable no-await-in-loop */
    }

    // Then we get from the addresses iterator all addresses
    return { tokensHistory, tokensBalance, tokens };
  }

Only after that complete download, the dashboard screen is displayed with the downloaded balances and history.

Another problem is that we download the balances and history for all the tokens the user has ever interacted with, even if they are not registered

This is a problem for users with multiple tokens as the perceived loading time of the wallet is very big

Guide-level explanation

The idea is to show the main screen of the wallet as soon as the token set as default on the config and the hathor token balance is loaded and start downloading asynchronously the list of tokens the user has registered on his device.

We also should change the behaviour of the wallet to only download the history of transactions when the user effectively clicks the token to see it, also displaying a loading indicator until the page is downloaded

UI Changes

Dashboard screen

We should display a loading indicator right next to the names of the registered tokens, indicating that the balance for this token is still being downloaded.

This download will be triggered by default when the user enters this screen for each token he has registered.

2022-08-03 14 40 58

2022-08-04 12 38 34

Unknown tokens screen

On the desktop wallet, we have a screen that displays all the tokens that have not been registered yet, including their balances and history

2022-08-11 16 00 37

If the user clicks the Show history button and the balance was already loaded, we should display only the Loading history... spinner

Screen Shot 2022-08-11 at 16 14 50

If the user clicks the "Hide history" button before the history download finishes, we will continue the download in background and the next time the user clicks the "Show history" button, we will show the downloaded history.

History screen

Since the history will only be downloaded from the wallet-service lambda when the user clicks the token to see it, we should show a loading indicator until the current page of transactions is downloaded

Once the history is downloaded, we should not trigger a download when the user clicks to see the details for a token

2022-08-03 14 40 38

On the desktop wallet, since we have multiple downloads on the same screen, we should have a single loading that will be up until every request is successfully resolved

2022-08-10 11 46 55

If any of the requests fail, we should display an error screen with a button to retry

Screen Shot 2022-08-10 at 11 53 16

Send token

While on the SendAmountInput screen, after clicking the select button to change token, we should also display the loading indicator instead of the balance so the user knows that it is still downloading

2022-08-03 16 30 44

2022-08-04 13 20 56

Reference-level explanation

Currently, both facades load all the history and balances for all tokens that the wallet has ever interacted with (at least one transaction involving at least one of the addresses that the xpriv owns)

Redux-saga

I propose adding a new redux middleware library to the project called redux-saga. The idea is to move all logic that contains side-effects to sagas that reacts to actions and deals with handling data and errors

Redux-saga can be understood as a separate thread to the application, listening for specific actions from the main application to perform complex asynchronous tasks and updating the application state once it is completed.

Alternatives

Currently, we are using redux-thunk which is another redux middleware library that allows us to execute a function before returning an object on a redux action. It is also a great library, it would also (although with less clarity) solve the problem of asynchronously load tokens, but I believe that redux-saga is a better fit for this usecase as it has multiple helpers to deal with asynchronicity (like waiting for two actions before continuing), as we will see on the Sagas section of this design.

Sagas

We should have some new sagas to deal with requesting tokens history and balances and also listen to new tokens being added by the user

loadTokenHistory

This saga will use the takeEvery helper to react to every TOKEN_FETCH_HISTORY_REQUESTED action, it shoud:

  1. Check if we are using the wallet-service facade
    1. If false, do nothing as the history will already be loaded
  2. Request the token history for the tokenId sent as payload on the action
  3. If success, dispatch the TOKEN_FETCH_HISTORY_SUCCESS action, with the history and tokenId as payload
  4. If false, dispatch the TOKEN_FETCH_HISTORY_FAILURE action, with the tokenId as payload

loadTokenBalance

This saga will also use the takeEvery helper to react to every TOKEN_FETCH_BALANCE_REQUESTED action, it shoud:

  1. Check if we are using the wallet-service facade
    1. If false, do nothing as the balance will already be loaded
  2. Check if the updatedAt of the last stored information is higher than TOKEN_BALANCE_TTL
    1. If false, do dispatch TOKEN_FETCH_BALANCE_SUCCESS to set loading to false and break
  3. Request the token history for the tokenId sent as payload on the action
  4. If success, dispatch the TOKEN_FETCH_BALANCE_SUCCESS action, with the history and tokenId as payload
  5. If false, dispatch the TOKEN_FETCH_BALANCE_SUCCESS action, with the tokenId as payload

loadWallet

Currently, we are requesting the entire history and balance for all tokens that ever interacted with the wallet, this is being done on an event-listener that listens for state changes from the wallet facade

When it detects that the wallet is ready, meaning that the wallet is already started and loaded on the wallet-service side, it starts fetching balance and history for all tokens (fetched from the getTokens API), this is the method as it is today:

export const startWallet = (words, pin) => async (dispatch) => {
  (...)
  const network = new Network(networkName);

  // Set urls for wallet service
  config.setWalletServiceBaseUrl(WALLET_SERVICE_MAINNET_BASE_URL);
  config.setWalletServiceBaseWsUrl(WALLET_SERVICE_MAINNET_BASE_WS_URL);

  wallet = new HathorWalletServiceWallet({
    requestPassword: showPinScreenForResult,
    seed: words,
    network
  });

  dispatch(setWallet(wallet));

  (...)

  dispatch(fetchHistoryBegin());

  (...)

  wallet.on('state', (state) => {
    if (state === HathorWallet.ERROR) {
      // ERROR
      dispatch(fetchHistoryError());
    } else if (wallet.isReady()) {
      // READY
      fetchHistoryAndBalance(wallet)
        .then((data) => {
          dispatch(fetchHistorySuccess(data));
        }).catch(() => dispatch(fetchHistoryError()));
    }
  });

  (...)
};

export const fetchHistoryAndBalance = async (wallet) => {
  // First we get the tokens in the wallet
  const tokens = await wallet.getTokens();

  const tokensHistory = {};
  const tokensBalance = {};
  for (const token of tokens) {
    const balance = await wallet.getBalance(token);
    const tokenBalance = balance[0].balance;
    tokensBalance[token] = { available: tokenBalance.unlocked, locked: tokenBalance.locked };
    const history = await wallet.getTxHistory({ token_id: token });
    tokensHistory[token] = history.map((element) => mapTokenHistory(element, token));
  }

  return { tokensHistory, tokensBalance };
};

After the complete download of all history and balances for all tokens is done, we finally dispatch the FETCH_HISTORY_SUCCESS action that causes the loading history screen to hide, displaying the dashboard screen with the list of all tokens and their balances.

I propose we refactor the startWallet (to a saga) to dispatch the FETCH_HISTORY_SUCCESS action right after the hathor token (and any other token that is defined as the DEFAULT_TOKEN) history and balance is loaded and dispatch actions to load asynchronously the balance (and only the balance) of all other tokens that the user already registered (fetched from AsyncStorage).

It would look something like this:

sagas.js

import { race, call, put, delay, fork } from 'redux-saga/effects'

export function* startWallet (action) {
  (...)

  const { words, pin } = action.payload;

  const wallet = new HathorWalletServiceWallet({
    requestPassword: showPinScreenForResult,
    seed: words,
    network
  });

  yield put(setWallet(wallet));

  const registeredTokens = await tokensUtils.getTokens();

  // This will run in another 'thread' and will create a channel
  // that listens for state changes and dispatches actions (that we will use further ahead)
  yield fork(listenForWalletReady, wallet);

  await wallet.start({ pinCode: pin, password: pin }));

  (...)

  // The 'race' effect will stop at whatever action comes first, this will wait until the
  // wallet is loaded before continuing
  const { error } = yield race({
    success: take('WALLET_STATE_READY'),
    error: take('WALLET_STATE_ERROR'),
  });

  if (!error) {
    // Helper method to wait until a specific action with a specific payload, this should be a helper
    const specificTypeAndPayload = (type, payload) => (action) => (
      action.payload === payload && action.type === type
    );

    // Wallet loaded succesfully here, we should load the hathor token balance and history
    yield put({ type: 'TOKEN_FETCH_BALANCE_REQUESTED', payload: hathorLib.constants.HATHOR_TOKEN_CONFIG.uid });
    yield put({ type: 'TOKEN_FETCH_HISTORY_REQUESTED', payload: hathorLib.constants.HATHOR_TOKEN_CONFIG.uid });

    const customTokenUid = constants.DEFAULT_TOKEN.uid;
    const htrUid = hathorLib.constants.HATHOR_TOKEN_CONFIG.uid;

    // If the DEFAULT_TOKEN is not HTR, also download its balance and history:
    let loadCustom = [];
    if (customTokenUid !== htrUid) {
      yield put({ type: 'TOKEN_FETCH_BALANCE_REQUESTED', payload: customTokenUid });
      yield put({ type: 'TOKEN_FETCH_HISTORY_REQUESTED', payload: customTokenUid });

      loadCustom.push(take(specificTypeAndPayload('TOKEN_FETCH_BALANCE_SUCCESS', customTokenUid)));
      loadCustom.push(take(specificTypeAndPayload('TOKEN_FETCH_HISTORY_SUCCESS', customTokenUid)));
    }

    // The `all` effect will wait for all effects in the list
    yield all([
      // The `take` effect will wait until one action that passes the predicate is captured
      take(specificTypeAndPayload('TOKEN_FETCH_BALANCE_SUCCESS', htrUid)),
      take(specificTypeAndPayload('TOKEN_FETCH_HISTORY_SUCCESS', htrUid)),
      ...loadCustom,
    ]);

    // Since we already know here what tokens are registered, we can dispatch actions
    // to asynchronously load the balances of each one. The `put` effect will just dispatch
    // and continue, loading the tokens asynchronously
    for (const token of registeredTokens) {
      yield put({ type: 'TOKEN_FETCH_BALANCE_REQUESTED', payload: token })
    }

    // Finally, trigger `fetchHistorySuccess` so the user is redirected to the Dashboard screen
    yield put(fetchHistorySuccess());
  } else {
    yield put(fetchHistoryError());
  }
}

// This will create a channel from an EventEmitter to wait until the wallet is loaded,
// dispatching actions
export function* listenForWalletReady (wallet) {
  const channel = eventChannel(emitter => {
    const listener = (state) => emitter(state);
    wallet.on('state', listener);

    // Cleanup when the channel is closed
    return () => {
      wallet.removeEventListener('state', listener);
    };
  });

  while (true) {
    const message = yield take(channel);

    if (message === HathorWallet.ERROR) {
      yield put({ type: 'WALLET_STATE_ERROR' });
      break;
    } else {
      if (wallet.isReady()) {
        yield put({ type: 'WALLET_STATE_READY' });
        break;
      }
    }
  }

  // When we close the channel, it will remove the event listener
  channel.close();
}

Real-time updates

Currently, while on the wallet-service facade, we are downloading the entire token balances and history every time a new transaction is received through the websocket channel:

  wallet.on('new-tx', (tx) => {
    fetchNewTxTokenBalance(wallet, tx).then(async (updatedBalanceMap) => {
      if (updatedBalanceMap) {
        dispatch(newTx(tx, updatedBalanceMap));
        handlePartialUpdate(updatedBalanceMap);
      }
    });
  });

We should refactor this to dispatch actions to download the balances and history for the registered tokens only and also only for the tokens involved in the transaction

State and reducers

Currently, the tokensHistory and tokensBalance data on the mobile and desktop wallet are stored in memory on redux

The way the data is represented after it is loaded is the following:

{
  "tokens": [{
    "name": "Hathor",
    "symbol": "HTR",
    "uid": "00"
  }],
  "tokensBalance": {
    "00": {
      "available": 8,
      "locked": 0
    },
    "0000441258c6cb61f73c0eb421eda0402f27205bbd677cc9b0f4aee052aa00c1": {
      "available": 99,
      "locked": 0
    },
    "000000007e22060517178ad93cb655c3a314870453fde0742f399697e78b60d4": {
      "available": 100,
      "locked": 0
    },
  },
  "tokensHistory": {
    "00": [{
      "txId": "000041fb314060278bd119d2d9df40130937b18fab2ee61e6b4810c4055be1f5",
      "timestamp": 1659122733,
      "tokenUid": "00",
      "balance": 0
    }, {
      "txId": "00006a3332a32bad50f8a501bea1b6009c88b56c9c36c77e296f9e8e56a7e73f",
      "timestamp": 1659122720,
      "tokenUid": "00",
      "balance": 0
    }, ...],
  }
}

This is the new proposed state for tokensBalance and tokensHistory after it is loaded (which will be detailed in the following section of this design):

{
  "tokens": [{
    "name": "Hathor",
    "symbol": "HTR",
    "uid": "00"
  }],
  "tokensBalance": {
    "00": {
      "status": "ready",
      "updatedAt": 1661192151,
      "data": {
        "available": 8,
        "locked": 0,
      },
    },
    "0000441258c6cb61f73c0eb421eda0402f27205bbd677cc9b0f4aee052aa00c1": {
      "status": "loading",
      "updatedAt": 1661192048,
      "data": {
        "available": 99,
        "locked": 0
      },
    },
    "000000007203e0dfd75121b3862055ac5af8f33c86904480ffa3442ad016d485": {
      "status": "error",
      "updatedAt": 1661192048,
      "data": {
        "available": 99,
        "locked": 0
      },
    },
  },
  "tokensHistory": {
    "00": {
      "status": "ready",
      "updatedAt": 1661192151,
      "data": [{
        "txId": "000041fb314060278bd119d2d9df40130937b18fab2ee61e6b4810c4055be1f5",
        "timestamp": 1659122733,
        "tokenUid": "00",
        "balance": 0
      }, {
        "txId": "00006a3332a32bad50f8a501bea1b6009c88b56c9c36c77e296f9e8e56a7e73f",
        "timestamp": 1659122720,
        "tokenUid": "00",
        "balance": 0
      }, ...],
    },
  }
}

Where:

On the old facade, since we will already have this information loaded on the facade (after the address scan is done), status for the tokensHistory and tokensBalance will always be populated with ready

onTokenFetchBalanceRequested, onTokenFetchBalanceSuccess and onTokenFetchBalanceFailed reducers:

Here are some code examples of what the reducers for the actions dispatched by the sagas would look like:

export const onFetchTokenBalanceRequested = (state, action) => {
  const tokenId = action.payload;
  const oldTokenBalance = state.tokensBalance[tokenId] || {};

  return {
    ...state,
    tokensBalance: {
      ...state.tokensBalance,
      [tokenId]: {
        ...oldTokenBalance,
        status: 'loading',
      },
    },
  };
};

export const onFetchTokenBalanceRequested = (state, action) => {
  const { tokenId, balance } = action.payload;
  const oldTokenBalance = state.tokensBalance[tokenId] || {};

  return {
    ...state,
    tokensBalance: {
      ...state.tokensBalance,
      [tokenId]: {
        ...oldTokenBalance,
        status: 'ready',
        data: balance,
        updatedAt: new Date().getTime(),
      },
    },
  };
};

export const onFetchTokenBalanceFailed = (state, action) => {
  const tokenId = action.payload;
  const oldTokenBalance = state.tokensBalance[tokenId] || {};

  return {
    ...state,
    tokensBalance: {
      ...state.tokensBalance,
      [tokenId]: {
        ...oldTokenBalance, // No need to remove old data, just set status to `error`
        status: 'error',
      },
    },
  };
};

onTokenFetchHistoryRequested, onTokenFetchHistorySuccess and onTokenFetchHistoryFailed reducers:

Same as above

Old facade

While using the old facade, the wallets will use the same sagas as the new facades, so both facades will benefit from redux-saga, but the old facade will still load scan the complete list of addresses and after that, all calls to the facade methods will return instantly

Screens will be adapted to the new async interface on both facades, but since the methods on the old facade will return instantly (as the balances and history are already loaded locally), the loading shouldn't be noticeable


Reviewers: @msbrogli @pedroferreira1

pedroferreira1 commented 2 years ago

For me this Guide Level is approved, I like the new UX.

For the low level, one important note is that this might affect the real time update from the ws. With this change, we must update the internal history only if the new real time tx has one of the loaded tokens.

msbrogli commented 2 years ago

The Guide Level is approved for me. I think we should have a low level design to discuss how we will handle all loading tasks internally. I feel we should replace all "integer fields" by "object fields" with a status (stopped, loading, loaded), last update time, and value. We should also discuss the updating policy for these fields.

TalisGalhardi commented 2 years ago

From my point of view, the UX is clear and adequate for this solution.

pedroferreira1 commented 2 years ago

I like the reference level design in general, just some comments:

  1. How are we going to handle real time updates? We should only update the tokens that are already loaded, right?

Check if the updatedAt of the last stored information is higher than TOKEN_HISTORY_TTL

If false, do dispatch TOKEN_FETCH_HISTORY_SUCCESS to set loading to false and break

When are we going to use this? If the history/balance has been loaded a long time ago, should still be fine because of the real time update, right?

  1. I think a small section about the old facade would be good. Just to explain that it will continue loading all tokens before the wallet gets ready but the UI/UX and methods changes will be for both facades.
pedroferreira1 commented 2 years ago

One note about the real time

We should refactor this to dispatch actions to download the balances and history for the registered tokens only and also only for the tokens involved in the transaction

We should dispatch the actions only if the token has already been loaded, right? Actually I'm not sure about the wallet service facade, when do we start getting real time update? We wait until the wallet is ready, or until the wallet loads all tokens nowadays?

andreabadesso commented 2 years ago

When are we going to use this? If the history/balance has been loaded a long time ago, should still be fine because of the real time update, right?

You are right, I guess that this time to live does not make sense as we will always have the updated history because of real-time updates. I will remove this part, but wait until @msbrogli reads this

andreabadesso commented 2 years ago

@pedroferreira1

We should dispatch the actions only if the token has already been loaded, right? Actually I'm not sure about the wallet service facade, when do we start getting real time update? We wait until the wallet is ready, or until the wallet loads all tokens nowadays?

I agree that we should dispatch the load balance and history actions only if the token has already been loaded, will update the design

Currently on the wallet service, we are only setting up the connection after the wallet is ready (the wallet start lambda returns 'ready') but before the history and balances are loaded

msbrogli commented 2 years ago

@andreabadesso I like the idea of using saga. Good job! Here are some thoughts:

andreabadesso commented 2 years ago

@msbrogli

Thanks for your review

As far as I understood, you are replacing old integer values (e.g., a token balance) by a dict with keys status (loading, ready, and error), value, and updatedAt. Is that right? I assume that for the old face, you will always populate those values with {status: "ready", value: balance, updatedAt: now}. Is that right? This part is missing from the low level design.

That is correct, I described the new data structure for tokensBalance and tokensHistory on the State and reducers section of the design

I added this note to make it clearer what will happen on the old facade:

On the old facade, since we will already have this information loaded on the facade (after the address scan is done), status for the tokensHistory and tokensBalance will always be populated with ready

What do you think?

I feel you should explicitly explain that the new interfaces will be the same for all implementations. So you will have to update both the old facade and the wallet service facade.

I agree, updated the Old facade section

I feel you should explicitly say that all screens will be adapted to a new async interface. The old facade will resolve the promises at once while the wallet service will request information from the backend.

Also added a note for that on the Old facade section

pedroferreira1 commented 2 years ago

Approved

msbrogli commented 2 years ago

@andreabadesso Thanks!

This design is approved by me.