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

feature gate: add activate instruction #5541

Closed

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Oct 16, 2023

This PR adds the ActivateFeature instruction to the Feature Gate program.

This instruction is the activation process via feature keypair that we are familiar with today.

The next PR in this stack will address this comment on #5510 with an instruction designed for multi-sig support.

Copy link
Contributor Author

buffalojoec commented Oct 16, 2023

@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 3d85831 to cb72e2a Compare October 16, 2023 08:38
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch 3 times, most recently from af10219 to b77b5ca Compare October 16, 2023 11:10
@CriesofCarrots
Copy link
Contributor

If possible, I think we should continue to support the current process, where a single signer is the feature account, so that we can upgrade to this version of the program without making engs immediately change process to having to derive an address to put in the feature_set list. Make authority optional and only derive the address when Some?

@buffalojoec
Copy link
Contributor Author

If possible, I think we should continue to support the current process, where a single signer is the feature account, so that we can upgrade to this version of the program without making engs immediately change process to having to derive an address to put in the feature_set list. Make authority optional and only derive the address when Some?

Yeah that's totally possible!

I'm going to re-write this PR to be the activate instruction as we currently know it, so nothing changes for contributors. Then I'll introduce another with the multi-sig support and we can check them both out in tandem.

@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from cb72e2a to e4afcb4 Compare October 17, 2023 10:25
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch from b77b5ca to af0ff1b Compare October 17, 2023 10:25
@buffalojoec buffalojoec marked this pull request as ready for review October 17, 2023 10:25
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from e4afcb4 to 5e3ee34 Compare October 17, 2023 11:10
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch from af0ff1b to 99faeff Compare October 17, 2023 11:10
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 5e3ee34 to f202fdb Compare October 18, 2023 08:09
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch from 99faeff to e1481be Compare October 18, 2023 08:18
Base automatically changed from 10-11-feature_gate_init_program to master October 18, 2023 15:49
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch from e1481be to ed56847 Compare October 18, 2023 20:54
feature-gate/program/src/instruction.rs Outdated Show resolved Hide resolved
feature-gate/README.md Outdated Show resolved Hide resolved
) -> [Instruction; 2] {
let lamports = Rent::default().minimum_balance(Feature::size_of());
[
system_instruction::transfer(payer, feature_id, lamports),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move transfer handling into process_activate_feature(). That way we can use the Rent syscall to be sure we're using correct rates. Rent::default() may not always be accurate.
Would be something like what we do in spl-assocated-token:

let required_lamports = rent
.minimum_balance(space)
.max(1)
.saturating_sub(new_pda_account.lamports());
if required_lamports > 0 {
invoke(
&system_instruction::transfer(payer.key, new_pda_account.key, required_lamports),
&[
payer.clone(),
new_pda_account.clone(),
system_program.clone(),
],
)?;
}

Unless there's a reason I'm missing that the transfer needs to be separate?

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 was following Token2022's example with how account initialization is handled, specifically initializing token metadata:

pub async fn token_metadata_initialize<S: Signers>(
&self,
update_authority: &Pubkey,
mint_authority: &Pubkey,
name: String,
symbol: String,
uri: String,
signing_keypairs: &S,
) -> TokenResult<T::Output> {
self.process_ixs(
&[spl_token_metadata_interface::instruction::initialize(
&self.program_id,
&self.pubkey,
update_authority,
&self.pubkey,
mint_authority,
name,
symbol,
uri,
)],
signing_keypairs,
)
.await
}

pub async fn token_metadata_initialize_with_rent_transfer<S: Signers>(
&self,
payer: &Pubkey,
update_authority: &Pubkey,
mint_authority: &Pubkey,
name: String,
symbol: String,
uri: String,
signing_keypairs: &S,
) -> TokenResult<T::Output> {
let token_metadata = TokenMetadata {
name,
symbol,
uri,
..Default::default()
};
let additional_lamports = self
.get_additional_rent_for_new_metadata(&token_metadata)
.await?;
let mut instructions = vec![];
if additional_lamports > 0 {
instructions.push(system_instruction::transfer(
payer,
&self.pubkey,
additional_lamports,
));
}
instructions.push(spl_token_metadata_interface::instruction::initialize(
&self.program_id,
&self.pubkey,
update_authority,
&self.pubkey,
mint_authority,
token_metadata.name,
token_metadata.symbol,
token_metadata.uri,
));
self.process_ixs(&instructions, signing_keypairs).await
}

But in hindsight I'm thinking that might be the process for some of the Token2022 stuff since the account in question may or may not exist prior to the instruction (?).

Considering these feature accounts will not exist prior to sending the instruction, and our CLI command solana feature activate will always use the "with rent" one, I think it makes sense to combine in the program's processor, yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this also requires a payer though

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 a commit for this. If we decide we don't like it I can always pluck it!

feature-gate/program/src/processor.rs Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the 10-15-feature_gate_add_activate_instruction branch from d1ba993 to 146f1c1 Compare October 19, 2023 09:45
@buffalojoec buffalojoec added the do-not-close Add this tag to exempt a PR / issue from being closed automatically label Oct 29, 2023
@buffalojoec
Copy link
Contributor Author

Closing for now, since whenever SIMD talks conclude, we'll want to upgrade the program one feature at a time, starting with the existing RevokePendingActivation instruction.

@buffalojoec buffalojoec deleted the 10-15-feature_gate_add_activate_instruction branch February 23, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-close Add this tag to exempt a PR / issue from being closed automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants