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

[stake-pool] instruction to add metadata for pool token #3335

Merged
merged 5 commits into from
Jul 22, 2022

Conversation

betterclever
Copy link
Contributor

@betterclever betterclever commented Jul 12, 2022

Problem

The ecosystem has abandoned the token-list and encourages using the Metaplex Metadata standard for tokens. This requires a signature from the mint authority, which means that it's currently impossible to create token metadata for stake pools.

Solution

Add new create_metadata and update_metadata instructions to the stake pool program to allow working with metaplex metadata accounts. This provides a signature from the stake pool withdraw authority, which is the mint authority. The instruction must be signed by the stake pool manager.

@mergify mergify bot added the community Community contribution label Jul 12, 2022
@joncinque joncinque self-requested a review July 12, 2022 20:04
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Lots of really great work! This is absolutely on the right track, and I'm looking forward to integrating this soon.

stake-pool/program/src/instruction.rs Outdated Show resolved Hide resolved
stake-pool/program/src/instruction.rs Outdated Show resolved Hide resolved
/// 1. `[]` Manager
/// 2. `[]` Stake pool withdraw authority
/// 3. `[]` Pool token mint account
/// 4. `[]` Payer for creation of token metadata account
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since it's paying, it needs to be writable and signer:

Suggested change
/// 4. `[]` Payer for creation of token metadata account
/// 4. `[s, w]` Payer for creation of token metadata account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests pass locally without making payer a writable AccountMeta though. What could be reason behind it?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question. It may be because the specified payer is the same as the fee payer in your test transactions. You can try a test with a different fee payer and see what happens! I'm surprised that it lets this through though 🤔

stake-pool/program/src/instruction.rs Outdated Show resolved Hide resolved
stake-pool/program/src/instruction.rs Outdated Show resolved Hide resolved
stake-pool/program/tests/helpers/mod.rs Outdated Show resolved Hide resolved
stake-pool/program/tests/helpers/mod.rs Outdated Show resolved Hide resolved
stake-pool/program/tests/create_pool_token_metadata.rs Outdated Show resolved Hide resolved
@betterclever
Copy link
Contributor Author

@joncinque I have addressed all the comments highlighted above. Please re-review whenever possible :D

@joncinque
Copy link
Contributor

This looks good to me, you'll need to run cargo fmt on stake-pool/program to pass CI, and run clippy to make sure everything's ok there. Let's wait to see if any of the other stake pool devs want to take a look too

assert_eq!(metadata.data.name.to_string(), puffed_name);
assert_eq!(metadata.data.symbol.to_string(), puffed_symbol);
assert_eq!(metadata.data.uri.to_string(), puffed_uri);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I'm just noticing at the end of this pass, but can you also add a test to make sure that the correct manager must sign? Same as with fail_manager_did_not_sign but for this instruction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor

@CMEONE CMEONE left a comment

Choose a reason for hiding this comment

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

I just skimmed through the changes, and it looks all good to me (other than adding those tests and resolving the CI as @joncinque has mentioned). I'm looking forward to seeing this get merged soon!

joncinque
joncinque previously approved these changes Jul 18, 2022
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great! I'll give one more day for any external reviews, and merge tomorrow

@joncinque
Copy link
Contributor

@betterclever can you look at the CI test failure and fix the issues? Looks like an unused import that needs to be removed

@mergify mergify bot dismissed joncinque’s stale review July 18, 2022 15:48

Pull request has been modified.

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Sorry for the late addition, but with this I'll feel a little more comfortable and ready to merge!

stake-pool/program/src/processor.rs Show resolved Hide resolved
@joncinque
Copy link
Contributor

Thanks so much for all of your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants