Skip to content

Conversation

@chedieck
Copy link
Collaborator

@chedieck chedieck commented Nov 21, 2023

Related to #730

Description

  1. Add cache for the balance of an address.
  2. Better organize cache logic. We currently support 3 different caches (dashboard data, payment list and the address balance), this is now reflect into three files inside the redis/ directory plus a types.ts file with all cache interfaces. Furthermore, there is now two interfaces CacheSet and CacheGet on redis/index.ts which will be used all across the codebase to actually get or set the cache values. The CacheSet interfaces export functions that are named after the unique context they will be used: for example: CacheSet.paybuttonUpdate is what will run when a paybutton is updated (and never in any other situation).
  3. Uses isSyncing new address column to show on the wallet card that one of its addresses is syncing
  4. Fix out-of-date cache issues when new txs arrive.

Test plan

Experiment with having a big address added (I use ecash:qrcuqadqrzp2uztjl9wn5sthepkg22majylrprqkuk) and checking if the dashboard values, the balance shown in the wallet and the payment list all are updated live, as the transactions arrive.

Also, after the sync is done, check that these values are not changing anymore and load very fast. This should include the logic for creating a Paybutton, since it gets the data for wallets and this in master was bottlenecking paybutton creation when a wallet would have a lot of transactions.

The whole codebase should work well even with large addresses added to the account, except for the payments page which should still await for #741 to have a smooth behavior.

@Klakurka Klakurka self-requested a review November 21, 2023 20:26
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

This is much better than what we have in master.

Here's the issues I've been able to find:

  • If you add an address with a boatload of txs, the Dashboard doesn't seem to get most of them:
image
  • Would be good to be able to differentiate between when there are 'No transactions.' and when it just needs a moment to grab them the first time. We have a separate task for this I'm pretty sure.
image
  • 'Payments' page no longer has a scrollbar for some reason (although scrolling with the mouse or keyboard still works).

@chedieck
Copy link
Collaborator Author

chedieck commented Nov 23, 2023

If you add an address with a boatload of txs, the Dashboard doesn't seem to get most of them:

This seems to be true to the IFP_ADDRESSES, I created a commit that should fix it.

Would be good to be able to differentiate between when there are 'No transactions.' and when it just needs a moment to grab them the first time. We have a separate task for this I'm pretty sure.

Indeed. I found this issue #316 which is not too specific but we can make that clearer there.

'Payments' page no longer has a scrollbar for some reason (although scrolling with the mouse or keyboard still works).

This shall be fixed when I address the Payments page in detail in the next PR,

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

When loading up the IFP address, the dashboard shows the revenue changes (doubles) on Nov 18, but this actually happened on Nov 15... any ideas?

image

It's obviously not a time zone issues as it's 3 days apart.

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

You know what -- it actually looks totally fine. Due to the difficulty adjustment that took place until Nov 17, there were fewer than expected blocks then.

We can double check the numbers later but looks good at first glance.

},
include: includeAddresses
})
// Update cache for existent addreses
Copy link
Member

Choose a reason for hiding this comment

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

Better comment would be explaining when this happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this comment was more useful before the creation of CacheSet; removed.

I don't understand what would be the when you refer to, since this function will always update the cache

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind -- but comments should be useful if they're going to be there so I'll leave it up to you what you want to do with it.

@chedieck chedieck requested a review from Klakurka November 23, 2023 18:52
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Probably just the one renaming and we're good.

@chedieck
Copy link
Collaborator Author

Probably just the one renaming and we're good.

Addressed.

@Klakurka Klakurka merged commit 27a2104 into master Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants