-
Notifications
You must be signed in to change notification settings - Fork 15
Asset + Marketplace Contract #81
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
base: main
Are you sure you want to change the base?
Conversation
be618c1 to
d0c79a7
Compare
| } | ||
|
|
||
| listing.reserved = Some(Reserve { | ||
| reserver: reservation.reserver.clone(), |
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.
This is an unsafe call. Addr types are not sanitized from msgs. Even though the type is set, the address might be invalid, and needs to be checked
|
|
||
| #[cw_serde] | ||
| pub struct Reserve { | ||
| pub reserver: Addr, |
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.
Addr types are good for state storage, but not good for msg types, because of their lack of sanitization. You probably can't reuse the state definition for the msg definition. Also, you likely want to make the reserver optional, and if unset, uses the msg sender
| .iter() | ||
| .find(|coin| coin.denom == price.denom) | ||
| .ok_or_else(|| ContractError::NoPayment {})? | ||
| .clone(); |
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.
What happens if multiple coins are sent? Seems like everything other than the first match will be stuck in the contract. If only one token is expected, it should be enforced
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.
Adjusted to fail the tx if multiple coins are sent
| .ok_or_else(|| ContractError::NoPayment {})? | ||
| .clone(); | ||
|
|
||
| if payment.amount.lt(&price.amount) || payment.denom != price.denom { |
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.
This checks if you underpaid, which is good, but does not check if you overpaid. Do we intend to let people overpay? If so, that will need to be explicitly documented
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 have an exact price plugin that can be set to enforce the exact price of an asset is sent. If this isn't set, only underpayments are considered. Should we fail on overpayments regardless ?
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.
| id: String, | ||
| price: Coin, | ||
| reservation: Option<Reserve>, | ||
| marketplace_fee_bps: Option<u16>, |
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.
This initialism is never defined anywhere. I don't know what it means
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.
this is sent in the listing msg and is the marketplace fee in basis point
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 removed this marketplace fee from the asset
| }); | ||
| } | ||
|
|
||
| let (validated_marketplace_fee_bps, validated_marketplace_fee_recipient) = |
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.
Seems like these marketplace values can be set by the lister. How do they interact with the marketplace? Why would someone listing an asset set them?
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.
an individual listing an item would not set this. This is strictly for scenarios where marketplaces are the ones listing and want to set a fee
|
|
||
| listing.reserved = Some(Reserve { | ||
| reserver: reservation.reserver.clone(), | ||
| reserved_until: reservation.reserved_until, |
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 there any bound on this value? Or can I just reserve something for 10,000,000 years?
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.
there is a time lock plugin that sets the maximum duration an asset can be reserved for and is enforced upon reservation but if the time lock plugin is not set, the asset can be reserved indefinitely until unreserved by the owner or approvals
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.
| .add_attribute("price", price.amount.to_string()) | ||
| .add_attribute("denom", price.denom) | ||
| .add_attribute("seller", seller.to_string()) | ||
| .add_attribute("buyer", buyer.to_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.
this doesn't include all of the data necessary for backing out the pricing. marketplace configuration args should be output here as well
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.
they are included early if the marketplace config was set or not at all
* chore: add in base spec msgs * chore: asset method trait * chore: implement default plugins * chore: listing an asset * chore: more coverage on Listing * chore: delist and buy WIP on timelocks * chore: unit test on delist * chore: reserve and reserve unit test * chore: more generic contract * feat: add in default plugins * feat: add in test and asset-base contract * chore: disable raw transfer if royalty is set * feat: add in query messages * fix: remove default features for asset-base * feat: make sure deductions are made in sellable buy logic * chore: add in query impl into asset-base contract * format toml * add marketplace to lockfile * add cw-address-like * add contract * update marketplace * add queries * add tests * move offers * add item-sold event * complete buy * offers * add more tests * remove unused * accept offer * change to queue * set fee recipient and update config by manager * chore: remove unwrap() * fix: proper seller is owner * fix linting * check listing is tied to seller * add justfile * fix clippy issues * add royalty test * Merge pull request #86 from burnt-labs/feature/marketplace-approval add approve/reject * update cargo file, finalize for and split execute * account for marketplace fees --------- Signed-off-by: Kehinde Faleye <[email protected]> Co-authored-by: peartes <[email protected]>
* handle asset price * split tests * fix tests * fix lint * format code
* fix fee on approval * apply fee on accept offer * format * add tests * update test and approval --------- Co-authored-by: Kehinde Faleye <[email protected]>
* fmt code * handle linter fixes for account, user map and treasury contracts * handle asset lint issues * format code
This is the Asset standard Contract. Details on the design is here