Skip to content

Commit

Permalink
Review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@anajuliabit
anajuliabit committed May 22, 2023
1 parent c50622d commit ff43c33
Show file tree
Hide file tree
Showing 9 changed files with 98 additions and 83 deletions.
28 changes: 28 additions & 0 deletions contracts/interfaces/handlers/IBosonExchangeHandler.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,34 @@ interface IBosonExchangeHandler is IBosonExchangeEvents, IBosonFundsLibEvents, I
*/
function commitToOffer(address payable _buyer, uint256 _offerId) external payable;

/**
* @notice Commits to an conditional offer (first step of an exchange).
*
* Emits a BuyerCommitted event if successful.
* Issues a voucher to the buyer address.
*
* Reverts if:
* - The exchanges region of protocol is paused
* - The buyers region of protocol is paused
* - OfferId is invalid
* - Offer has been voided
* - Offer has expired
* - Offer is not yet available for commits
* - Offer's quantity available is zero
* - Buyer address is zero
* - Buyer account is inactive
* - Conditional commit requirements not met or already used
* - Offer price is in native token and caller does not send enough
* - Offer price is in some ERC20 token and caller also sends native currency
* - Contract at token address does not support ERC20 function transferFrom
* - Calling transferFrom on token fails for some reason (e.g. protocol is not approved to transfer)
* - Received ERC20 token amount differs from the expected value
* - Seller has less funds available than sellerDeposit
*
* @param _buyer - the buyer's address (caller can commit on behalf of a buyer)
* @param _offerId - the id of the offer to commit to
* @param _tokenId - the id of the token to use for the conditional commit
*/
function commitToConditionalOffer(address payable _buyer, uint256 _offerId, uint256 _tokenId) external payable;

