Skip to content

feat: finish swap+send UI and handle transactions#23889

Merged
BZahory merged 60 commits intomb843/swap-and-sendfrom
mb843/ui-2
May 1, 2024
Merged

feat: finish swap+send UI and handle transactions#23889
BZahory merged 60 commits intomb843/swap-and-sendfrom
mb843/ui-2

Conversation

@BZahory
Copy link
Copy Markdown
Contributor

@BZahory BZahory commented Apr 8, 2024 •
edited by github-actions bot
Loading

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@BZahory BZahory requested review from a team as code owners April 8, 2024 13:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2024

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.

@BZahory BZahory changed the base branch from develop to mb843/data-flows April 8, 2024 13:49
micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
ui/components/app/currency-input/currency-input.js
address: PropTypes.string,
symbol: PropTypes.string,
decimals: PropTypes.number,
decimals: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What causes this to be a string? Can the value be normalized to a number on selection, before it's passed into this component?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a mistype somewhere. The commit (a5a5417) was made in response to a console error (not thrown) regarding an invalid prop type being passed. Also doesn't hurt to have a bit of extra robustness anyways.

micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
ui/components/app/currency-input/currency-input.js
setTokenDecimalValue(newTokenDecimalValue);
setFiatDecimalValue(newFiatDecimalValue);

// timeout intentionally not cleared so this always runs
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by this? Won't a new timeout always be created when the dependencies change anyway, assuming the component is not unmounted?

Copy link
Copy Markdown
Member

@micaelae micaelae Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why is the timeout needed?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the dependencies update but Number(decimalizedHexValue) === Number(tokenDecimalValue) is true, then the timeout won't be ran after the update.

The ref doesn't update on the input state variable's (or any state variable's) cadence, so we need a delay to give the input – and its ref – time to update before checking.

micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
ui/components/multichain/pages/send/components/quote-card/hooks/useGetConversionRate.tsx

const NATIVE_CURRENCY_DECIMALS = 18;

export default function useGetConversionRate() {
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you adding unit tests for this?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah will do in the testing PR

micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
ui/components/multichain/pages/send/components/quote-card/hooks/useTranslatedNetworkName.tsx
Comment on lines +7 to +8
export default function useTranslatedNetworkName() {
const chainId = useSelector(getCurrentChainId);
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you adding unit tests for this?

micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
micaelae
micaelae reviewed Apr 17, 2024
View reviewed changes
micaelae
micaelae reviewed Apr 18, 2024
View reviewed changes
ui/ducks/send/send.js
draftTransactionInitialState.swapQuotesError;
draftTransaction.isSwapQuoteLoading =
draftTransactionInitialState.isSwapQuoteLoading;
draftTransaction.swapQuotesLatestRequestTimestamp =
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the latestFetchTime value be derived from this?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swapQuotesLatestRequestTimestamp is the time of the request, not the response. It's used for cleaning up multiple ongoing fetches

Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an abort functionality we can use? similar to the AbortController in portfolio

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but the quotes kept coming through anyways sometimes IIRC

micaelae
micaelae reviewed Apr 18, 2024
View reviewed changes
ui/ducks/send/send.js
Comment on lines +2107 to +2114
const isSwapAndSend =
draftTransaction?.sendAsset?.details?.address !==
draftTransaction?.receiveAsset?.details?.address;
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract to a selector?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selectors are created later in the file (i.e., after usage)

Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Maybe an internal method then? This condition is used multiple times so that would keep them in sync and reusable

micaelae
micaelae reviewed Apr 18, 2024
View reviewed changes
ui/ducks/send/send.js
Comment on lines 2749 to +2755
const quotesAsArray = Object.values(quotes || {});
if (isSwapQuoteLoading || swapQuotesError || !quotesAsArray.length) {
if (swapQuotesError || !quotesAsArray.length) {
Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the keys of quotes used? If not, it might make more sense to store the quotes as an array in state

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine if I revisit this once the best quote logic is finalized?

Copy link
Copy Markdown
Member

@micaelae micaelae Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, can you add it to the TODO list?

Copy link
Copy Markdown
Contributor Author

@BZahory BZahory Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the next PR: aff8ab57c46d942cb0278ed7c42c8a59fe073eba

Bilal Zahory and others added 25 commits April 24, 2024 13:46
chore: remove 1155 filtering
15e1c4e
chore: tweak search bar
adfa9f4
chore: remove extra search bar
6dc5065
feat: show sending asset in dest modal
e141e55
refactor: extract asset list
bd8b19b
feat: only allow swaps for tokens
c8fee38
chore: remove vertical tab padding
3af357b
chore: update modal title
a2a7712
chore: tweak spacing
8c71258
chore: shorten raw address
a2d5beb
fix: update condition
b95e1f2
docs: update merge train tasks
07a782b
refactor: extract AssetList
5016bb6
perf: simplify token list logic
b659791
@BZahory @micaelae
chore: remove unneeded condition
a230680 
Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>
perf: memoize token list
f72d2fd
@BZahory @micaelae
chore: remove unneeded variables
822279c 
Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>
@BZahory @micaelae
chore: use existing method for conversion
4c021a5 
Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>
chore: ensure selected variable is populated
7539182
refactor: extract help url
12c4df4
style: appease lint gods: the reboot
0424eda
chore: memoize value
97bd5d0
chore: tweak skeleton styling
b6b2e33
@BZahory @micaelae
docs: update description
d183f74 
Co-authored-by: micaelae <100321200+micaelae@users.noreply.github.com>
chore: merge files after rebase
9a55802
@BZahory BZahory merged commit f0455a3 into mb843/swap-and-send May 1, 2024
@BZahory BZahory deleted the mb843/ui-2 branch May 1, 2024 20:14
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@micaelae micaelae micaelae approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BZahory @micaelae