Skip to content
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

feat: cross chain swaps - tx status - UI #28657

Merged
merged 10 commits into from
Dec 2, 2024

Conversation

infiniteflower
Copy link
Contributor

@infiniteflower infiniteflower commented Nov 22, 2024

Description

This PR is a collection of all the UI related code from #27740 (no UI changes). It has been split up in order to make it easier to review.

The main addition is the Bridge Transaction Details and its supporting code.

Open in GitHub Codespaces

Related issues

Related to #27740, #28636

Manual testing steps

Add BRIDGE_USE_DEV_APIS=1 to .metamaskrc to enable Bridge

Refer to to #27740

Screenshots/Recordings

Refer to to #27740

Pre-merge author checklist

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.

Copy link
Contributor

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.

@infiniteflower infiniteflower changed the base branch from develop to cross-chain-swaps-bridgeStatusController November 22, 2024 17:33
@infiniteflower infiniteflower changed the title Cross chain swaps status UI feat: cross chain swaps - tx status - UI Nov 22, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [3f3864f]
Page Load Metrics (1976 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24621931897401192
domContentLoaded17462132193112259
load17572190197613263
domInteractive1577482010
backgroundConnect13178464321
firstReactRender883651688239
getState692262612
initialActions01000
loadScripts12451595142010249
setupStore69014209
uiStartup196628872336263126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11.32 KiB (0.19%)
  • ui: 36.22 KiB (0.47%)
  • common: 2.61 KiB (0.03%)

@infiniteflower infiniteflower force-pushed the cross-chain-swaps-status-ui branch from 3f3864f to e522a06 Compare November 22, 2024 18:47
@metamaskbot
Copy link
Collaborator

Builds ready [74b114e]
Page Load Metrics (1933 ± 118 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint166127851935246118
domContentLoaded161926921903231111
load166027931933245118
domInteractive2289452210
backgroundConnect11103302211
firstReactRender481631122914
getState45915188
initialActions01000
loadScripts11701957140317986
setupStore661192110
uiStartup182433562166317152
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11.27 KiB (0.19%)
  • ui: 36.25 KiB (0.47%)
  • common: 2.7 KiB (0.03%)

@metamaskbot
Copy link
Collaborator

Builds ready [47a8853]
Page Load Metrics (2138 ± 114 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint42727762050439211
domContentLoaded176727302085217104
load183927792138237114
domInteractive308341136
backgroundConnect20145603115
firstReactRender752881134421
getState697272412
initialActions01000
loadScripts12952160154319493
setupStore76319189
uiStartup205330072386263126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11.27 KiB (0.19%)
  • ui: 36.52 KiB (0.47%)
  • common: 2.77 KiB (0.03%)

@metamaskbot
Copy link
Collaborator

Builds ready [ac773b7]
Page Load Metrics (1893 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32725541824390187
domContentLoaded16422469186316680
load16502556189317885
domInteractive22135482612
backgroundConnect1086362311
firstReactRender631661022512
getState55420178
initialActions00000
loadScripts12221847136613364
setupStore6501095
uiStartup18462775209819091
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11.27 KiB (0.19%)
  • ui: 35.8 KiB (0.46%)
  • common: 2.77 KiB (0.03%)

github-merge-queue bot pushed a commit that referenced this pull request Nov 26, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR is a collection of all the background related code from #27740
(no UI changes). It has been split up in order to make it easier to
review. A follow up PR containing all the UI changes from #27740 is
here: #28657

The main addition is the `BridgeStatusController` and its supporting
code.

If you would like to test the functionality of this PR through the UI,
please do so through #27740.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28636?quickstart=1)

## **Related issues**

Branched off from #27740

## **Manual testing steps**

Refer to #27740

## **Screenshots/Recordings**

Refer to #27740

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
Base automatically changed from cross-chain-swaps-bridgeStatusController to develop November 26, 2024 15:04
@infiniteflower infiniteflower force-pushed the cross-chain-swaps-status-ui branch from ac773b7 to 98cd8d4 Compare November 26, 2024 18:05
@metamaskbot
Copy link
Collaborator

Builds ready [569b06f]
Page Load Metrics (1778 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15402126177218991
domContentLoaded15222117174019292
load15442156177819493
domInteractive258245189
backgroundConnect985372311
firstReactRender1799463115
getState589282512
initialActions01000
loadScripts10701631127717082
setupStore677212211
uiStartup171928032060257124

@infiniteflower infiniteflower marked this pull request as ready for review November 26, 2024 20:02
@infiniteflower infiniteflower requested review from a team as code owners November 26, 2024 20:02
@jclancy93
Copy link
Contributor

jclancy93 commented Nov 27, 2024

I'm noticing an issue where I'm seeing the transaction pending on both the Lifi and socket explorers but it's returning pending in the UI and LiFi because of dest transaction despite the transaction being complete

https://www.loom.com/share/f66dec23c63b4353aa289a2723e3d984

It does look like it was executed through lifi based on the calldata for the transaction. So seems like it may just be an issue on Lifi side

Update: once Lifi was correctly resolving the transaction status, I could see the bridge showing up correctly in the UI

@jclancy93
Copy link
Contributor

I also see issues sometimes when submitting bridge transactions. I know it's not directly related to status but thought I'd flag here anyway
image

jclancy93
jclancy93 previously approved these changes Nov 27, 2024
Copy link
Contributor

@jclancy93 jclancy93 left a comment

Choose a reason for hiding this comment

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

Apart from some edge cases I'm seeing here, the polling and status fetching seems to work correctly

@infiniteflower
Copy link
Contributor Author

I also see issues sometimes when submitting bridge transactions. I know it's not directly related to status but thought I'd flag here anyway

This Action must be plain objects error is fixed in #28460

Specifically this commit: 8f699a3

@infiniteflower
Copy link
Contributor Author

I'm noticing an issue where I'm seeing the transaction pending on both the Lifi and socket explorers but it's returning pending in the UI and LiFi because of dest transaction despite the transaction being complete

https://www.loom.com/share/f66dec23c63b4353aa289a2723e3d984

It does look like it was executed through lifi based on the calldata for the transaction. So seems like it may just be an issue on Lifi side

Update: once Lifi was correctly resolving the transaction status, I could see the bridge showing up correctly in the UI

So is the issue here that the MM UI is not updating fast enough given that the Socket Explorer said that both source + dest txs were confirmed?

micaelae
micaelae previously approved these changes Nov 27, 2024
@infiniteflower infiniteflower dismissed stale reviews from micaelae and jclancy93 via 0077794 November 27, 2024 20:59
@infiniteflower infiniteflower force-pushed the cross-chain-swaps-status-ui branch from 569b06f to 0077794 Compare November 27, 2024 20:59
micaelae
micaelae previously approved these changes Nov 27, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [b44c49f]
Page Load Metrics (1910 ± 83 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16882354191017182
domContentLoaded16512311187616981
load17032353191017383
domInteractive257337147
backgroundConnect896332612
firstReactRender165423105
getState5111286189
initialActions01000
loadScripts12401850143615876
setupStore728952
uiStartup19532667216120397
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -1.04 KiB (-0.02%)
  • ui: 36.18 KiB (0.46%)
  • common: 2.61 KiB (0.03%)

@darkwing
Copy link
Contributor

Most of the UI functions pretty well but I never get a quote or am able to execute the bridge:

Screen.Recording.2024-11-27.at.4.59.25.PM.mov

@infiniteflower
Copy link
Contributor Author

Most of the UI functions pretty well but I never get a quote or am able to execute the bridge:

Screen.Recording.2024-11-27.at.4.59.25.PM.mov

Spoke to the team, the UI for "no quotes received" is still in development.

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Just nits but I won't block on them provided they are fixed shortly thereafter.

Also, I'd prefer that Confirmations look at the transaction math.

transition: width 1.5s cubic-bezier(0.68, -0.55, 0.27, 1.55);

&--pending {
width: 50%;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These width values are likely available via a prop for Box

}

&__step-grid {
display: grid;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be constructed with props instead of CSS.

import { getTransactionBreakdownData } from '../../../components/app/transaction-breakdown/transaction-breakdown-utils';
import { MetaMaskReduxState } from '../../../store/store';
import { hexToDecimal } from '../../../../shared/modules/conversion.utils';
import UserPreferencedCurrencyDisplay from '../../../components/app/user-preferenced-currency-display/user-preferenced-currency-display.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better path for this?

import UserPreferencedCurrencyDisplay from '../../../components/app/user-preferenced-currency-display/user-preferenced-currency-display.component';
import { EtherDenomination } from '../../../../shared/constants/common';
import { PRIMARY } from '../../../helpers/constants/common';
import CurrencyDisplay from '../../../components/ui/currency-display/currency-display.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better path for this?

@infiniteflower
Copy link
Contributor Author

infiniteflower commented Dec 2, 2024

Just nits but I won't block on them provided they are fixed shortly thereafter.

Will fix all your comments in a follow up PR, thanks so much for the approve 🙏 !

Also, I'd prefer that Confirmations look at the transaction math.

If you're referring to getTransactionBreakdownData, that was just pulling existing logic into a reusable function. No logic has been changed.

@infiniteflower infiniteflower added this pull request to the merge queue Dec 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 2, 2024
@infiniteflower infiniteflower added this pull request to the merge queue Dec 2, 2024
Merged via the queue into main with commit 19d5637 Dec 2, 2024
75 checks passed
@infiniteflower infiniteflower deleted the cross-chain-swaps-status-ui branch December 2, 2024 22:29
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2024
@metamaskbot metamaskbot added the release-12.10.0 Issue or pull request that will be included in release 12.10.0 label Dec 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.10.0 Issue or pull request that will be included in release 12.10.0 team-bridge team-swaps
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants