-
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
[MplCore] Withdraw Sell + fulfill sell + fulfill buy #94
Conversation
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
|
||
pool.sellside_asset_amount = pool | ||
.sellside_asset_amount | ||
.checked_sub(1) |
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.
we should also support asset_amount > 1 right
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 the args passed in can be > 1 but it doesn't make sense since we only deal with 1 asset per instruction. deposit sell also did this
lgtm |
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_withdraw_sell.rs
Outdated
Show resolved
Hide resolved
asset: &IndexableAsset, | ||
collection: Option<&IndexableAsset>, | ||
) -> Option<Royalties> { | ||
let asset_royalty_plugin = asset.plugins.get(&PluginType::Royalties); |
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 believe this is actually the only place that we read data from the collection. Let's make sure that the collection is actually the collection that the asset belongs to.
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.
we actually did check these in https://github.com/magicoss/mmm/blob/37d0f8378b1a0f51e9a99ea4424120e638fe8286/programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs#L242
I'm hesitate add in this function because it make code a bit more complicated that has to handle the error when call this function
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_buy.rs
Outdated
Show resolved
Hide resolved
programs/mmm/src/instructions/mpl_core_asset/mpl_core_fulfill_sell.rs
Outdated
Show resolved
Hide resolved
assetMint, | ||
); | ||
if (!!mintOrCoreAsset && isMplCoreAsset(mintOrCoreAsset)) { | ||
const asset = deserializeAssetV1( |
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.
we can build this into the rpc metadata provider right?
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 I plan to do it another PR
all existing functionalities (reinvest, shared escrow etc) + royalty enforcement