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

fix and improve pda-sharing course #529

Closed
wants to merge 4 commits into from

Conversation

wuuer
Copy link
Contributor

@wuuer wuuer commented Sep 27, 2024

Problem

Magic number found on space calculation.
"token account" in the" Starter" section should refer to "associated token account"
"token account" in the "Add initialize_pool_secure instruction" section should refer to "associated token account"
Some typescript code is not synced to the project.

Summary of Changes

Use InitSpace to calculate space needed for accounts.
change "token account" in the "Starter" section to "associated token account"
change "token account" in the "Add `initialize_pool_secure" section to "associated token account"

Also, I made a PR for solana-pda-sharing starter branch and a PR for solana-pda-sharing solution branch
which must be synced with this PR.

Jeff Wood and others added 3 commits September 27, 2024 09:58
….InitSpace.html) to calculate space needed for accounts.

change "token account" in the "Starter" section to "associated token account"
change "token account" in the "Add `initialize_pool_secure" section to "associated token account"

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

@mikemaccana mikemaccana left a comment

Choose a reason for hiding this comment

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

Some good changes but please see the notes above re: CONTRIBUTING.md


In the example below, the `authority` of the `vault` account is a PDA derived
using the `mint` address stored on the `pool` account. This PDA is passed into
the instruction handler as the `authority` account to sign for the transfer of
tokens from the `vault` to the `withdraw_destination`.
the instruction as the `authority` account to sign for the transfer tokens from
Copy link
Contributor

Choose a reason for hiding this comment

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

See CONTRIBUTING.md re instruction handlers.

@@ -63,31 +63,29 @@ pub struct WithdrawTokens<'info> {
pool: Account<'info, TokenPool>,
vault: Account<'info, TokenAccount>,
withdraw_destination: Account<'info, TokenAccount>,
/// CHECK: This is the PDA that signs for the transfer
/// CHECK: PDA
Copy link
Contributor

Choose a reason for hiding this comment

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

/// CHECK is supposed to be a sentence explaining why you're not checking the account.

@@ -96,7 +94,7 @@ pub struct TokenPool {
One approach to create an account specific PDA is to use the
`withdraw_destination` as a seed to derive the PDA used as the authority of the
`vault` token account. This ensures the PDA signing for the CPI in the
`withdraw_tokens` instruction handler is derived using the intended
`withdraw_tokens` instruction is derived using the intended
Copy link
Contributor

Choose a reason for hiding this comment

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

See CONTRIBUTING.md again.

@@ -127,57 +125,54 @@ pub struct WithdrawTokens<'info> {
pool: Account<'info, TokenPool>,
vault: Account<'info, TokenAccount>,
withdraw_destination: Account<'info, TokenAccount>,
/// CHECK: This is the PDA that signs for the transfer
/// CHECK: PDA
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above re: /// CHECK

has_one = withdraw_destination,
seeds = [withdraw_destination.key().as_ref()],
bump = pool.bump,
)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Check your editor settings, pretty sure you're using tabs. Rustfmt uses 4 spaces.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. Remove stale label or comment or this will be closed in 7 days.

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

Successfully merging this pull request may close these issues.

None yet

2 participants