EIP-1559 - Restore custom values in Edit Gas Popover #11589
Conversation
|
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. |
darkwing
force-pushed
the
restore-values-upon-save
branch
from
a137dd4 to
9ae66b6
Compare
| @@ -120,6 +120,12 @@ export default function EditGasPopover({ | |||
| dispatch(createSpeedUpTransaction(transaction.id, newGasSettings)); | |||
| break; | |||
| case EDIT_GAS_MODES.MODIFY_IN_PLACE: | |||
| // TODO: This shouldn't be required by prevents the following error: | |||
There was a problem hiding this comment.
I think this will likely be handled by this change in the send slice update PR: https://github.com/MetaMask/metamask-extension/pull/11549/files#diff-bad76d7279418f3520a0ca93cfaeeb6fcbb1f0d4782f6ff942d422632a1c2780R917
There was a problem hiding this comment.
@danjm Is this confirmed? Should I remove from here?
There was a problem hiding this comment.
Confirmed that between that change and #11615, this bug goes away and the below if block is not needed
| // transitional because it is only used to modify a transaction in the | ||
| // metamask (background) state tree. | ||
| const [maxFeePerGas, setMaxFeePerGas] = useState( | ||
| transaction?.txParams?.maxFeePerGas |
There was a problem hiding this comment.
So, in Swaps when we want to edit gas on the View Quote page, we don't have a transaction yet.
However, we store custom gasLimit, maxFeePerGas and maxPriorityFeePerGas in swapsState, which we could pass to the EditGasPopover component, e.g. in the customGas object instead of the transaction object.
What do you think?
There was a problem hiding this comment.
@darkwing was this addressed ?
There was a problem hiding this comment.
@adonesky1 Yup, but in another place. @danjm has a commit that handles prefilling of the gas editing modal in Swaps for EIP-1559. We just need to get this PR in and then I can include Dan's commit in my PR that I will open shortly.
There was a problem hiding this comment.
Swaps can be addressed separately from this PR, IMO
| transaction?.txParams?.gasPrice | ||
| ? Number(hexWEIToDecGWEI(transaction.txParams.gasPrice)) | ||
| : null, | ||
| ); |
There was a problem hiding this comment.
This change is needed below:
There was a problem hiding this comment.
Because the property on txParams is called gas and no gasLimit, and because gas/gasLimit is not in WEI or GWEI (it does not have a unit)
There was a problem hiding this comment.
the property on txParams is called
gasand notgasLimit
We do need to better improve this naming situation in the codebase. Ideally, it would be the same everywhere... but that is for a different PR
|
Updated! |
Here we grab values from an existing transaction so we can restore values chose by the user.
ToDo