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: use evm and avalanche modules to load balances CP-9002 #43

Merged
merged 12 commits into from
Sep 12, 2024

Conversation

gergelylovas
Copy link
Contributor

@gergelylovas gergelylovas commented Sep 10, 2024

Description

Refactor Core Extension to use the vm module system for all balance loading

VM modules PR to add NFT loading: https://github.com/ava-labs/vm-modules/pull/153

Changes

  • Update types to be used from the vm-modules
  • Add EVM and Avalanche modules
  • Adjust NFT handling to work with vm-modules

Testing

  • Balances are loaded correctly
  • Send, swap, transaction approval, onboarding, total ballances, nfts are working correctly. Essentially a full regression is needed.

Screenshots:

No ui changed.

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.

@gergelylovas gergelylovas changed the title feat: use evm and avalanche modules to load balances feat: use evm and avalanche modules to load balances CP-9002 Sep 10, 2024
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 did a sanity check, comparing returned balances between this branch & current production build - it's mostly consistent.

I found a bug with Beam (which probably also relates to other networks that go through DebankService in the EvmModule).

Quick investigation tells me it's caused by the usage of Zodios in the VM Modules, which internally uses axios (which does not work in extension land). I see this error when debugging (place a conditional breakpoint in BalancesService.ts:62 with network.chainId === 4337 condition)

"getNativeBalance failed: AxiosError: There is no suitable adapter to dispatch the request since :
- adapter xhr is not supported by the environment
- adapter http is not available in the build"
beam.bam.boom.mov

Other than that, it's working great (even better than prod build when it comes to NFTs, I just noticed).

I left some additional comments, mostly minor / nitpicks.

patches/jest-runner+29.7.0.patch Show resolved Hide resolved
src/background/services/balances/models.ts Outdated Show resolved Hide resolved
src/contexts/BalancesProvider.tsx Outdated Show resolved Hide resolved
src/hooks/useDisplayTokenList.ts Outdated Show resolved Hide resolved
Comment on lines +83 to +86
const funds =
firstAsset && 'decimals' in firstAsset
? normalizeBalance(firstAsset.balance, firstAsset.decimals) ?? new Big(0)
: new Big(0);
Copy link
Member

@meeh0w meeh0w Sep 11, 2024

Choose a reason for hiding this comment

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

nitpick: I believe this is the same typing issue as in one of the previous comments, where ActiveNetworkWidgetProps typings allow for NFTs to be passed, but we never actually (intent to) do it...so maybe we could exclude NftTokenWithBalance from assetList prop :)

src/utils/bigintToLocaleString.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@vvava vvava left a comment

Choose a reason for hiding this comment

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

send swap balances onboarding etc seem working well 🐤

@gergelylovas gergelylovas merged commit d300dc5 into main Sep 12, 2024
5 checks passed
@gergelylovas gergelylovas deleted the feat/integrate-evm-module branch September 12, 2024 10:57
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