Alert users when the network is busy #12268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcmire
changed the title
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
mcmire
commented
Oct 2, 2021
| @@ -91,7 +91,7 @@ export default function EditGasDisplay({ | |||
|
|
|||
| useLayoutEffect(() => { | |||
| if (showAdvancedForm && scrollRef.current) { | |||
| scrollRef.current.scrollIntoView(); | |||
| scrollRef.current.scrollIntoView?.(); | |||
There was a problem hiding this comment.
Added because the unit test I wrote for EditGasPopover choked on this. My guess is that this works differently in a jsdom environment than in a real environment.
There was a problem hiding this comment.
This is still applicable, only I ended up writing a test for EditGasDisplay instead.
...ts/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js
Outdated
Show resolved
Hide resolved
...ponents/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js
Outdated
Show resolved
Hide resolved
ui/pages/confirm-transaction-base/confirm-transaction-base.component.js
Outdated
Show resolved
Hide resolved
mcmire
commented
Oct 2, 2021
jpuri
reviewed
Oct 5, 2021
jpuri
reviewed
Oct 5, 2021
...ts/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js
Outdated
Show resolved
Hide resolved
jpuri
reviewed
Oct 5, 2021
...ts/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js
Outdated
Show resolved
Hide resolved
|
Great work @mcmire , esp with test coverage. |
mcmire
commented
Oct 14, 2021
mcmire
commented
Oct 14, 2021
mcmire
force-pushed
the
add-busy-network-warning
branch
from
1e8bc62 to
8d0a9bc
Compare
mcmire
force-pushed
the
add-busy-network-warning
branch
2 times, most recently
from
d8aa3f2 to
a039c0e
Compare
mcmire
commented
Nov 3, 2021
mcmire
changed the title
mcmire
force-pushed
the
add-busy-network-warning
branch
from
a039c0e to
9a511a6
Compare
Builds ready [9a511a6]
Page Load Metrics (488 ± 52 ms)
highlights:storybook
|
mcmire
force-pushed
the
add-busy-network-warning
branch
from
9a511a6 to
596317b
Compare
Builds ready [596317b]Page Load Metrics (1162 ± 76 ms)
highlights:storybook
|
When a lot of transactions are occurring on the network, such as during an NFT drop, it drives gas fees up. When this happens, we want to not only inform the user about this, but also dissuade them from using a higher gas fee (as we have proved in testing that high gas fees can cause bidding wars and exacerbate the situation). The method for determining whether the network is "busy" is already handled by GasFeeController, which exposes a `networkCongestion` property within the gas fee estimate data. If this number exceeds 0.66 — meaning that the current base fee is above the 66th percentile among the base fees over the last several days — then we determine that the network is "busy".
mcmire
force-pushed
the
add-busy-network-warning
branch
from
596317b to
0651d8b
Compare
mcmire
changed the title
mcmire
commented
Jan 4, 2022
| @@ -133,14 +133,6 @@ export default function EditGasDisplay({ | |||
| return ( | |||
| <div className="edit-gas-display"> | |||
| <div className="edit-gas-display__content"> | |||
| {warning && !isGasEstimatesLoading && ( | |||
There was a problem hiding this comment.
I opted to take off the warning prop because I didn't see it getting used. We can very easily just add a warning ourselves to this component.
| @@ -24,24 +25,24 @@ const determineStatusInfo = (givenNetworkCongestion) => { | |||
| const color = GRADIENT_COLORS[colorIndex]; | |||
| const sliderTickValue = colorIndex * 10; | |||
|
|
|||
| if (networkCongestion <= 0.33) { | |||
There was a problem hiding this comment.
I changed this slightly so that instead of:
- not busy: <= 0.33
- stable: > 0.33, <= 0.66
- busy: > 0.66
It is now:
- not busy: 0 up to 0.33
- stable: 0.33 up to 0.66
- busy: 0.66-
This is not strictly necessary to change, but given that I need to just test that the network is busy elsewhere, it made more sense in my head to say that the status changes to busy as soon as networkCongestion crosses 0.66 instead of as soon as it exceeds it. Happy to change this back though.
| @@ -20,31 +20,31 @@ const renderComponent = ({ networkCongestion }) => { | |||
|
|
|||
| describe('StatusSlider', () => { | |||
| it('should show "Not busy" when networkCongestion is less than 0.33', () => { | |||
| const { queryByText } = renderComponent({ networkCongestion: 0.32 }); | |||
There was a problem hiding this comment.
Apparently I forgot the difference between query* and get* 😆
At the same time all of these tests changed because the logic behind the slider changed.
| export function getIsNetworkBusy(state) { | ||
| const gasFeeEstimates = getGasFeeEstimates(state); | ||
| return ( | ||
| gasFeeEstimates?.networkCongestion >= NETWORK_CONGESTION_THRESHOLDS.BUSY |
There was a problem hiding this comment.
Here's the corresponding busy check that I referenced here #12268 (comment)
| @@ -105,6 +106,13 @@ const TransactionAlerts = ({ | |||
| type="warning" | |||
| /> | |||
| )} | |||
| {isNetworkBusy ? ( | |||
There was a problem hiding this comment.
This warning applies to the EIP-1559 V2 UI and will appear on the confirm transaction screen. It will NOT appear within the edit gas modal because we already have the status slider which indicates the status of the network.
| @@ -1,172 +1,307 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
I opted to refactor these tests so that they're simpler and don't require us to recreate the Redux state so much. I can put these changes in another PR if necessary, but I will point out the additional check for isNetworkBusy regardless.
| }); | ||
| }); | ||
|
|
||
| describe('if isNetworkBusy from useGasFeeContext is truthy', () => { |
There was a problem hiding this comment.
Here are the new tests for the isNetworkBusy warning.
| @@ -10,8 +10,11 @@ import { | |||
| TRANSACTION_TYPES, | |||
| } from '../../shared/constants/transaction'; | |||
| import { transactionMatchesNetwork } from '../../shared/modules/transaction.utils'; | |||
| import { getCurrentChainId, deprecatedGetCurrentNetworkId } from './selectors'; | |||
There was a problem hiding this comment.
The main change here is to have getSelectedAddress come from ui/selectors/selectors and not from ui/selectors. The previous change created a circular dependency between ui/selectors and ui/selectors/transactions, which prevented us from jest.mock'ing ui/selectors correctly in component tests.
| import configureStore from '../../../store/store'; | ||
| import EditGasDisplay from '.'; | ||
|
|
||
| jest.mock('../../../selectors'); |
There was a problem hiding this comment.
This might have been the simplest component test file I've ever written 😆
| @@ -156,6 +148,16 @@ export default function EditGasDisplay({ | |||
| /> | |||
| </div> | |||
| )} | |||
| {isNetworkBusy ? ( | |||
There was a problem hiding this comment.
This warning applies to the EIP-1559 V1 UI. Note that this warning appears within the edit gas modal, NOT the confirm transaction screen, unlike EIP-1559 V2. I could have added it to the confirm transaction screen, but it would be more work. Given that we are soon transitioning to the V2 UI I feel like this is an okay compromise for now, but happy to change if not.
There was a problem hiding this comment.
Looks good, in V@ as I understand we are showing network busy only on edit gas fee modal.
There was a problem hiding this comment.
In V2 it's slightly different — we are showing the network busy message on the confirm transaction screen, but not in the edit gas fee modal.
Builds ready [0651d8b]Page Load Metrics (1108 ± 40 ms)
highlights:storybook
|
NiranjanaBinoy
approved these changes
Jan 7, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a lot of transactions are occurring on the network, such as during
an NFT drop, it drives gas fees up. When this happens, we want to not
only inform the user about this, but also dissuade them from using a
higher gas fee (as we have proved in testing that high gas fees can
cause bidding wars and exacerbate the situation).
The method for determining whether the network is "busy" is already
handled by GasFeeController, which exposes a
networkCongestionproperty within the gas fee estimate data. If this number exceeds 0.66 —
meaning that the current base fee is above the 66th percentile among the
base fees over the last several days — then we determine that the
network is "busy".
References
Fixes: #11307
Manual testing steps
There are two versions of this message: one for the "EIP-1559 V1" designs and one for the "EIP-1559 V2" designs. These designs impact the appearance of the confirm transaction screen and modal that appears when you edit the gas fee. You can switch from V1 to V2 locally by going into
.metamaskrcand adding the following line:Then re-running
yarn startoryarn start:dev.Once you've done this, attempt to send a transaction. Under the V1 design, you should see a message if the network is busy when you go to edit the gas fee. Under the V2 design, you should see a message if the network is busy right away on the confirm transaction screen.
What does it mean for the network to be busy? There isn't really a great way to see this because it depends on the base fee of the latest block (and the base fee of the last 20,000 blocks). That said, I made a dashboard that you can use to monitor current gas fees in realtime. It re-uses the same logic that the extension is using to determine network busyness, which is defined as when the current base fee ranks in the top third of all base fees over the last 20,000 blocks. I've provided this threshold on the dashboard. So, to test, you would wait until the current base fee is greater than this threshold. Once this happens and the base fee turns orange, try sending a transaction through the extension and you should see the "network busy" message. (Note: Based on recent base fees, this is likely to happen more often than not.)
Screencaps
eip_1559_v1_network_busy.mov
eip_1559_v2_network_busy.mov