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

Fetch token prices & history using new Explorer BE endpoints (rebased and fixed) #322

Merged
merged 34 commits into from
Feb 5, 2024
Merged

Fetch token prices & history using new Explorer BE endpoints (rebased and fixed) #322

merged 34 commits into from
Feb 5, 2024

Conversation

nop33
Copy link
Member

@nop33 nop33 commented Jan 29, 2024 •
edited
Loading

This is @mvaivre's initial PR, rebased.

Fixes most subtasks of #243.

@nop33's subtasks:

  • Rebase to latest master
  • Use latest web3
  • Update code to use latest explorer API
  • Fix: Loading indication of total worth and chart is buggy
  • Fix: Switching currency does not update chart
  • Re-implement price polling
  • Fix greeting message animation
  • Compare loading speed with master and improve if needed

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 29, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: c04f2cf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
alephium-desktop-wallet Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nop33 nop33 mentioned this pull request Jan 29, 2024
Closed
@nop33 nop33 added the 🖥 DW Desktop wallet label Jan 29, 2024
@nop33 nop33 force-pushed the fetch-token-prices-rebased branch 2 times, most recently from fea79dd to 0ea9f05 Compare January 29, 2024 07:58
@nop33 nop33 marked this pull request as draft January 29, 2024 10:23
nop33
nop33 commented Jan 30, 2024
View reviewed changes
apps/explorer/package.json Outdated Show resolved Hide resolved
@nop33 nop33 requested a review from mvaivre January 30, 2024 08:35
@nop33 nop33 marked this pull request as ready for review January 30, 2024 08:36
@nop33 nop33 changed the title Fetch token prices & history using new Explorer BE endpoints Fetch token prices & history using new Explorer BE endpoints (rebased and fixed) Jan 31, 2024
mvaivre
mvaivre reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@mvaivre mvaivre left a comment

Choose a reason for hiding this comment

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

Awesome job! 👏

apps/desktop-wallet/src/App.tsx Show resolved Hide resolved
apps/desktop-wallet/src/App.tsx Outdated Show resolved Hide resolved
apps/desktop-wallet/src/api/addresses.ts
@@ -30,7 +30,9 @@ import {

const PAGE_LIMIT = 100
Copy link
Member

Choose a reason for hiding this comment

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

Not directly linked to this PR but this could be extracted as well (I actually thought it was already). WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

To be discussed later:

  • For maintainability sake, we could use comments to describe what each constant does.
  • Some constants would be in SHRD, those ones should IMO be imported and re-declared in each sub-package for clarity, instead of being imported directly in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, why should we re-declare them? We can easily click on the constant in our IDEs and navigate to the source where we could have the comments that you mentioned.

Copy link
Member

@mvaivre mvaivre Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I find it pretty useful to see the list of constants actually in use in the project, makes it easier to know what can be tweaked. But yeah, it's a bit more maintenance involved.

apps/desktop-wallet/src/pages/UnlockedWallet/AddressesPage/AddressGridRow.tsx Show resolved Hide resolved
apps/desktop-wallet/src/pages/UnlockedWallet/OverviewPage/AmountsOverviewPanel.tsx Show resolved Hide resolved
apps/desktop-wallet/src/storage/addresses/addressesActions.ts Show resolved Hide resolved
apps/desktop-wallet/src/storage/prices/pricesActions.ts Show resolved Hide resolved
apps/desktop-wallet/src/storage/prices/pricesActions.ts Show resolved Hide resolved
apps/explorer/package.json Outdated Show resolved Hide resolved
@nop33 nop33 requested a review from mvaivre February 5, 2024 07:33
@mvaivre
Copy link
Member

mvaivre commented Feb 5, 2024

Testing, everything looks pretty good. As expected, I'm not really happy with the amount / price history situation (lack of), but it's a temporary thing :)

@nop33 nop33 merged commit aea9137 into master Feb 5, 2024
5 checks passed
@nop33 nop33 deleted the fetch-token-prices-rebased branch February 20, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvaivre mvaivre mvaivre approved these changes

Assignees
No one assigned
Labels
🖥 DW Desktop wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nop33 @mvaivre