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 signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md & closing-accounts.md & arbitrary-cpi.md #372

Closed
wants to merge 23 commits into from

Conversation

wuuer
Copy link
Contributor

@wuuer wuuer commented Aug 28, 2024

Problem

signer-auth.md

The way to get the seed "*ctx.bumps.get("vault").unwrap()" is outdated for the latest anchor version.
Unnecessary parameters in "accounts()" in the test typescript.

reinitialization-attacks.md

Magic number found on space calculation
“@project-serum/anchor” is outdated.
The two recommendedInitialization test cases do not use the same method to send transactions.

pda-sharing.md

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.

owner-checks.md

Magic number found on space calculation.
Unnecessary parameters are found in the test typescript.
Two Different ways to send transactions are found.

duplicate-mutable-accounts.md

Magic number found on space calculation.
"BorshDeserialize" and "BorshSerialize" are outdated.
Test descriptions are not clear enough.

closing-accounts.md

Use InitSpace to calculate space needed for accounts.
Delete "force_defund" because “CLOSED_ACCOUNT_DISCRIMINATOR” was removed in the latest version of anchor-lang
Add the new secure instruction for account closing in the "Secure account closing" section .
Delete Unnecessary parameters found in the test typescript.
Change the Logic of account closing "Sets the account discriminator to the CLOSED_ACCOUNT_DISCRIMINATOR variant" to "Assigning the owner of the account to the System Program and rellocating the
size of the account's data with 0 bytes."

arbitrary-cpi.md

The anchor cpi lesson link is invalid.
Magic number found on space calculation.

Summary of Changes

signer-auth.md

Update the latest way to get the seed.
replace "init" with "init_if_needed" for vault field in the struct InitializeVault<'info>.
delete unnecessary parameters "accounts()" in the test typescript.
Update the project repo links.

reinitialization-attacks.md

Use InitSpace to calculate space needed for accounts.
Use "@coral-xyz/anchor" instead of "@project-serum/anchor" for the test typescript.
Consistently use "rpc()" to send transactions in the test typescript.
Update BDD style for the test typescript.

Also, I made a PR for solana-reinitialization-attacks starter branch and a PR for solana-reinitialization-attacks solution branch
which must be sync with this PR.

pda-sharing.md

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.

owner-checks.md

Use InitSpace to calculate space needed for accounts.
Delete Unnecessary parameters found in the test typescript.
Consistently use "rpc()" as sending transactions in the test typescript.

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

duplicate-mutable-accounts.md

Use InitSpace to calculate space needed for accounts.
Replace "BorshDeserialize" and "BorshSerialize" with "AnchorDeserialize" and "AnchorSerialize".
Make test descriptions more clear.

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

closing-accounts.md

Use InitSpace to calculate space needed for accounts.
Delete "force_defund"  because “CLOSED_ACCOUNT_DISCRIMINATOR” was removed in the latest version of anchor-lang.
Add the new secure instruction for account closing in the "Secure account closing" section.
Change the Logic of closing account "Sets the account discriminator to the CLOSED_ACCOUNT_DISCRIMINATOR variant" to "Assigning the owner to the System Program".

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

arbitrary-cpi.md

Fix the anchor cpi lesson link.
Use InitSpace to calculate space needed for accounts.

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

replace "init" with "init_if_needed" for vault field in the struct InitializeVault<'info>.
delete unnecessary parameters "accounts()" in the test typescript.
@wuuer wuuer requested a review from nickfrosty as a code owner August 28, 2024 16:04
@mikemaccana
Copy link
Contributor

I've reviewed the PR on the other repo, but also forked it (and all branches to https://github.com/solana-developers/solana-signer-auth/ and invited you.

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.

Great work. Nearly ready to go in. Just some minor bits.

PS - I'm mikemaccana on telegram and x if you want faster responses.

wuuer added 3 commits August 29, 2024 09:47
…st typescript.

Consistently use "rpc()" in the test typescript.
Use InitSpace to calculate space needed for accounts.
Update BDD style for the test typescript.
@wuuer wuuer changed the title fix and improve signer-auth.md fix and improve signer-auth.md & reinitialization-attacks.md Aug 29, 2024
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"
@wuuer wuuer changed the title fix and improve signer-auth.md & reinitialization-attacks.md fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md Aug 30, 2024
@wuuer
Copy link
Contributor Author