/**
Expand Down
8 changes: 1 addition & 7 deletions contracts/protocol/bases/BundleBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,7 @@ contract BundleBase is ProtocolBase, IBosonBundleEvents {
uint256 _offerId
) internal view returns (uint256 offersTotalQuantity) {
// make sure all offers exist and belong to the seller
Offer storage offer = getValidOffer(_offerId);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
(Offer storage offer, ) = getValidOfferAndSeller(_offerId);

// Unchecked because we're handling overflow below
unchecked {
Expand Down
30 changes: 9 additions & 21 deletions contracts/protocol/bases/GroupBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,7 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents {

for (uint256 i = 0; i < _group.offerIds.length; i++) {
// make sure offer exists and belongs to the seller
Offer storage offer = getValidOffer(_group.offerIds[i]);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
getValidOfferAndSeller(_group.offerIds[i]);

// Offer should not belong to another group already
(bool exist, ) = getGroupIdByOffer(_group.offerIds[i]);
Expand Down Expand Up @@ -108,11 +102,11 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents {
*
* A invalid condition is one that fits any of the following criteria:
* - EvaluationMethod.None: any fields different from 0
* - EvaluationMethod.Threshold: token address, maxCommits or threshold is zero and length is not zero
* - EvaluationMethod.Threshold: token address, maxCommits or threshold is zero or length is not zero
* - EvaluationMethod.SpecificToken:
* - tokenType is FungibleToken
* - tokenType is NonFungibleToken and threshold is not zero
* - tokenType is NonFunbileToken, tokenId is not zero and length is zero
* - tokenId is not zero and length is zero
* - tokenType is MultiToken and threshold is zero
* - maxCommits is zero
* - token address is zero
Expand All @@ -139,14 +133,14 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents {
_condition.maxCommits > 0 &&
_condition.tokenType != TokenType.FungibleToken); // FungibleToken not allowed for SpecificToken

if (_condition.tokenId != 0) {
// SpecificToken with tokenId should have length
valid = valid && _condition.length > 0;
}

// SpecificToken with NonFungibleToken should not have threshold
if (_condition.tokenType == TokenType.NonFungibleToken) {
valid = valid && _condition.threshold == 0;

if (_condition.tokenId != 0) {
// SpecificToken with NonFungibleToken and tokenId should have length
valid = valid && _condition.length > 0;
}
} else {
// SpecificToken with MultiToken should have threshold
valid = valid && _condition.threshold > 0;
Expand Down Expand Up @@ -185,14 +179,8 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents {

for (uint256 i = 0; i < _offerIds.length; i++) {
uint256 offerId = _offerIds[i];
// make sure offer exist and belong to the seller
Offer storage offer = getValidOffer(offerId);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
getValidOfferAndSeller(offerId);

// Offer should not belong to another group already
(bool exist, ) = getGroupIdByOffer(offerId);
Expand Down
8 changes: 1 addition & 7 deletions contracts/protocol/bases/OfferBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,7 @@ contract OfferBase is ProtocolBase, IBosonOfferEvents {
address _to
) internal offersNotPaused exchangesNotPaused {
// Get offer, make sure the caller is the assistant
Offer storage offer = getValidOffer(_offerId);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
(Offer storage offer, ) = getValidOfferAndSeller(_offerId);

// Prevent reservation of an empty range
require(_length > 0, INVALID_RANGE_LENGTH);
Expand Down
23 changes: 23 additions & 0 deletions contracts/protocol/bases/ProtocolBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,29 @@ abstract contract ProtocolBase is PausableBase, ReentrancyGuardBase {
require(!offer.voided, OFFER_HAS_BEEN_VOIDED);
}

/**
* @notice Gets offer and seller from protocol storage
*
* Reverts if:
* - Offer does not exist
* - Offer already voided
* - Seller assistant is not the caller
*
* @param _offerId - the id of the offer to check
*/
function getValidOfferAndSeller(
uint256 _offerId
) internal view returns (Offer storage offer, Seller storage seller) {
// Get offer
offer = getValidOffer(_offerId);

// Get seller, we assume seller exists if offer exists
(, seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
}

/**
* @notice Gets the bundle id for a given offer id.
*
Expand Down
7 changes: 5 additions & 2 deletions contracts/protocol/facets/ExchangeHandlerFacet.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
*
* @param _buyer - the buyer's address (caller can commit on behalf of a buyer)
* @param _offerId - the id of the offer to commit to
* @param _tokenId - the id of the token to use for the conditional commit
*/
function commitToConditionalOffer(
address payable _buyer,
Expand All @@ -139,6 +140,8 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
// Get the condition
Condition storage condition = fetchCondition(groupId);

require(condition.method != EvaluationMethod.None, GROUP_HAS_NO_CONDITION);

bool allow = authorizeCommit(_buyer, condition, groupId, _tokenId);
require(allow, CANNOT_COMMIT);

Expand Down Expand Up @@ -960,7 +963,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {

if (_condition.method == EvaluationMethod.SpecificToken) {
// How many times has this token id been used to commit to offers in the group?
uint256 commitCount = lookups.conditionalCommitsByTokenId[_groupId][_tokenId];
uint256 commitCount = lookups.conditionalCommitsByTokenId[_tokenId][_groupId];

require(commitCount < _condition.maxCommits, MAX_COMMITS_TOKEN_REACHED);

Expand All @@ -976,7 +979,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {

if (allow) {
// Increment number of commits to the group for this token id if they are allowed to commit
lookups.conditionalCommitsByTokenId[_groupId][_tokenId] = ++commitCount;
lookups.conditionalCommitsByTokenId[_tokenId][_groupId] = ++commitCount;
}
} else if (_condition.method == EvaluationMethod.Threshold) {
// How many times has this address committed to offers in the group?
Expand Down
18 changes: 3 additions & 15 deletions contracts/protocol/facets/OfferHandlerFacet.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,8 @@ contract OfferHandlerFacet is IBosonOfferHandler, OfferBase {
* @param _offerId - the id of the offer to void
*/
function voidOffer(uint256 _offerId) public override offersNotPaused nonReentrant {
// Get offer, make sure the caller is the assistant
Offer storage offer = getValidOffer(_offerId);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
// Get offer. Make sure caller is assistant
(Offer storage offer, ) = getValidOfferAndSeller(_offerId);

// Void the offer
offer.voided = true;
Expand Down Expand Up @@ -220,13 +214,7 @@ contract OfferHandlerFacet is IBosonOfferHandler, OfferBase {
*/
function extendOffer(uint256 _offerId, uint256 _validUntilDate) public override offersNotPaused nonReentrant {
// Make sure the caller is the assistant, offer exists and is not voided
Offer storage offer = getValidOffer(_offerId);

// Get seller, we assume seller exists if offer exists
(, Seller storage seller, ) = fetchSeller(offer.sellerId);

// Caller must be seller's assistant address
require(seller.assistant == msgSender(), NOT_ASSISTANT);
(Offer storage offer, ) = getValidOfferAndSeller(_offerId);

// Fetch the offer dates
OfferDates storage offerDates = fetchOfferDates(_offerId);
Expand Down
2 changes: 1 addition & 1 deletion contracts/protocol/libs/ProtocolLib.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ library ProtocolLib {
mapping(uint256 => BosonTypes.AuthToken) pendingAuthTokenUpdatesBySeller;
// dispute resolver id => DisputeResolver
mapping(uint256 => BosonTypes.DisputeResolver) pendingAddressUpdatesByDisputeResolver;
// groupId => tokenId => commit count (count how many times a token has been used as gate for this group)
// tokenId => groupId => commit count (count how many times a token has been used as gate for this group)
mapping(uint256 => mapping(uint256 => uint256)) conditionalCommitsByTokenId;
}

Expand Down
57 changes: 27 additions & 30 deletions test/protocol/ExchangeHandlerTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1553,36 +1553,6 @@ describe("IBosonExchangeHandler", function () {
});
});

context("✋ Group without condition", async function () {
let tokenId = "0";
beforeEach(async function () {
// Create a new offer
await offerHandler
.connect(assistant)
.createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);

// Required constructor params for Group
groupId = "1";
offerIds = [(++offerId).toString()];

// Create Condition
condition = mockCondition({ method: EvaluationMethod.None, threshold: "0", maxCommits: "0" });
expect(condition.isValid()).to.be.true;

// Create Group
group = new Group(groupId, seller.id, offerIds);
expect(group.isValid()).is.true;
await groupHandler.connect(assistant).createGroup(group, condition);
});

it("should emit a BuyerCommitted event", async function () {
// Commit to offer.
await expect(
exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price })
).to.emit(exchangeHandler, "BuyerCommitted");
});
});

context("💔 Revert Reasons", async function () {
let tokenId;

Expand Down Expand Up @@ -1717,6 +1687,33 @@ describe("IBosonExchangeHandler", function () {
exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price })
).to.revertedWith(RevertReasons.OFFER_SOLD_OUT);
});

it("Group without condition ", async function () {
let tokenId = "0";

// Create a new offer
await offerHandler
.connect(assistant)
.createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);

// Required constructor params for Group
groupId = "1";
offerIds = [(++offerId).toString()];

// Create Condition
condition = mockCondition({ method: EvaluationMethod.None, threshold: "0", maxCommits: "0" });
expect(condition.isValid()).to.be.true;

// Create Group
group = new Group(groupId, seller.id, offerIds);
expect(group.isValid()).is.true;
await groupHandler.connect(assistant).createGroup(group, condition);

// Commit to offer.
await expect(
exchangeHandler.connect(buyer).commitToConditionalOffer(buyer.address, offerId, tokenId, { value: price })
).to.revertedWith(RevertReasons.GROUP_HAS_NO_CONDITION);
});
});
});

Expand Down

0 comments on commit ff43c33

Please sign in to comment.