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

[MplCore] Deposit sell #93

Merged
merged 9 commits into from
Apr 26, 2024
Merged

[MplCore] Deposit sell #93

merged 9 commits into from
Apr 26, 2024

Conversation

JeremyLi28
Copy link
Contributor

@JeremyLi28 JeremyLi28 commented Apr 20, 2024

Added new mpl_core_deposit_sell.rs instruction and sdk + tests

  • Added a new allowlist type update_authority to be used by mpl core collections.
  • Mpl core assets without collection can still use mintAddress allowlist type

@JeremyLi28 JeremyLi28 changed the title init [MplCore] Deposit sell Apr 23, 2024
pub pool: Box<Account<'info, Pool>>,
#[account(
mut,
constraint = asset.to_account_info().owner == asset_program.key,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check the ownership here? maybe cpi of transfer does this, good

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 reply on cpi check for now

@@ -783,6 +785,68 @@ pub fn check_allowlists_for_mint_ext(
Err(MMMErrorCode::InvalidAllowLists.into())
}

pub fn check_allowlists_for_mpl_core<'info>(
allowlists: &[Allowlist],
asset: &Box<Account<'info, IndexableAsset>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should be able to just

Suggested change
asset: &Box<Account<'info, IndexableAsset>>,
asset: &IndexableAsset,

Copy link
Contributor Author

@JeremyLi28 JeremyLi28 Apr 25, 2024

Choose a reason for hiding this comment

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

interesting, "Deref coercion", good rust learning

) -> Result<()> {
if allowlists
.iter()
.any(|&val| val.kind == ALLOWLIST_KIND_METADATA)
Copy link
Contributor

Choose a reason for hiding this comment

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

explore if we can make Core's onchain traits verifiable.

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 be address in the future when we support onchain attribute offer for mpl core

export function isMplCoreAsset(
account: anchor.web3.AccountInfo<Buffer>,
): boolean {
return account?.owner.equals(new PublicKey(MPL_CORE_PROGRAM_ID)) ?? false;
Copy link
Contributor

Choose a reason for hiding this comment

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

MPL_CORE_PROGRAM_ID is already a PublicKey, right? so we don't need a new here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a separate PublicKey type defined in mpl core package, need to convert to web3 type here

return Err(MMMErrorCode::InvalidAccountState.into());
}

let _ = check_allowlists_for_mpl_core(&pool.allowlists, asset, args.allowlist_aux)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't need let _ =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compiler seems will show some warning if I remove them

@JeremyLi28 JeremyLi28 merged commit 33c5fc3 into main Apr 26, 2024
1 check passed
@JeremyLi28 JeremyLi28 deleted the chen/core-deposit-sell branch April 26, 2024 00:43
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