Updated bundle sell in secondary market #1606
Conversation
Updated bundle sell in secondary market
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| @@ -83,6 +83,8 @@ library LibAsset { | |||
| /// @param rightClass The asset class type of the right side of the trade. | |||
| /// @return FeeSide representing which side should bear the fee, if any. | |||
| function getFeeSide(AssetClass leftClass, AssetClass rightClass) internal pure returns (FeeSide) { | |||
| require((leftClass == AssetClass.ERC20 || rightClass == AssetClass.ERC20), "exchange not allowed"); | |||
There was a problem hiding this comment.
This is doing too much, its essentially blocking ERC721 <> ERC721 trade and its out of scope for us.
We just want to make sure that if one side is BUNDLE the other is ERC20
Example:
| require((leftClass == AssetClass.ERC20 || rightClass == AssetClass.ERC20), "exchange not allowed"); | |
| if (leftClass == AssetClass.BUNDLE || rightClass == AssetClass.BUNDLE) { | |
| require( | |
| ((leftClass == AssetClass.BUNDLE && rightClass == AssetClass.ERC20) || | |
| (rightClass == AssetClass.BUNDLE && leftClass == AssetClass.ERC20)), | |
| "exchange not allowed" | |
| ); | |
| } | |
| } |
| if (!_isTSBSeller(nftSide.account)) { | ||
| remainder = _doTransfersWithFeesAndRoyaltiesForBundle(paymentSide, nftSide, nftSideRecipient); | ||
| } else { | ||
| // No royalties but primary fee should be paid on the total value of the bundle | ||
| remainder = _transferProtocolFees(remainder, paymentSide, fees); | ||
| } |
There was a problem hiding this comment.
We don't want to eliminate the _isTSBSeller check because TSB may be selling land, catalysts etc and we dont want to pay royalty ourselves.
There was a problem hiding this comment.
Generally:
_isTSBSeller => forces primary market on all bundle items, skips royalties
_isTSBBundleSeller => allows selling non-primary market items in a bundle, pays royalties
regular user => can only sell items that pass _isPrimaryMarket check as true, no royalties
There was a problem hiding this comment.
The names of the roles probably aren't great, I find them bit confusing.
There was a problem hiding this comment.
Roles name will be updated to
- TSB_PRIMARY_MARKET_SELLER
- TSB_SECONDARY_MARKET_SELLER
| if (quadSize > 0 && _isTSBBundleSeller(nftSide.account)) { | ||
| remainder = _processQuadBundles(paymentSide, nftSideRecipient, remainder, feePrimary, quadSize, bundle); | ||
| } |
There was a problem hiding this comment.
We dont need this check for isTSBBundleSeller here, users should not be able to bundle quads because it should fail on _isPrimaryMarket check
There was a problem hiding this comment.
_isPrimaryMarket isn't checked for bundled quad.
and we need only TSB_SECONDARY_MARKET_SELLER to be checked for royalties to be paid for quad.
capedcrusader21
force-pushed
the
feat/added-TSB_BUNDLE_SELLER-and-non-bundle-payment-side
branch
from
b510c64
to
90ee70f
Compare
capedcrusader21
requested review from
wojciech-turek and
adjisb
and removed request for
a team
capedcrusader21
changed the title
capedcrusader21
changed the title
…-place Verify bundle seller in one place
wojciech-turek
merged commit fef969b
into
marketplace-bundle/N-06-gas-optimization
wojciech-turek
deleted the
feat/added-TSB_BUNDLE_SELLER-and-non-bundle-payment-side
branch
Description
updated TSB seller role to
TSB_PRIMARY_MARKET_SELLER&TSB_SECONDARY_MARKET_SELLER.updated contracts for secondary market sale for bundle.
Allow exchange only for bundle when one side of order is ERC20 and other is Bundle.
fixed and added test cases according to above changes.