Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,7 @@ const INSTRUCTION_WITH_TWO_SIGNERS: InstructionData = {
name: 'testInstruction',
};

const EMPTY_INSTRUCTION: InstructionData = {
accounts: [],
args: [],
docs: [],
name: 'testInstruction',
};
const makeRef = (val?: string): { current: string | undefined } => ({ current: val });
Comment thread
C0mberry marked this conversation as resolved.

describe('createWalletPrefillDependency', () => {
it('should fill signer accounts with wallet address', () => {
Expand All @@ -83,9 +78,13 @@ describe('createWalletPrefillDependency', () => {
const { form, fieldNames } = result.current;

const walletPublicKey = PublicKey.default;
const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_SIGNER_AND_NON_SIGNER, walletPublicKey, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_SIGNER_AND_NON_SIGNER,
{
account: fieldNames.account,
},
makeRef(),
);

const walletAddress = walletPublicKey.toBase58();
dependency.onValueChange(walletPublicKey, form);
Expand All @@ -103,9 +102,13 @@ describe('createWalletPrefillDependency', () => {
);
const { form, fieldNames } = result.current;

const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_SIGNER, null, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_SIGNER,
{
account: fieldNames.account,
},
makeRef(),
);

const setValueSpy = vi.spyOn(form, 'setValue');
dependency.onValueChange(null, form);
Expand All @@ -123,26 +126,20 @@ describe('createWalletPrefillDependency', () => {
const { form, fieldNames } = result.current;

const walletPublicKey = PublicKey.default;
const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_NESTED_SIGNER, walletPublicKey, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_NESTED_SIGNER,
{
account: fieldNames.account,
},
makeRef(),
);

const walletAddress = walletPublicKey.toBase58();
dependency.onValueChange(walletPublicKey, form);

expect(form.getValues('accounts.testInstruction.group.nestedSigner')).toBe(walletAddress);
});

it('should return correct dependency id and getValue', () => {
const walletPublicKey = PublicKey.default;
const dependency = createWalletPrefillDependency(EMPTY_INSTRUCTION, walletPublicKey, {
account: () => 'accounts.testInstruction.test' as any,
});

expect(dependency.id).toBe('wallet');
expect(dependency.getValue()).toBe(walletPublicKey);
});

it('should ignore non-PublicKey values in onValueChange', () => {
const { result } = renderHook(() =>
useInstructionForm({
Expand All @@ -152,9 +149,13 @@ describe('createWalletPrefillDependency', () => {
);
const { form, fieldNames } = result.current;

const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_SIGNER, null, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_SIGNER,
{
account: fieldNames.account,
},
makeRef(),
);

const setValueSpy = vi.spyOn(form, 'setValue');
dependency.onValueChange('not-a-public-key', form);
Expand All @@ -176,15 +177,46 @@ describe('createWalletPrefillDependency', () => {
form.setValue('accounts.testInstruction.signer', PREFILLED_ADDRESS);

const walletPublicKey = PublicKey.default;
const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_SIGNER, walletPublicKey, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_SIGNER,
{
account: fieldNames.account,
},
makeRef(),
);

dependency.onValueChange(walletPublicKey, form);

expect(form.getValues('accounts.testInstruction.signer')).toBe(PREFILLED_ADDRESS);
});

it('should overwrite a signer field that still contains the previous wallet address', () => {
const { result } = renderHook(() =>
useInstructionForm({
instruction: INSTRUCTION_WITH_SIGNER,
onSubmit: vi.fn(),
}),
);
const { form, fieldNames } = result.current;

const walletAAddress = Keypair.generate().publicKey.toBase58();
const walletB = Keypair.generate().publicKey;

form.setValue('accounts.testInstruction.signer', walletAAddress);
const ref = makeRef(walletAAddress);

const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_SIGNER,
{
account: fieldNames.account,
},
ref,
);
dependency.onValueChange(walletB, form);

expect(form.getValues('accounts.testInstruction.signer')).toBe(walletB.toBase58());
});

it('should fill only empty signer fields when some are already filled', () => {
const { result } = renderHook(() =>
useInstructionForm({
Expand All @@ -198,9 +230,13 @@ describe('createWalletPrefillDependency', () => {

const walletPublicKey = PublicKey.default;
const walletAddress = walletPublicKey.toBase58();
const dependency = createWalletPrefillDependency(INSTRUCTION_WITH_TWO_SIGNERS, walletPublicKey, {
account: fieldNames.account,
});
const dependency = createWalletPrefillDependency(
INSTRUCTION_WITH_TWO_SIGNERS,
{
account: fieldNames.account,
},
makeRef(),
);

dependency.onValueChange(walletPublicKey, form);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import type { InstructionData } from '@entities/idl';
import { PublicKey } from '@solana/web3.js';
import type { MutableRefObject } from 'react';
import type { FieldPath, UseFormReturn } from 'react-hook-form';

import type { FormValue, InstructionFormData, InstructionFormFieldNames } from '../../use-instruction-form';
import { isPrefilledAccount, WALLET_ACCOUNT_PATTERNS } from '../const';
import type { ExternalDependency } from '../types';
import { traverseInstructionAccounts } from './traverse-accounts';

/**
Expand All @@ -13,9 +13,9 @@ import { traverseInstructionAccounts } from './traverse-accounts';
*/
export function createWalletPrefillDependency(
instruction: InstructionData,
publicKey: PublicKey | null,
fieldNames: Pick<InstructionFormFieldNames, 'account'>,
): ExternalDependency<PublicKey> {
lastPrefillAddressRef: MutableRefObject<string | undefined>,
): { onValueChange: (value: unknown, form: UseFormReturn<InstructionFormData>) => void } {
const signerPaths: FieldPath<InstructionFormData>[] = [];

traverseInstructionAccounts(instruction, (account, parentGroup) => {
Expand Down Expand Up @@ -43,22 +43,26 @@ export function createWalletPrefillDependency(
});

return {
getValue: () => publicKey,
id: 'wallet',
onValueChange: (value: unknown, form: UseFormReturn<InstructionFormData>) => {
if (!value || !(value instanceof PublicKey)) return;

const walletAddress = value.toBase58();
const lastPrefillAddress = lastPrefillAddressRef.current;

for (const path of signerPaths) {
const currentValue = form.getValues(path);
if (!currentValue || String(currentValue).trim() === '') {
const currentValue = String(form.getValues(path) ?? '').trim();
const isEmpty = currentValue === '';
const hasPreviousWallet = lastPrefillAddress !== undefined && currentValue === lastPrefillAddress;

if (isEmpty || hasPreviousWallet) {
form.setValue(path, walletAddress as unknown as FormValue, {
shouldDirty: false,
shouldValidate: false,
});
}
}

lastPrefillAddressRef.current = walletAddress;
},
};
}
11 changes: 9 additions & 2 deletions app/features/idl/interactive-idl/ui/InteractInstruction.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Button } from '@shared/ui/button';
import { Card } from '@shared/ui/card';
import { Tooltip, TooltipContent, TooltipTrigger } from '@shared/ui/tooltip';
import { useWallet } from '@solana/wallet-adapter-react';
import { useEffect, useRef } from 'react';
import { Loader, Send } from 'react-feather';
import { Control, Controller, FieldPath } from 'react-hook-form';

Expand Down Expand Up @@ -50,12 +51,18 @@ export function InteractInstruction({
const pdas = usePdas({ form, idl, instruction });
const getAutocompleteItems = createGetAutocompleteItems({ pdas, publicKey });

const walletPrefillDependency = createWalletPrefillDependency(instruction, publicKey, fieldNames);
const lastPrefillAddressRef = useRef<string | undefined>(undefined);
useEffect(() => {
createWalletPrefillDependency(instruction, fieldNames, lastPrefillAddressRef).onValueChange(publicKey, form);
// instruction and fieldNames are stable for the lifetime of this component instance
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [publicKey, form]);

const knownAccountsPrefillDependency = createKnownAccountsPrefillDependency(instruction, fieldNames);
const pdaPrefillDependency = createPdaPrefillDependency(idl, instruction, fieldNames);
useFormPrefill({
config: {
externalDependencies: [walletPrefillDependency, knownAccountsPrefillDependency, pdaPrefillDependency],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: There are some concerns with architecture and a simpler fix available.

The current approach lifts wallet prefill out of the useFormPrefill framework into a an ad-hoc useEffect + useRef in InteractInstruction.tsx, while pda and knownAccounts providers still live inside the framework. A few problems with this:

  1. Asymmetric pattern. Three sibling providers, two follow the ExternalDependency contract, one doesn't. A future maintainer adding a fourth provider has to guess which pattern to copy. The "stale prefill" problem isn't unique to wallet - it could apply to any provider, so the right fix is either fully inside the framework or fully outside, not split.

  2. Implementation detail leaks into the call site. lastPrefillAddressRef is created in the UI component and threaded into the provider as a parameter. The provider's internal bookkeeping should not appear in the component that renders accounts.

  3. Misleading eslint-disable. The comment claims fieldNames is stable, but useInstructionForm returns a fresh fieldNames object on every render (see use-instruction-form.ts:48). The closure captures the latest value, so it's harmless in practice - but the comment will mislead the next reader.

  4. Reinvents what react-hook-form already tracks. We're maintaining external state to answer "did the user type this value, or did we prefill it?" RHF already answers that via isDirty. Every prefill in this provider is called with shouldDirty: false, so:

    • prefilled fields → isDirty: false
    • user-typed fields → isDirty: true (RHF sets this automatically on onChange)

    The "match previous wallet address" check is a workaround for not using the signal RHF already gives us.

Suggested fix:

  1. Revert InteractInstruction.tsx to the symmetric form — drop the useRef, useEffect, and the eslint-disable. Wallet rejoins the externalDependencies array next to the other two:

    const walletPrefillDependency = createWalletPrefillDependency(instruction, publicKey, fieldNames);
    const knownAccountsPrefillDependency = createKnownAccountsPrefillDependency(instruction, fieldNames);
    const pdaPrefillDependency = createPdaPrefillDependency(idl, instruction, fieldNames);
    useFormPrefill({
        config: { externalDependencies: [walletPrefillDependency, knownAccountsPrefillDependency, pdaPrefillDependency] },
        form,
    });
  2. Restore id and getValue on createWalletPrefillDependency so it conforms to ExternalDependency<PublicKey>. Drop the lastPrefillAddressRef parameter — no external state needed.

  3. Replace the isEmpty || hasPreviousWallet check with a single dirty check inside onValueChange:

    for (const path of signerPaths) {
        if (form.getFieldState(path).isDirty) continue;
        form.setValue(path, walletAddress as unknown as FormValue, {
            shouldDirty: false,
            shouldValidate: false,
        });
    }

    Note: getFieldState reads internal RHF state directly, no formState subscription required.

  4. Keep shouldDirty: false on every setValue (already the case) — that's the load-bearing invariant that makes step 3 work.

externalDependencies: [knownAccountsPrefillDependency, pdaPrefillDependency],
},
form,
});
Expand Down
2 changes: 1 addition & 1 deletion bench/BUILD.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
| Dynamic | `/address/[address]/idl` | 130 kB | 1.27 MB |
| Dynamic | `/address/[address]/instructions` | 10 kB | 1.13 MB |
| Dynamic | `/address/[address]/metadata` | 10 kB | 1.03 MB |
| Dynamic | `/address/[address]/nftoken-collection-nfts` | 10 kB | 1.10 MB |
| Dynamic | `/address/[address]/nftoken-collection-nfts` | 10 kB | 1.08 MB |
| Dynamic | `/address/[address]/program-multisig` | 10 kB | 1.08 MB |
| Dynamic | `/address/[address]/rewards` | 10 kB | 1.02 MB |
| Dynamic | `/address/[address]/security` | 10 kB | 1.08 MB |
Expand Down
Loading