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

fix: solana balance on accounts selector #29054

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Conversation

aganglada
Copy link
Contributor

@aganglada aganglada commented Dec 10, 2024

Screenshot 2024-12-10 at 15 45 14

Description

Solana native balance weren't showing before

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

As of right now, manually testing is a bit complex, it needs to run the snap manually and the extension, since we 1st need to publish a new release to npm with more up to date work. The snap version we have in npm is outdated and won't support this flow. That said, if you want to go ahead and run locally the steps are the following:

  1. Clone the Solana Snap monorepo and run it locally with yarn and then yarn start
  2. In the extension, at this branch, apply the following changes and run the extension as flask:
At builds.yml add the solana feature to the flask build:
 
     features:
       - build-flask
       - keyring-snaps
+      - solana

At shared/lib/accounts/solana-wallet-snap.ts point the snap ID to the snap localhost:

-export const SOLANA_WALLET_SNAP_ID: SnapId = SolanaWalletSnap.snapId as SnapId;
+//export const SOLANA_WALLET_SNAP_ID: SnapId = SolanaWalletSnap.snapId as SnapId;
+export const SOLANA_WALLET_SNAP_ID: SnapId = "local:http://localhost:8080/";
  1. Manually install the snap via the snap dapp at http://localhost:3000
  2. Enable the Solana account via Settings > Experimental > Enable Solana account
  3. Create a Solana account from the account-list menu and see the account balance on it

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@aganglada aganglada self-assigned this Dec 10, 2024
@aganglada aganglada requested a review from a team as a code owner December 10, 2024 14:54
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-sol PRs from the Solana snap team label Dec 10, 2024
Copy link

socket-security bot commented Dec 10, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] network 0 639 kB metamaskbot

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

Copy link

socket-security bot commented Dec 10, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Network access npm/@metamask/[email protected] 🚫

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

Copy link
Contributor

@javiergarciavera javiergarciavera left a comment

Choose a reason for hiding this comment

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

Great!

@metamaskbot
Copy link
Collaborator

Builds ready [47143db]
Page Load Metrics (2071 ± 127 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34628431880545262
domContentLoaded169928112048259124
load171128412071265127
domInteractive257338136
backgroundConnect78419189
firstReactRender179824178
getState873691485426
initialActions00000
loadScripts126123581584260125
setupStore77513147
uiStartup200631372390275132
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

zone-live
zone-live previously approved these changes Dec 10, 2024
@aganglada aganglada dismissed stale reviews from zone-live and javiergarciavera via a9bdc68 December 10, 2024 17:54
@metamaskbot
Copy link
Collaborator

Builds ready [a9bdc68]
Page Load Metrics (1851 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16342221184914067
domContentLoaded16282156182913364
load16402214185113967
domInteractive23493173
backgroundConnect95724168
firstReactRender1590292512
getState792981486330
initialActions01000
loadScripts12061666140312058
setupStore714821
uiStartup185228452199245118
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

zone-live
zone-live previously approved these changes Dec 11, 2024
ccharly
ccharly previously approved these changes Dec 11, 2024
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

Was able to test:

Before

Screenshot 2024-12-11 at 16 22 30

After

Screenshot 2024-12-11 at 15 22 24

@aganglada aganglada enabled auto-merge December 11, 2024 16:34
@aganglada aganglada disabled auto-merge December 11, 2024 19:29
@aganglada aganglada dismissed stale reviews from ccharly and zone-live via a1a563d December 11, 2024 19:31
@aganglada aganglada force-pushed the fix/account-balance-solana branch from a9bdc68 to a1a563d Compare December 11, 2024 19:31
@metamaskbot
Copy link
Collaborator

Builds ready [a1a563d]
Page Load Metrics (1565 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1468174315676933
domContentLoaded1460173015406632
load1469174215656833
domInteractive246639157
backgroundConnect989262110
firstReactRender1599453316
getState45113147
initialActions01000
loadScripts1088126111614522
setupStore65111136
uiStartup16262077181815072
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@aganglada aganglada force-pushed the fix/account-balance-solana branch from a1a563d to c4dd6ec Compare December 11, 2024 21:34
@metamaskbot
Copy link
Collaborator

Builds ready [c4dd6ec]
Page Load Metrics (1625 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1468184416319746
domContentLoaded1428176016059144
load1471178416258943
domInteractive236639167
backgroundConnect85923168
firstReactRender1798472613
getState56412168
initialActions00000
loadScripts1009133511838842
setupStore65310105
uiStartup16422203183612359
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@aganglada aganglada added this pull request to the merge queue Dec 12, 2024
Merged via the queue into main with commit 714fa10 Dec 12, 2024
76 of 77 checks passed
@aganglada aganglada deleted the fix/account-balance-solana branch December 12, 2024 11:26
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
@metamaskbot metamaskbot added the release-12.10.1 Issue or pull request that will be included in release 12.10.1 label Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.10.1 Issue or pull request that will be included in release 12.10.1 team-sol PRs from the Solana snap team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants