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: show balances in the Safe List #3546

Merged
merged 21 commits into from
May 8, 2024
Merged

Feat: show balances in the Safe List #3546

merged 21 commits into from
May 8, 2024

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Apr 10, 2024

What it solves

Resolves #3610

Screenshot 2024-04-10 at 18 26 53

How this PR fixes it

I've added @iamacook's new endpoint for balances and pending transaction counts (the counts aren't displayed yet), as well as an infinite scroll pagination for the safe list.

Gateway SDK PR: safe-global/safe-gateway-typescript-sdk#161

Fiat formatting

I've also adjusted the currency formatting function a bit: it will now not show cents for amounts greater than 1k.

@katspaugh katspaugh requested a review from jmealy April 10, 2024 16:28
Copy link

github-actions bot commented Apr 10, 2024

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Apr 10, 2024

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1010.45 KB (🟡 +2.17 KB)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Thirteen Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/apps 46.54 KB (🟢 -46 B) 1.03 MB
/apps/custom 38.17 KB (🟢 -42 B) 1.02 MB
/apps/open 74.03 KB (🟢 -1.09 KB) 1.06 MB
/balances 29.87 KB (🟢 -46 B) 1.02 MB
/balances/nfts 20.26 KB (🟢 -172 B) 1.01 MB
/home 59.7 KB (🟢 -67 B) 1.05 MB
/new-safe/create 32.22 KB (🟡 +6 B) 1.02 MB
/new-safe/load 18.35 KB (🟡 +6 B) 1 MB
/transactions 101.46 KB (🟢 -205 B) 1.09 MB
/transactions/history 101.42 KB (🟢 -205 B) 1.09 MB
/transactions/messages 61.24 KB (🟢 -198 B) 1.05 MB
/transactions/queue 56.55 KB (🟢 -533 B) 1.04 MB
/transactions/tx 46.38 KB (🟢 -40 B) 1.03 MB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

Copy link

github-actions bot commented Apr 10, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
79.33% (-0.01% 🔻)
11327/14278
🔴 Branches
58.74% (+0.01% 🔼)
2681/4564
🟡 Functions
66.49% (+0.01% 🔼)
1835/2760
🟢 Lines
80.62% (+0.01% 🔼)
10202/12655
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / useAllOwnedSafes.ts
41.18% (-3.27% 🔻)
0% 0%
50% (-7.14% 🔻)
🟢
... / useLoadBalances.ts
91.11% (-2.07% 🔻)
66.67% 100% 95%

Test suite run success

1426 tests passing in 198 suites.

Report generated by 🧪jest coverage report action from 33debac

Copy link

github-actions bot commented Apr 22, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh katspaugh marked this pull request as ready for review April 22, 2024 13:56
Copy link
Contributor

@jmealy jmealy left a comment

Choose a reason for hiding this comment

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

The balances on mobile are not positioned:
image

@katspaugh katspaugh marked this pull request as draft April 22, 2024 16:09
Copy link

github-actions bot commented Apr 26, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh
Copy link
Member Author

@jmealy fixed, thank you!

Screenshot 2024-04-26 at 09 20 19Screenshot 2024-04-26 at 09 20 28

Copy link
Contributor

@jmealy jmealy left a comment

Choose a reason for hiding this comment

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

Looks good ✨

@katspaugh katspaugh marked this pull request as ready for review April 26, 2024 08:00
@katspaugh
Copy link
Member Author

Added pending actions also:

Screenshot 2024-04-26 at 14 16 33

@francovenica
Copy link
Contributor

The "To confirm" tag shows whenever there is a tx that you (as connected owner) have not signed, even for safes 1/x, so technically there is nothing to confirm there. I'd say that, for safes 1/x the tag should not show.
image

Question:
Currently in prod, if you are not connected with an owner, no safes are shown (except in the watch list). Now it seems the safes I saw with a connected owner are being stored so even if I'm not connected the full list loads.
Is this an intended change? maybe for the new pagination?
image
image

Copy link

github-actions bot commented May 6, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh
Copy link
Member Author

Fixed the cached list on disconnect, figuring out the "1 to confirm" thing with Aaron.

@katspaugh
Copy link
Member Author

@francovenica Aaron has fixed it on the CGW side. 👍

@francovenica
Copy link
Contributor

The List of safes being unloaded when the user disconnected looks good

The other issue still persist:
The user still see the "1 to confirm" even tho the current connected user cannot confirm it
Steps:
Create a safe 1/2
Queue a tx with Owner1
Connect with Owner2
Check the safe in the login page

Since the tx is fully confirmed, neither owner1 or owner2 should see the "1 to confirm". Only the "1 pending tx" tag should show

@iamacook
Copy link
Member

iamacook commented May 8, 2024

The user still see the "1 to confirm" even tho the current connected user cannot confirm it

I am looking into this issue from the backend perspective now.

Edit: a fix has been put into review on our end. I've tested it as per your reproduction steps and it solves the issue. I'll inform you when it's live.

Copy link

github-actions bot commented May 8, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@iamacook
Copy link
Member

iamacook commented May 8, 2024

@francovenica, the "1 to confirm" issue appears to be solved now that we've merged the fix on our end.

@francovenica
Copy link
Contributor

@iamacook Yes. It works fine now.

LGTM

@katspaugh katspaugh merged commit 77b9bda into dev May 8, 2024
13 of 14 checks passed
@katspaugh katspaugh deleted the safe-list branch May 8, 2024 14:40
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Login page] Show balances and pending actions
4 participants