wuuer commented Aug 30, 2024

I've reviewed the PR on the other repo, but also forked it (and all branches to https://github.com/solana-developers/solana-signer-auth/ and invited you.
@mikemaccana I committed changes to this repo and changed the links in the course to refer to this repo.

Please do the same things:

  1. Fork solana-reinitialization-attacks including all the branches.
  2. Fork solana-pda-sharing including all the branches.
  3. Fork solana-owner-checks including all the branches.
  4. Fork solana-duplicate-mutable-accounts including all the branches.
  5. Fork solana-closing-accounts including all the branches.

Use InitSpace to calculate space needed for accounts.
Use the latest "connection.confirmTransaction()"
Delete Unnecessary parameters found in the test typescript.
Consistently use "rpc()" as sending transactions in the test typescript.
@wuuer wuuer changed the title fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md Aug 31, 2024
Replace "BorshDeserialize" and "BorshSerialize" with "AnchorDeserialize" and "AnchorSerialize".
Make test descriptions more clear.
Delete the "Secure account closing section" section because “CLOSED_ACCOUNT_DISCRIMINATOR” was removed in the latest version of anchor-lang.
Delete "force_defund"  because “CLOSED_ACCOUNT_DISCRIMINATOR” was removed in the latest version of anchor-lang.
Add the new secure instruction of account closing.
Delete Unnecessary parameters found in the test typescript.
Change the Logic of closing account "Sets the account discriminator to the `CLOSED_ACCOUNT_DISCRIMINATOR` variant" to "Assigning the owner to the System Program"
@wuuer wuuer changed the title fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md & closing-accounts. md Sep 2, 2024
Use InitSpace to calculate space needed for accounts.
@wuuer wuuer changed the title fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md & closing-accounts. md fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md & closing-accounts.md & arbitrary-cpi.md Sep 3, 2024
wuuer and others added 4 commits September 4, 2024 09:27
Update the error message in the section "Run the existing test".
Add “--skip-deploy” to the test command.
Delete an unnecessary setup in the section "Adding a `local-testing` feature".
Delete Unnecessary parameters found in the test typescript.
@mikemaccana
Copy link
Contributor

great work @wuuer!

  1. Great work! Let's jump into telegram or X and DM for faster chat - I'm mikemaccana on either.

  2. You have the same code in this PR and fix and improve program-configuration.md & program-architecture.md #423 - which should I review? There's no need to open two PRs for the same work.

  3. New repos made:

https://github.com/solana-developers/reinitialization-attacks/
https://github.com/solana-developers/pda-sharing
https://github.com/solana-developers/closing-accounts

https://github.com/Unboxed-Software/solana-duplicate-mutable-accounts PRs need rebasing, as someone else already submitted updates.

I could not do https://github.com/Unboxed-Software/solana-owner-checks, see Unboxed-Software/solana-owner-checks#2

@wuuer
Copy link
Contributor Author

wuuer commented Sep 5, 2024

great work @wuuer!

  1. You have the same code in this PR and fix and improve program-configuration.md #423 - which should I review? There's no need to open two PRs for the same work.

