refactor: Decompose simulation, add tests#928
refactor: Decompose simulation, add tests#928askov wants to merge 7 commits intosolana-foundation:masterfrom
Conversation
|
@askov is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR decomposes the monolithic Confidence Score: 5/5This PR is safe to merge — all previously-flagged correctness issues are resolved and no new P0/P1 issues were found. Both prior blocking concerns (ALT-resolved accounts silently dropped, cache key missing ALT configuration) are definitively fixed. The new resolveAddressLookupTables also silently fixed a key-index alignment bug from the old filter-then-map pattern, and the test suite is comprehensive across all extracted units. No remaining findings rise above P2. No files require special attention.
|
| Filename | Overview |
|---|---|
| app/features/instruction-simulation/lib/simulate-transaction.ts | New pure async function orchestrating RPC calls and result interpretation; now correctly uses keySegments().flat() to include ALT-resolved accounts, fixing the previously-flagged silent data loss. |
| app/features/instruction-simulation/model/use-simulation.ts | Refactored to use useSWRMutation; fingerprint now serializes the full message (fixing the ALT cache-key collision); clean discriminated-union state type replaces scattered useState calls. |
| app/features/instruction-simulation/lib/build-token-balances.ts | Extracted token balance builder; uses readU64LE returning bigint instead of Number(...), fixing precision loss for amounts > Number.MAX_SAFE_INTEGER. Token-2022 discriminator byte correctly handled. |
| app/features/instruction-simulation/lib/resolve-address-lookup-tables.ts | New helper; also fixes a subtle index-alignment bug from the old code where filtering null accounts before mapping caused key mismatches for missing ALTs. |
| app/features/instruction-simulation/ui/SimulationCard.tsx | Cleanly decomposed into distinct status branches using SimulationCardShell; introduces generateTokenBalanceRows at the UI layer where it belongs. Token balance and SOL balance cards are correctly gated on succeeded. |
| app/features/instruction-simulation/lib/tests/simulate-transaction.spec.ts | 456-line test suite covering happy path, error branches, ALT resolution, token balance parsing, and large-amount precision. |
| app/entities/compute-unit/index.ts | New barrel export for the compute-unit FSD entity; clean public API surface. |
| app/features/instruction-simulation/lib/get-mint-decimals.ts | Correctly distinguishes mint vs token-account buffers for both base spl-token and Token-2022 extended accounts using the type discriminator at offset 165. |
Reviews (3): Last reviewed commit: "fix: show simulation error when RPC retu..." | Re-trigger Greptile
|
@greptile-apps please review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptile-apps please review |
6dfde53 to
eab0de7
Compare
|
|
||
| const KEY_A = new PublicKey('5ZiE3vAkrdXBgyFL7KqG3RoEGBws8CjY8AsGq1MuR5My'); | ||
| const KEY_B = new PublicKey('EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v'); | ||
| const KEY_C = new PublicKey('So11111111111111111111111111111111111111112'); |
There was a problem hiding this comment.
nit: I'd prefer using Keypair.generate().pubkey here to create a key if we just need a key. Otherwise, it seems that we need a specific key
There was a problem hiding this comment.
Replaced all the hardcoded keys where possible
| const USDC_MINT = 'EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v'; | ||
|
|
||
| /** Real ALT account data from mainnet. Addresses: ADDR_3ZK, ADDR_SQDS, ADDR_BGUMA */ | ||
| const ALT_FIXTURE_3_ADDRS = |
There was a problem hiding this comment.
I'd move these into app/fixtures directory as they are for real ALTs
|
|
||
| import { MINT_SIZE, TOKEN_ACCOUNT_SIZE } from '../lib/token-layout'; | ||
|
|
||
| export const TOKEN_PROGRAM = 'TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA'; |
There was a problem hiding this comment.
For token programs, it is better to use solana-program/token & token2022
| export const SYSTEM_PROGRAM = '11111111111111111111111111111111'; | ||
|
|
||
| export const USDC_MINT = new PublicKey('EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v'); | ||
| export const WSOL_MINT = new PublicKey('So11111111111111111111111111111111111111112'); |
There was a problem hiding this comment.
WSOL is present at the project as NATIVE_MINT. USDC_MINT might be moved to app/__fixtures or __mocks
There was a problem hiding this comment.
Replaced NATIVE_MINT and moved USDC_MINT to shared, as it's not a fixture, and we have several of them hardcoded in different files.
|
|
||
| /** String comparison for RPC responses where owner is a base58 string, not a PublicKey */ | ||
| export function isTokenProgramBase58(owner: string): boolean { | ||
| return owner === TOKEN_PROGRAM_BASE58 || owner === TOKEN_2022_PROGRAM_BASE58; |
There was a problem hiding this comment.
We have a similar helper at app/shared/model/token-program.ts
There was a problem hiding this comment.
Moved the helper to the shared model
| export const MIN_MINT_ACCOUNT_BUFFER_LENGTH = MINT_SIZE; | ||
|
|
||
| /** Base SPL token account size (bytes) */ | ||
| export const TOKEN_ACCOUNT_SIZE = 165; |
There was a problem hiding this comment.
@solana-program/token client has methods to get sizes for mint size and TA size
| * Token-2022 account type discriminator values. | ||
| * The discriminator byte sits at offset `TOKEN_ACCOUNT_SIZE` (165). | ||
| */ | ||
| export const ACCOUNT_TYPE_MINT = 1; |
There was a problem hiding this comment.
solana-program/token also has account types in it
There was a problem hiding this comment.
I mean that we could reuse already existing logic within the client to make better refactoring
There was a problem hiding this comment.
Imported everything I could find
| import type { SimulationResult } from '../../lib/simulate-transaction'; | ||
| import type { SimulationState } from '../use-simulation'; | ||
|
|
||
| const MOCK_URL = 'https://api.devnet.solana.com'; |
There was a problem hiding this comment.
Let's use 'https://devnet.rpc.address', maybe, to distinguish it from a real address unless the types require it to look like a devnet address
| expect(typeof getSimulate(result.current)).toBe('function'); | ||
| }); | ||
|
|
||
| it('should transition to done with result on success', async () => { |
There was a problem hiding this comment.
I may be missing something, but it looks like we need to construct some real transaction data (Message), which would have tolen and sol changes to test that buffer layouts work
There was a problem hiding this comment.
This is already covered by unit tests
eab0de7 to
c86a616
Compare
Description
compute-unitFSD entity — moves CU schedule logic, default CU values, instruction log formatting, and theCUProfilingCardcomponent out ofutils/andfeatures/cu-profiling/intoentities/compute-unit/, with a clean public API via barrel exportuseSimulationhook — breaks the monolithic 300+ line hook into focused, pure library functions (simulateTransaction,buildTokenBalances,computeSolBalanceChanges,getMintDecimals,resolveAddressLookupTables) that are independently testable and easier to reason aboutuseSimulationhook andSolBalanceChangesCardStorybook storiesType of change
Testing
Regression targets
Related Issues
Closes HOO-410
Checklist
build:infoscript to update build information