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: cp-8368 historyservice #42

Merged
merged 18 commits into from
Sep 13, 2024
Merged

feat: cp-8368 historyservice #42

merged 18 commits into from
Sep 13, 2024

Conversation

vvava
Copy link
Contributor

@vvava vvava commented Sep 10, 2024

Description

We want to use to fetch the transaction histories via the vm-modules.

Changes

Get rid of the old solutions and a unified history service fetch all the activities through the modules system.

Testing

Onboarding with a wallet which has p and / or x chain history. Try to go to all the chains and check the Activity, it should be almost the same as before except the ERC-1155 token transfers should show up.

Screenshots:

Screen.Recording.2024-09-12.at.18.30.10.mov

Checklist for the author

Tick each of them when done or if not applicable.

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

Copy link
Member

@meeh0w meeh0w left a comment

Choose a reason for hiding this comment

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

I know it's still a draft, but just wanted to leave my thoughts:

I liked how @gergelylovas got rid of different types of balances services and only left one BalancesService. Can we do the same with the HistoryServices?

For reference, here's the link to his BalancesService:

We could essentially be left with just HistoryService here that could load specific module for a given network and invoke getTransactionHistory() on it.

@vvava vvava changed the title Feat/cp 8368 historyservice feat: cp-8368 historyservice Sep 11, 2024
@vvava
Copy link
Contributor Author

vvava commented Sep 11, 2024

I know it's still a draft, but just wanted to leave my thoughts:

I liked how @gergelylovas got rid of different types of balances services and only left one BalancesService. Can we do the same with the HistoryServices?

For reference, here's the link to his BalancesService:

We could essentially be left with just HistoryService here that could load specific module for a given network and invoke getTransactionHistory() on it.

good point, thanks!

@vvava vvava marked this pull request as ready for review September 12, 2024 17:42
Copy link
Member

@meeh0w meeh0w left a comment

Choose a reason for hiding this comment

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

Great work!! 👏

I left some comments, but I think they'll be pretty quick to fix :)

src/background/services/history/HistoryService.ts Outdated Show resolved Hide resolved
src/background/services/history/HistoryService.ts Outdated Show resolved Hide resolved
src/background/vmModules/ModuleManager.ts Outdated Show resolved Hide resolved
src/background/vmModules/ModuleManager.ts Outdated Show resolved Hide resolved
src/background/vmModules/mocks/avm.manifest.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/__mocks__/@blockaid/client.ts Outdated Show resolved Hide resolved
Copy link
Member

@meeh0w meeh0w left a comment

Choose a reason for hiding this comment

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

I also took it for a quick test and found filtering for Bridge in activity tab doesn't seem to work. Posted suggestions for fixes below :)

src/pages/Wallet/WalletRecentTxs.tsx Outdated Show resolved Hide resolved
src/pages/Wallet/WalletRecentTxs.tsx Outdated Show resolved Hide resolved
@meeh0w meeh0w merged commit d6b971d into main Sep 13, 2024
5 checks passed
@meeh0w meeh0w deleted the feat/CP-8368_historyservice branch September 13, 2024 17:33
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