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

Add functions to narrow instructions to IInstructionWithAccounts and IInstructionWithData #2212

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

mcintyre94
Copy link
Collaborator

Our IInstruction type may not include accounts/data. When we serialize a transaction we only know that it has instructions: IInstruction[]

The generated Kinobi clients sensibly require data to identify eg a system instruction:

export function identifySplSystemInstruction(
  instruction: { data: Uint8Array } | Uint8Array
)

And accounts and data to parse one:

export function parseTransferSolInstruction<
  TProgram extends string,
  TAccountMetas extends readonly IAccountMeta[]
>(
  instruction: IInstruction<TProgram> &
    IInstructionWithAccounts<TAccountMetas> &
    IInstructionWithData<Uint8Array>
)

This PR adds functions to narrow the type of an IInstruction to IInstruction & IInstructionWithAccounts and IInstruction & IInstructionWithData

This will allow code using generated clients to do eg:

// instruction: IInstruction
if(isInstructionWithAccounts(instruction) && isInstructionWithData(instruction)) {
  const identified = identifySplSystemInstruction(instruction);
  const parsed = parseTransferSolInstruction(instruction)
}

Notes:

  • I've added new coded errors, please LMK if I did that wrong!
  • I tweaked the error message formatter to handle null/undefined as context values, since it's useful for a message identifying the instruction at fault to use these.

Copy link
Collaborator

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

A few nits but I think this is valuable.

Comment on lines 28 to 31
[SOLANA_ERROR__EXPECTED_INSTRUCTION_TO_HAVE_ACCOUNTS]:
'The instruction with program address `$programAddress` and data `$data` does not have any accounts.',
[SOLANA_ERROR__EXPECTED_INSTRUCTION_TO_HAVE_DATA]:
'The instruction with program address `$programAddress` and accounts `$accountAddresses` does not have any data.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we don't know for sure that an account without data has accounts and vice versa, I wouldn't explicitly add them in the error message. I think it's fine to have in the context if someone wants to check their values but would be confusing in the error message for instruction that have neither.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that's what I was missing, I thought context was just about getting values into the error message but it's displayed separately too. Agreed, I'll remove the accounts/data from the messages and just include them in context.

packages/errors/src/codes.ts Outdated Show resolved Hide resolved
@mcintyre94 mcintyre94 force-pushed the instruction-narrow branch 2 times, most recently from 13f8a02 to 3607732 Compare February 29, 2024 18:38
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

giphy

Comment on lines +97 to +122
it('interpolates a Uint8Array variable into a error message format string', () => {
const messagesSpy = jest.spyOn(MessagesModule, 'SolanaErrorMessages', 'get');
messagesSpy.mockReturnValue({
// @ts-expect-error Mock error config doesn't conform to exported config.
123: 'Here is some data: $data',
});
const message = getErrorMessage(
// @ts-expect-error Mock error context doesn't conform to exported context.
123,
{ data: new Uint8Array([1, 2, 3, 4]) },
);
expect(message).toBe('Here is some data: 1,2,3,4');
});
it('interpolates an undefined variable into a error message format string', () => {
const messagesSpy = jest.spyOn(MessagesModule, 'SolanaErrorMessages', 'get');
messagesSpy.mockReturnValue({
// @ts-expect-error Mock error config doesn't conform to exported config.
123: 'Here is a variable: $variable',
});
const message = getErrorMessage(
// @ts-expect-error Mock error context doesn't conform to exported context.
123,
{ variable: undefined },
);
expect(message).toBe('Here is a variable: undefined');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

borat-borat-very-nice

@mcintyre94 mcintyre94 merged commit 07c30c1 into master Mar 1, 2024
6 checks passed
@mcintyre94 mcintyre94 deleted the instruction-narrow branch March 1, 2024 11:41
Copy link
Contributor

github-actions bot commented Mar 2, 2024

🎉 This PR is included in version 1.90.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants