-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Return to send page with different asset types #28382
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. |
Builds ready [e729514]
Page Load Metrics (1899 ± 97 ms)
Bundle size diffs
|
currentConfirmation.type === TransactionType.simpleSend; | ||
const isERC20TokenSend = | ||
currentConfirmation.type === TransactionType.tokenMethodTransfer; | ||
const isNFTTokenSend = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, if these booleans only exist to support the if below, would it be cleaner to use a switch
:
let assetType: AssetType;
switch(currentConfirmation.type) {
case TransactionType.simpleSend:
assetType = AssetType.native;
break;
...
case TransactionType.tokenMethodTransferFrom:
case TransactionType.tokenMethodSafeTransferFrom:
assetType = AssetType.NFT;
break;
default:
assetType = AssetType.unknown;
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be nice to test cover also, may be in following PR.
Description
This bug was reproducible by opening a new wallet initiated send confirmation with a Native token ("simple send") from the extension full screen view, and then triggering a dApp initiated confirmation, and trying to return back to the send flow stepper.
The bug was provoked due to having the
editExistingTransaction
action dispatched on back button click hardcoded for asset of type token. The fix involves dynamically determining the asset type.Related issues
Fixes: #28316
Manual testing steps
See above or check video on the bug report ticket.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist