Adding warnings for excessive custom gas input #10582
Conversation
ui/app/ducks/metamask/metamask.js
Outdated
| @@ -47,6 +47,7 @@ export default function reduceMetamask(state = {}, action) { | |||
| participateInMetaMetrics: null, | |||
| metaMetricsSendCount: 0, | |||
| nextNonce: null, | |||
| customGasIsExcessive: false, | |||
There was a problem hiding this comment.
After navigating away from the advanced gas options, hex value representations of the custom gas are wiped from memory, so we can't recalculate using isCustomPriceExcessive to display the warning on the Send screen. We can just cache it here for access.
There was a problem hiding this comment.
We mostly use this Redux slice for holding background data. We've added UI-specific data here in the past, but we had plans to migrate all of that state elsewhere and reduce this to just the background state. In the meantime I think we should avoid adding more UI state here.
I'm not sure I follow the reason for caching this in the first place though 🤔 The full transaction state remains accessible throughout the send flow. It should be accessible while the user is on the Send screen.
There was a problem hiding this comment.
So from the send transaction screen, after navigating to "Advanced Options", and inputting 100 GWEI for the custom gas amount, the value of state.gas.customData is: {price: "0x174876e800", limit: "0x5208"}. The excessive calculation is then performed on that price hex value.
After clicking "Save" and the advanced options being closed, the value of state.gas.customData is: {price: null, limit: null}, so the Send component if it uses the isCustomPriceExcessive selector at that point will always see that to be false.
I'm not sure if there's a good reason that state.gas.customData.price is cleared after closing the advanced options, but maybe it should persist as long as the transaction screen is open at all?
There was a problem hiding this comment.
A couple of things on this.
- I don't think we need to refer to this as a cache, it is just an application state entry. I understand that we're not doing the calculation again, which mimics a cache-like behavior, but that's a side effect of not having access.
- I think that this entry should likely be in the 'send' duck, vs metamask state. I believe this slice is used/cleared from the send page level.
There was a problem hiding this comment.
Oops, refresh page before commenting brad, duh.
There was a problem hiding this comment.
After clicking "Save" and the advanced options being closed, the value of state.gas.customData is: {price: null, limit: null}, so the Send component if it uses the isCustomPriceExcessive selector at that point will always see that to be false.
At that point, the new gas data is saved on the actual transaction though, which is in state.metamask.send.gasPrice. The selector can use that value instead, without needing to cache anything.
The state.gas slice is used for state relating to the gas customization modal that is currently open (which, for the record, seems like a fundamentally poor use of Redux but 🤷 It is what it is). I'm guessing that the custom data is cleared when the modal closes, because then it's not meant to be used for anything.
The state for the Send flow is currently split between state.send and state.metamask.send, with state.send being the preferred place for all send state.
I'm not opposed to adding this to the send state, but using send.gasPrice directly seems safer to me. That way if it changes for any reason, this warning will always reflect the correct state.
There was a problem hiding this comment.
Ah good to know! I think ideally nothing would be stored in the state for this, I can modify the isCustomPriceExcessive selector to check state.metamask.send.gasPrice when we are outside the customization modal 👍
There was a problem hiding this comment.
I'm not opposed to adding this to the send state, but using send.gasPrice directly seems safer to me. That way if it changes for any reason, this warning will always reflect the correct state.
Yeah that looks like the winner.
There was a problem hiding this comment.
Fixed! Now no modifications to the state, I added some additional selector tests to support the send.gasPrice check
Builds ready [28591ea]
Page Load Metrics (604 ± 21 ms)
|
There was a problem hiding this comment.
With the exception of moving the state related to this excessive gas error into the send slide of state, this looks good to me. Ran locally to QA and did code review., prefer to use the gasPrice from the metamask.send.gasPrice and remove the need to have an additional state entry.
Builds ready [1cf6846]
Page Load Metrics (572 ± 26 ms)
|
ui/app/components/app/gas-customization/advanced-gas-inputs/advanced-gas-inputs.component.js
Show resolved
Hide resolved
...ustomization/gas-modal-page-container/advanced-tab-content/advanced-tab-content.component.js
Outdated
Show resolved
Hide resolved
Builds ready [74dfbe8]
Page Load Metrics (566 ± 22 ms)
|
Fixes #9811
Manual testing steps: