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(llm,lld): improve Solana scan accounts performances #9512

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

francois-guerin-ledger
Copy link
Contributor

@francois-guerin-ledger francois-guerin-ledger commented Mar 13, 2025

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

The scan account operation for Solana is particularly slow in Ledger Live. One reason is that quite a lot of HTTP requests are done sequentially to list token and staking accounts, as well as fetching all the transactions.
This PR focuses on re-writing this part, grouping RPC requests together or running them asynchronously when possible.

❓ Context

  • JIRA or GitHub link:

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Mar 13, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Mar 21, 2025 5:27pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 5:27pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 5:27pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 5:27pm

@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from 4bc6d0d to db18121 Compare March 13, 2025 08:57
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from db18121 to 6fe69e6 Compare March 13, 2025 09:12
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from 6fe69e6 to a6fa77f Compare March 13, 2025 10:12
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from a6fa77f to 2b2b3ac Compare March 13, 2025 12:01
@francois-guerin-ledger francois-guerin-ledger changed the base branch from develop to bugfix/LIVE-17003 March 13, 2025 12:01
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from 2b2b3ac to d34c102 Compare March 13, 2025 22:36
@live-github-bot live-github-bot bot added desktop Has changes in LLD ui Has changes in the design system library translations Translation files have been touched labels Mar 13, 2025
@francois-guerin-ledger francois-guerin-ledger changed the base branch from bugfix/LIVE-17003 to develop March 13, 2025 22:59
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from d34c102 to bed6385 Compare March 14, 2025 08:35
@live-github-bot live-github-bot bot removed desktop Has changes in LLD ui Has changes in the design system library translations Translation files have been touched labels Mar 14, 2025
@francois-guerin-ledger francois-guerin-ledger force-pushed the fix/LIVE-17491-improve-solana-performances branch from bed6385 to 9cf2238 Compare March 14, 2025 09:06
@francois-guerin-ledger francois-guerin-ledger marked this pull request as ready for review March 14, 2025 09:06
@francois-guerin-ledger francois-guerin-ledger requested a review from a team as a code owner March 14, 2025 09:06
);
});

return compact(stakes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could do this without lodash but I don't really mind using it here

Suggested change
return compact(stakes);
return stakes.filter(Boolean);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't work as is, since Boolean is no predicate. Better keeping it to avoid extra boilerplate in my opinion.

Comment on lines +87 to +111
const rawStakeAccounts = await api.getStakeAccountsByWithdrawAuth(mainAccountAddress);

if (!rawStakeAccounts.length) return [];

const sysvarStakeHistoryAccount = await api.getAccountInfo(
SYSVAR_STAKE_HISTORY_PUBKEY.toBase58(),
);
if (
!sysvarStakeHistoryAccount ||
!("parsed" in sysvarStakeHistoryAccount.data) ||
!("info" in sysvarStakeHistoryAccount.data.parsed)
)
throw new Error("StakeHistory not found");

const sysvarStakeHistoryAccountInfo = sysvarStakeHistoryAccount.data.parsed.info;
const stakeHistory = Array.isArray(sysvarStakeHistoryAccountInfo)
? sysvarStakeHistoryAccountInfo.filter(isHistoryEntry).map(e => ({
epoch: BigNumber(e.epoch),
effective: BigNumber(e.stakeHistory.effective),
activating: BigNumber(e.stakeHistory.activating),
deactivating: BigNumber(e.stakeHistory.deactivating),
}))
: [];

const { epoch } = await api.getEpochInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably keep the promise.all on the 3 calls getStakeAccountsByWithdrawAuth, getAccountInfo and getEpochInfo as it was done before, wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually didn't do it in order to save useless calls to the network (if getStakeAccountsByWithdrawAuth does not return any stake account, no need to call neither getAccountInfo nor getEpochInfo).

Comment on lines +239 to +238
const nextSubAccs: TokenAccount[] = await Promise.all(
accountsWithTransactions.map(account => {
Copy link
Contributor

@Justkant Justkant Mar 17, 2025

Choose a reason for hiding this comment

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

Nice, looks like you're already doing the parallel calls we talked about today for the sub accounts
The only drawback maybe here is that we don't limit the number of concurrent promises

Comment on lines 66 to 78
const {
value: balanceValue,
context: { slot: blockHeight },
} = await api.getBalanceAndContext(address);
const balance = new BigNumber(balanceValue);

const tokenAccountsRaw = (await api.getParsedTokenAccountsByOwner(address)).value;
const token2022AccountsRaw = coinConfig.getCoinConfig().token2022Enabled
? (await api.getParsedToken2022AccountsByOwner(address)).value
: [];
const tokenAccounts = [...tokenAccountsRaw, ...token2022AccountsRaw].map(toTokenAccountWithInfo);

const stakes = await getStakeAccounts(api, address);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also probably do those calls in parallel too

descriptors: Array<TransactionDescriptor>;
}>
> {
// TODO: before merging, rebase this PR on https://github.com/LedgerHQ/ledger-live/pull/9480 once CI is green, and use `ky` instead of `fetch`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want to wait for the other PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just merged the other PR πŸ™‚

Comment on lines 230 to 236
.map(account =>
account && "parsed" in account.data ? tryParseAsMintAccount(account.data) : undefined,
)
.map(mintInfo => (mintInfo && !(mintInfo instanceof Error) ? mintInfo.extensions : undefined));
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably do a single map instead of 2 here, wdyt ?

Justkant
Justkant previously approved these changes Mar 21, 2025
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required β‰₯ 80%)
D Reliability Rating on New Code (required β‰₯ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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.

2 participants