refactor: Don't call transactionCountOf$ until needed

[amaury1093]

Only the TxForm component needs the transaction count, so we don't make additional RPC calls on other screens.

fixes #420. It makes the overall transitions between screens quicker.

[axelchalon]

Good! Maybe you can have a look at #355 (comment) as well

[amaury1093]

Ah yeah, I think you're right.

https://github.com/paritytech/fether/blob/b8d3aa460f99be97a2561f3fac9d98bffd2d6dcc/packages/fether-react/src/Send/TxForm/TxForm.js#L391-L409

I think in this case we should add chainId, transactionCount and ethBalance into the initialValues

[amaury1093]

I'm unfortunately out for vacation for 2 weeks, so this will stay in-progress for a while. If someone wants to push commits on this pr, feel free

[ltfschoen]

I think in this case we should add chainId, transactionCount and ethBalance into the initialValues

@amaurymartiny Sorry I don't understand, why would we want to add those props to initialValues?

Is the idea to add them as follows:

initialValues={
  {
    chainId: chainId,
    ethBalance: ethBalance,
    from: address,
    gasPrice: 4,
    transactionCount: transactionCount,
    ...tx
  }
}

And then show validation errors with them instead as follows:

validateForm = debounce(values => {
  if (!values) {
    return;
  }

  if (!values.chainId) {
    return { amount: 'Loading chain identifier...' };
  }

  if (!values.transactionCount) {
    return { amount: 'Loading transaction count...' };
  }

  if (!values.ethBalance) {
    return { amount: 'Loading Ethereum balance...' };
  }

But I think we need to shows these messages in a notification message (like we do when the sender and receiver address are the same) instead of as input field validation error

[ltfschoen]

I think we need to:

• Refactor to create and use @withTxCount and @withChainId decorators

• On the "Send Ether" and "Send THIBCoin" pages check in the handleSubmit method that chainId, transactionCount and ethBalance values are not undefined, since those values may change to undefined between the time that the user enters valid form values and the time that they actually click the "Send" or "Scan" button, and could result in an invalid transaction being generated infether-react/src/utils/transaction.js

• On the "Send Ether" and "Send THIBCoin" pages we now need to save the prop values of chainId, balance, ethBalance, erc20Balance, and transactionCount in the TxForm component state using getDerivedStateFromProps and use the values in the state for rendering. And then only re-render if we receive different updated values for those props. Otherwise if we don't do this then the input fields will keep being reset at intervals, which is frustrating for the user.
  An alternative is to show a "loading" overlay each time update values for these props are received, but that would be incredibly annoying to the user.

I've tried saving the values in the state using getDerivedStateFromProps but so far the values still keep reseting.

To observe their values changing just add the following in the render method of TxForm

console.log('TxForm chainId: ', this.props.chainId && this.props.chainId.toString());
console.log('TxForm balance: ', this.props.balance && this.props.balance.toString());
console.log('TxForm ethBalance: ', this.props.ethBalance && this.props.ethBalance.toString());
console.log('TxForm erc20Balance: ', this.props.erc20Balance && this.props.erc20Balance.toString());
console.log('TxForm transactionCount: ', this.props.transactionCount && this.props.transactionCount.toString());

[ltfschoen]

I've pushed additional commits in a separate branch "am-optimize-txcount-luke". I can't get it to save the input field values without them being reset

[axelchalon]

Is the idea to add them as follows:

Yes, see #355 (comment)
Basically we just want to make sure that validateForm() is called again when one of {chainId, transactionCount, ethBalance} changes

Refactor to create and use @withTxCount and @withChainId decorators

I don't think that's necessary, using the @light decorator is only one-line and does the job afaik?

check in the handleSubmit method that chainId, transactionCount and ethBalance values are not undefined, since those values may change to undefined between the time that the user enters valid form values and the time that they actually click the "Send" or "Scan" button

Why can they change back to undefined? afaik they start with undefined but as soon as they have a value they can't go back to undefined?

(cannot test the bit of code you pasted right now as my light node cannot connect to peers)

[Tbaut]

What's your take @ltfschoen? Are we good to merge?

[amaury1093]

@Tbaut No, still not, there are still some weird behavior. I'll clean up this PR this week.

[ltfschoen]

Previously I found that the validation errors only appear in the UI if when the key is amount (i.e. { amount: 'some message' }, I have not figured out why, so if you comment out the if statement, and run the code, it will not display 'Fetching chainId'.

Since it may be shown to the user who don't need to know about variable names, suggest changing it to Fetching chain ID, i.e. return { amount: 'Fetching chain ID' };.

The validation errors popup will all appear above the Amount input field with the small arrow, indicating that it's associated with a problem with that input field, which would be incorrect. But unless we can get it to associate with the correct input field we could do this in the interim, and could create a separate issue to find out why it is not working with key names of other input fields. It may be more appropriate to address these issues as general notifications as part of #331, since some aren't associated with a specific input field but instead are overall form validation errors

[ltfschoen]

For same reason as mentioned in comment https://github.com/paritytech/fether/pull/414/files#r268440068.

Suggest changing to return { amount: 'Fetching Ether balance' }; (or Fetching balance of Ether)

[ltfschoen]

For same reason as mentioned in comment https://github.com/paritytech/fether/pull/414/files#r268440068.

Suggest changing to return { amount: 'Fetching transaction count' }; (or even Fetching transaction count for nonce)

[ltfschoen]

LGTM, suggested adding amount in the interim as the key so all the validation errors open as popups, otherwise if values.chainId, values.transactionCount, or values.ethBalance is undefined after the user has entered a valid amount and recipient address, they'll have no idea why the "Send" or "Scan" button is still disabled. Suggest creating new issue to resolve why we can't associate input field validation errors with other input fields other than the amount input field

[amaury1093]

is undefined after the user has entered a valid amount and recipient address, they'll have no idea why the "Send" or "Scan" button is still disabled

Yes, I agree with that. However are you sure it's a good idea to put a tooltip on the amount field (I actually avoided that on purpose to avoid confusion)?

An alternative I can propose is to put "Checking..." text on the Scan/Send button while waiting, so that users (esp not advanced ones) don't see words like "chain ID" or "transaction count", what do you think?

Suggest creating new issue to resolve why we can't associate input field validation errors with other input fields other than the amount input field

It seems to me that this works well: if you return {amount: '...'}, then it'll show a tooltip on the amount field, here we show a tooltip on the recipient field.

[ltfschoen]

Your idea is great. Let's not put the tooltip on the amount field in those situations then.

I agree with doing Checking... instead of the Send/Sign button.
How about also doing pino.debug or pino.info for each of them so it's clear what's going on for more advanced users

[amaury1093]

Done.

[Tbaut]

This PR makes the experience so much smoother, thanks a lot!

(Agreed with not showing any "chainId" or other weird names to our users, it's not needed.)

[ltfschoen]

@amaurymartiny I'm not sure if this is related to this issue, but I just created this issue #474 and this PR #475