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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

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

wants to merge 10 commits into from

Conversation

infiniteflower
Copy link
Contributor

@infiniteflower infiniteflower commented Nov 22, 2024 •
edited
Loading

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.

@github-actions GitHub Actions
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%)

@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%)

@infiniteflower infiniteflower mentioned this pull request Nov 22, 2024
Merged
7 tasks
@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
@infiniteflower
feat: cross chain swaps - tx status - BridgeStatusController (#28636)
8773328 
<!--
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
@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 •
edited
Loading

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
View reviewed changes
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
View reviewed changes
@infiniteflower infiniteflower mentioned this pull request Nov 27, 2024
Closed
7 tasks
micaelae
micaelae previously approved these changes Nov 27, 2024
View reviewed changes
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micaelae micaelae micaelae left review comments

@jclancy93 jclancy93 jclancy93 left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
team-bridge team-swaps
Projects
PR review queue
Status: Needs dev review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@infiniteflower @metamaskbot @jclancy93 @darkwing @micaelae