feat: Implement gas input validations in redesigned gas modals #15478
Conversation
| return strings('transactions.gas_modal.gas_limit_too_low'); | ||
| }; | ||
|
|
||
| export const validateGas = (value: string): string | boolean => { |
There was a problem hiding this comment.
minor, could we rename this to validateGasLimit to be more explicit?
There was a problem hiding this comment.
Good call @digiwand, in fact we should use gas because we deprecated gasLimit https://github.com/MetaMask/core/blob/main/packages/transaction-controller/src/types.ts#L809
But I just noticed that I created input as gas-limit-input which creates confusion I guess, will rename it on ogp/4831 after merging this one 👍
There was a problem hiding this comment.
I am merging this now and will fix the concern in ultimate gas modal PR here: #15234
| }); | ||
| const isAnyErrorExists = errors.gasLimit || errors.gasPrice; |
There was a problem hiding this comment.
[suggestion] possibly we can rename isAnyErrorExists → hasError
| label: string; | ||
| } & TextFieldProps; | ||
|
|
||
| export const TextInputEnhanced = (props: TextInputEnhancedProps) => { |
There was a problem hiding this comment.
Q: as we're using TextField rather than TextInput, shall we rename this? maybe TextFieldWithLabel
| autoCorrect={false} | ||
| <TextInputEnhanced | ||
| error={error} | ||
| key="gas-limit" |
There was a problem hiding this comment.
[suggestion] possibly to prevent collision, we can incorporate more of the file path name
"modal-gas-fee-limit-input"
There was a problem hiding this comment.
I don't think this will cause a collision since we are not rendering any list but I also agree that key is making a confusion and purpose is not clear - so I renamed this to inputType
|
|
||
|
|
| @@ -14,24 +17,28 @@ jest.mock('../../../../../../../../core/Engine', () => ({ | |||
| }, | |||
| })); | |||
|
|
|||
| jest.mock('../../../../../utils/gas-validations', () => ({ | |||
| validateGasPrice: jest.fn(), | |||
| })); | |||
There was a problem hiding this comment.
I am hesitant about mocking validateGasPrice. I think we can use jest spy without mocking its return value to verify it's called with the expected input
same comment applies to other instances in PR
There was a problem hiding this comment.
I understand your point on spy over mocking. But we don't really need / want validation functions to be run on these tests because then it's not testing functionality of this unit and it's not an integration test. This is why we are capsulating all validation logic under different file and assuming that they are executing.
There was a problem hiding this comment.
@digiwand to save some time, I am merging this into final gas modal PR. We can carry this conversation there <3
OGPoyraz
added
No QA Needed
Description
This PR aims to implement gas input validations.
To see it in action, please see the recordings below.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4919
Manual testing steps
N/A
Screenshots/Recordings
Before
After
eip1559.compatible.transaction.validations.mp4
legacy.transaction.validations.mp4
Pre-merge author checklist
Pre-merge reviewer checklist