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: Add ProgramData account #1095

Merged

Conversation

paul-schaaf
Copy link
Contributor

closes #1083

@paul-schaaf paul-schaaf marked this pull request as draft December 3, 2021 02:21
@paul-schaaf paul-schaaf marked this pull request as ready for review December 3, 2021 06:13
}
// Easy to implement. Just need to write a test.
// Feel free to open a PR.
assert!(!state.is_zero_copy, "Trait implementations not yet implemented for zero copy state structs. Please file an issue.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy asked for this

@@ -0,0 +1 @@
[114,99,192,17,48,208,90,184,231,46,220,91,47,115,132,253,218,163,228,101,8,121,220,138,41,140,176,127,254,91,51,28,176,244,174,182,223,57,57,125,117,201,31,213,9,39,207,212,100,173,88,252,61,235,89,156,53,86,4,90,16,251,191,219]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this exists so it can be copied into the target/deploy folder. see the github actions job test-program-data

}

#[derive(Debug, Clone)]
pub struct ProgramData<'info> {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making a wrapper type for UpgradeableLoaderState that implements the required traits? Then we could use Account<UpgradeableLoaderState> vs ProgramData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sg,Ill try.

although I'd prefer to have an Account<ProgramData> and an Account<Program> instead of Account<UpgradeableLoaderState>. or all three of them. cause the first two are useful for constraints. You can't match easily on an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm actually idk if Account<Program> is a good idea. We already have Program<'info, PROGRAM> which also checks that we have the correct program. We could just add the data of the program (=programdata_address) to that struct as an option. It would be Some(..) if the given program is owned by the upgradable loader.

Account<ProgramData> and Account<UpgradableLoaderState> still sg to me tho

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've added ProgramData and UgpradableLoaderState and they work, just gotta decide what to do with Program

Copy link
Member

Choose a reason for hiding this comment

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

Let's add Option<Pubkey> to Program<'info, T> for the programdata_address. I'm not a fan of Account<Program> since there are no address checks done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, agree.

can merge this pr and do that in a separate issue tho. theyre not connected.

Copy link
Contributor Author

@paul-schaaf paul-schaaf Dec 4, 2021

Choose a reason for hiding this comment

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

already created it some days ago #1094

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@paul-schaaf paul-schaaf marked this pull request as draft December 4, 2021 21:38
@@ -0,0 +1,5 @@
.anchor
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this file.

Copy link
Contributor Author

@paul-schaaf paul-schaaf Dec 5, 2021

Choose a reason for hiding this comment

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

I removed it but added it back so it can gitignore yarn.lock in the test repo.

Copy link
Member

@armaniferrante armaniferrante left a comment

Choose a reason for hiding this comment

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

Edit. Still need to think about the Program type.


declare_id!("Cum9tTyj5HwcEiAmhgaS7Bbj4UczCwsucrCkxRECzM4e");

// TODO: Once anchor can deserialize program data, update this test.
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we should have a test that checks that you can uses settings, program, program_data, and the upgrade authority together to set data. this use case was what got these issues created.

eventually we should have a test that can check the following

#[derive(Accounts)]
#[instruction(admin_data: u64)]
pub struct SetAdmin {
  upgrade_authority_address: Signer<'info>,
  #[account(has_one = programdata_address)
  program: Program<'info, <YOUR_PROGRAM>,
  #[account(has_opt = upgrade_authority_address)]
  programdata_address: ProgramData<'info>
}

(pending has_opt feature approval)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

program data in that comment was referring to the data of the Program (i.e. programdata_address), not the data of ProgramData.

@paul-schaaf paul-schaaf marked this pull request as ready for review December 5, 2021 01:56
@armaniferrante armaniferrante merged commit 3321a3f into coral-xyz:master Dec 5, 2021
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.

lang: Add ProgramData account
2 participants