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

lang: Fix declare_program! using non-instruction composite accounts multiple times #3350

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tomjohn1028
Copy link
Contributor

Problem

Described in #3349

Summary of changes

Use a HashSet to enforce unique naming for account definitions

Resolves #3349

Copy link

vercel bot commented Nov 8, 2024

@tomjohn1028 is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@tomjohn1028 tomjohn1028 marked this pull request as ready for review November 8, 2024 20:22
@acheroncrypto acheroncrypto added lang fix Bug fix PR labels Nov 9, 2024
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, but I think the solution is insufficient, because we're deriving the name of the accounts struct from the name of the account field. So in the example you've added:

#[derive(Accounts)]
pub struct SecondUseOfNonInstructionComposite<'info> {
    pub non_instruction_update: NonInstructionUpdate<'info>,
}

This works, but if the accounts were different with the same identifier non_instruction_update:

#[derive(Accounts)]
pub struct SecondUseOfNonInstructionComposite<'info> {
    pub non_instruction_update: DifferentCompositeAccount<'info>,
}

In this case, it would use the incorrect NonInstructionUpdate rather than DifferentCompositeAccount. We can't know the name DifferentCompositeAccount (due to this information not being stored in the IDL), so we'd probably need to add some sort of suffix to non_instruction_update to indicate this is a different set of composite accounts.

@@ -138,17 +140,25 @@ fn gen_internal_accounts_common(
.iter()
.flat_map(|ix| ix.accounts.to_owned())
.collect::<Vec<_>>();
let mut account_names: HashSet<String> = std::collections::HashSet::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The get_non_instruction_composite_accounts should ideally return a HashSet rather than a Vec, but the IDL types don't implement the necessary traits currently. I guess I thought this check would make them unique, but it doesn't cover the initial outer case.

I think it would be better for the get_non_instruction_composite_accounts to return unique accounts rather than sorting them out here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PR lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

declare_program! panics when program has a shared Accounts context
2 participants