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

Added IDO Program Milestone Project #85

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

0xcatrovacer
Copy link

@0xcatrovacer 0xcatrovacer commented Jun 26, 2023

Anchor milestone project based on advanced Anchor concepts

Concepts added:

  • Cross Program Invocations
  • Access Control
  • Constants
  • Errors
  • Testing

@acheroncrypto

@buffalojoec
Copy link

Commenting so I get updatez

From RFP for Anchor Book contributions

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Hey, thank you for the contribution. Please see the review comments(some are pretty repetitive) that should be addressed before this can get merged.

It would be great if the explanation side of things improved as well. When contributing to a book of something such as anchor book, it's expected for the contributors to have a higher degree of precision in their words. I'll give an example: The term "fiat" and "stablecoin" are two different phenomena and should not be used interchangebly. The word "fiat" has been used 435 times in this PR yet it doesn't look like anything in the program that checks whether that's the case and the program doesn't have to use "fiat" anyway.

programs/ido-program/Anchor.toml Outdated Show resolved Hide resolved
programs/ido-program/migrations/deploy.ts Outdated Show resolved Hide resolved
programs/ido-program/package.json Outdated Show resolved Hide resolved
programs/ido-program/programs/ido-program/src/lib.rs Outdated Show resolved Hide resolved
src/anchor_in_depth/milestone_project_ido-program.md Outdated Show resolved Hide resolved
src/anchor_in_depth/milestone_project_ido-program.md Outdated Show resolved Hide resolved
src/anchor_in_depth/milestone_project_ido-program.md Outdated Show resolved Hide resolved
src/anchor_in_depth/milestone_project_ido-program.md Outdated Show resolved Hide resolved
src/anchor_in_depth/milestone_project_ido-program.md Outdated Show resolved Hide resolved
0xcatrovacer and others added 27 commits July 2, 2023 09:30

/// CHECK: This is not dangerous
#[account(mut)]
pub pool_signer: AccountInfo<'info>,

Choose a reason for hiding this comment

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

I think _authority is a better naming convention here, especially considering it's not signing anything in this instruction

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that would be very confusing considering we also have authority account.

_ generally means not being used in Rust and it's good practice to make account names consistent in instructions.

Choose a reason for hiding this comment

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

This was meant as a suffix, ie *_authority

Copy link
Author

Choose a reason for hiding this comment

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

Just to be clear, should I be using pool_signer or pool_authority?

Comment on lines 208 to 220
impl<'info> InitializePool<'info> {
fn accounts(ctx: &Context<InitializePool<'info>>, bump: u8) -> Result<()> {
let expected_signer = Pubkey::create_program_address(
&[ctx.accounts.pool_native.mint.as_ref(), &[bump]],
ctx.program_id,
)
.map_err(|_| ErrorCode::InvalidBump)?;
if ctx.accounts.pool_signer.key != &expected_signer {
return Err(ErrorCode::InvalidBump.into());
}
Ok(())
}
}

Choose a reason for hiding this comment

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

The function you instead want to use is Pubkey::find_program_address() and it will take the seeds (without bump) and return the Pubkey and the bump.

You really shouldn't need to pass the bump as an instruction parameter

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

For the terms, we could use depositor instead of investor and deposit token instead of fiat.


/// CHECK: This is not dangerous
#[account(mut)]
pub pool_signer: AccountInfo<'info>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that would be very confusing considering we also have authority account.

_ generally means not being used in Rust and it's good practice to make account names consistent in instructions.

Comment on lines 208 to 220
impl<'info> InitializePool<'info> {
fn accounts(ctx: &Context<InitializePool<'info>>, bump: u8) -> Result<()> {
let expected_signer = Pubkey::create_program_address(
&[ctx.accounts.pool_native.mint.as_ref(), &[bump]],
ctx.program_id,
)
.map_err(|_| ErrorCode::InvalidBump)?;
if ctx.accounts.pool_signer.key != &expected_signer {
return Err(ErrorCode::InvalidBump.into());
}
Ok(())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Though both are valid and you are free to use Native validations, it's not "Anchor way" of doing things.

There is #[instruction(...)] macro if you want to get instruction arguments and validating seeds/bump should be done with #[account(...)] macro, especially in the Anchor Book.

programs/ido-program/programs/ido-program/src/lib.rs Outdated Show resolved Hide resolved

This program will be doing a couple of things. In an IDO, a project distributes its native tokens among willing investors. There are a few types of IDOs which are done. We will be implementing a fair launch IDO platform. In here, a project escrows the native tokens into a pool. When the IDO opens, investors come and deposit their fiat tokens into the pool. After the IDO is over, investors can claim their native tokens, the amount of which is calculated by the the pool percentage share of their deposit. The project can also finally withdraw all the deposited fiat tokens.

We recommend keeping programs in a single `lib.rs` file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we recommend this?

Copy link
Author

Choose a reason for hiding this comment

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

Although maintaining a project directory structure is a good practice, implementing it in the milestone project might make the Anchor Book content massive.

@0xcatrovacer
Copy link
Author

@acheroncrypto @buffalojoec
Made all the recommended changes in the program and the content. Please take a look.

Copy link

@Aursen Aursen left a comment

Choose a reason for hiding this comment

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

I tried to do a little review of the program without necessarily focusing on the rest, I hope it can be useful

pub token_program: Program<'info, Token>,
}

#[account]
Copy link

Choose a reason for hiding this comment

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

Here you can just add #[derive(InitSpace)] and remove the LEN const

pub fn exchange_redeemable_for_native(ctx: Context<ExchangeRedeemableForNative>) -> Result<()> {
let native_amount = (ctx.accounts.depositor_redeemable.amount as u128)
.checked_mul(ctx.accounts.pool_native.amount as u128)
.unwrap()
Copy link

Choose a reason for hiding this comment

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

better here to chain and unwrap at the end (the better is a result)

pool.end_ido_ts = end_ido_ts;
pool.withdraw_deposit_token_ts = withdraw_deposit_token_ts;

pool.bump = bump;
Copy link

Choose a reason for hiding this comment

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

Not a huge fan to let set the bump by an argument, it let place to bump seed non canonicalization exploit

}
// While token::transfer will check this, we prefer a verbose error msg
if ctx.accounts.depositor_deposit_token.amount < amount {
return Err(ErrorCode::LowDepositToken.into());
Copy link

Choose a reason for hiding this comment

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

maybe here use require! macro

pub native_mint: Box<Account<'info, Mint>>,

#[account(mut, constraint = pool_native.owner == *pool_signer.key)]
pub pool_native: Box<Account<'info, TokenAccount>>,
Copy link

Choose a reason for hiding this comment

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

I don't know why the boxing of all accounts are required here

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.

4 participants