@mikemaccana
This PR(#342) is for tokens-and-nfts course content.
This PR(#372) is for program-security course content.
This PR(#423) is for program-optimization course content.
Do you agree with me doing this? If not, I will merge these three PRs.

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.

@wuuer sorry about the wait here - we had a 24-person in-room class with new content in NY, then Rustconf then Breakpoint.

Re: your recent comment, some of your PRs are shortlisted for some lessons but yes, it's much easier to do one PR per lesson as others have done. There's no guarantee that someone that wins one lesson will win another one so we want to be able to merge and review them independently.

After you split them out, link them here with the lesson they are for.

@wuuer
Copy link
Contributor Author

wuuer commented Sep 27, 2024

@wuuer sorry about the wait here - we had a 24-person in-room class with new content in NY, then Rustconf then Breakpoint.

Re: your recent comment, some of your PRs are shortlisted for some lessons but yes, it's much easier to do one PR per lesson as others have done. There's no guarantee that someone that wins one lesson will win another one so we want to be able to merge and review them independently.

After you split them out, link them here with the lesson they are for.

@mikemaccana

token and nfts

token-program
token-program-advance

token extensions

close-mint

program security

signer-auth
reinitialization-attacks
pad-sharing
owner-checks
duplicate-mutable-accounts
closing-account
arbitrary-cpi

program optimization

program-configuration
program-architecture

@@ -1,13 +1,13 @@
---
title: Program Configuration
title: Admin configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

@wuuer this is great, sorry it's the last big PR and I've just gotten around to reviewing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the approach of labelling this as 'admin' config, ie program configuration could be per-user config. 👍

We should probably rename it, but maybe we can do that later to not worry about 404s for the old URL

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.

This is some solid work, if you can address the small issues below you'll win at least another one of the bounties.

@@ -1,13 +1,13 @@
---
title: Program Configuration
title: Admin configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the approach of labelling this as 'admin' config, ie program configuration could be per-user config. 👍

We should probably rename it, but maybe we can do that later to not worry about 404s for the old URL

"env9Y3szLdqMLU9rXpEGPqkjdvVn8YNHtxYNvCKXmHe.json"
);
let keyData = JSON.parse(rawdata);
let key = anchor.web3.Keypair.fromSecretKey(new Uint8Array(keyData));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use loadKeypairFromFile() for this, see https://github.com/solana-developers/helpers

```

Boom. Just like that, you've used features to run two different code paths for
different environments.

#### 4. Program Config
#### 4. admin config
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### 4. admin config
#### 4. Admin config

@@ -650,7 +640,7 @@ pub mod config {
}
```

#### 5. Program Config State
#### 5. admin config State
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### 5. admin config State
#### 5. Admin config state

#[account(init, seeds = [SEED_PROGRAM_CONFIG], bump, payer = authority, space = ProgramConfig::LEN)]
pub program_config: Account<'info, ProgramConfig>,
pub struct InitializeAdminConfig<'info> {
#[account(init, seeds = [SEED_ADMIN_CONFIG], bump, payer = authority, space = AdminConfig::LEN)]
Copy link
Contributor

Choose a reason for hiding this comment

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

You only use AdminConfig::LEN once - just remove it and inline

DISCRIMINATOR_SIZE + AdminConfig::INIT_SPACE;


```typescript
it("Payment completes successfully", async () => {
it("Payment should complete successfully", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar is off with these.

Either:

it("should complete successfully"

(if describe was 'payment')

or

test("payment completes sucessfully")

See https://github.com/solana-foundation/developer-content/blob/main/CONTRIBUTING.md#jsts

Do this with all the tests.


```typescript
it("Update Program Config Account with unauthorized admin (expect fail)", async () => {
it("Admin Config Update with unauthorized admin should throw an exception", async () => {
try {
const tx = await program.methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Use transaction, instruction, signature, error, etc everywhere. See 'use full names' in https://github.com/solana-foundation/developer-content/blob/main/CONTRIBUTING.md#code

content/courses/program-security/pda-sharing.md Outdated Show resolved Hide resolved
@@ -1027,52 +1024,6 @@ of [the same repository](https://github.com/Unboxed-Software/solana-admin-instr

## Challenge

Now it's time for you to do some of this on your own. We mentioned being able to
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know why you got rid of this!

@wuuer
Copy link
Contributor Author

wuuer commented Oct 8, 2024

This is some solid work, if you can address the small issues below you'll win at least another one of the bounties.

@mikemaccana . As 0xCipherCoder won the lesson program-configuration, I will not update the lesson.

Please let me know if there are other PRs of mine left to win the Superteam earn 😉

@mikemaccana
Copy link
Contributor

@wuuer I sent you an email with details on your 3 official wins!

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.

Just a reminder this has won program-security/closing-accounts.md, you can probably remove the changes to other lessons (maybe submit them later) and focus on program-security/closing-accounts.md

Copy link

github-actions bot commented Dec 2, 2024

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.

2 participants