-
Notifications
You must be signed in to change notification settings - Fork 11
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
[mmm] T22 extension MMM support #87
Conversation
bump | ||
)] | ||
pub pool: Box<Account<'info, Pool>>, | ||
pub asset_mint: InterfaceAccount<'info, Mint>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets enforce token program here
programs/mmm/src/ext_util.rs
Outdated
pub fn check_allowlists_for_mint_ext( | ||
allowlists: &[Allowlist], | ||
token_mint: &AccountInfo, | ||
allowlist_aux: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we going to use merkle tree for t22? or token group is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be good for now as no one is using the group extension
programs/mmm/src/ext_util.rs
Outdated
@@ -0,0 +1,120 @@ | |||
use anchor_lang::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we create a util mod and put this file along with the generic util.rs
file in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The util file exists, I can merge these two
new Array(32).fill(0), | ||
); | ||
|
||
const { mint: mint2, recipientTokenAccount: recipientTokenAccount2 } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably remove this second test, or test with no group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no_group has been tested in correctly verifies depositing nfts with ANY allowlist
.
tests/utils/nfts.ts
Outdated
connection: Connection, | ||
payer: Keypair, | ||
recipient?: PublicKey, | ||
groupKeyPair?: Keypair, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make some of these arguments PublicKey
if there is no need for them to be a keypair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. what about fulfill buy for t22 ext?
merged in to this branch as well. Also need to take care of withdraw but will implement in a separate pr |
try_close_sell_state(sell_state, payer.to_account_info())?; | ||
|
||
// return the remaining per pool escrow balance to the shared escrow account | ||
if pool.using_shared_escrow() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we will need to consider shared escrow, which means we will need to use check_remaining_accounts_for_m2 and withdraw_m2 to withdraw the sol from M2 first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good call!
pub system_program: Program<'info, System>, | ||
pub token_program: Interface<'info, TokenInterface>, | ||
pub associated_token_program: Program<'info, AssociatedToken>, | ||
pub rent: Sysvar<'info, Rent>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is rent required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it is required for the init_if_needed_at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be good to get rid of rent account now; in other places the init_if_needed doesn't require this account anymore
} else { | ||
owner.to_account_info() | ||
}; | ||
let PoolPriceInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to double check if we cover all the necessary logic within this function refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's simply a DRY refactor and should be covered by the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added more comments about shared escrow testing
0, // buyside_creator_royalty_bp, | ||
) | ||
}?; | ||
let lp_fee = get_sol_lp_fee(pool, buyside_sol_escrow_account.lamports(), seller_receives)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this same logic was checked into the previous shared escrow PRs, and it won't affect the current implementation, but if there is a double-sided pool that is shared escrow, because we withdraw the SOL from shared escrow after we perform this LP fee calculation, there will be no LP fee charged ever.
|
||
let (total_price, next_price) = | ||
get_sol_total_price_and_next_price(pool, args.asset_amount, true)?; | ||
let seller_receives = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to move this as well with the comment, just fyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
collection offer support for T22 extension