Multiple collections per seller #592

zajck · 2023-03-28T08:40:27Z

PR implements #590.

In contracts/domain/BosonTypes.sol:
- added new type Collection which contains collection address and its external id
- collectionIndex is added to Offer struct
in contracts/interfaces/handlers/IBosonAccountHandler.sol and contracts/protocol/facets/SellerHandlerFacet.sol two methods are added:
- createNewCollection that creates an additional collection for the seller
- getSellersCollections which returns default boson voucher address and list of additional collections
in contracts/protocol/facets/ExchangeHandlerFacet.sol,
- all methods that previously interacted with clone address now get the correct clone address, based on collectionIndex, stored as the part of the offer
VoucherClone initializeVoucher is changed to accept _collectionIndex and add it as part of the name and symbol. Might still be changed: Multiple collections per seller BPIPs#15 (comment)
added a new event CollectionCreated which is emitted when a new collection is created
added/updated domain script and tests for Collection and Offer
added unit tests for use cases with collections

contracts/protocol/facets/SellerHandlerFacet.sol

coveralls · 2023-06-05T20:34:37Z

coverage: 99.679% (+0.003%) from 99.676% when pulling bd773d9 on multiple-collections into ad78b61 on main.

anajuliabit

Looks really good.

Just some thoughts

contracts/protocol/bases/ProtocolBase.sol

contracts/protocol/bases/SellerBase.sol

anajuliabit · 2023-06-05T20:45:56Z

test/protocol/ExchangeHandlerTest.js

+      it("should work on an additional collection", async function () {
+        const externalId = `Brand1`;
+        const contractURI = `https://brand1.com`;
+
+        // Create a new collection
+        await accountHandler.connect(assistant).createNewCollection(externalId, contractURI);
+
+        offer.collectionIndex = 1;
+        offer.id = await offerHandler.getNextOfferId();
+        exchange.id = await exchangeHandler.getNextExchangeId();
+        const tokenId = deriveTokenId(offer.id, exchange.id);
+
+        // Create the offer
+        await offerHandler
+          .connect(assistant)
+          .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);
+
+        // Commit to offer, creating a new exchange
+        await exchangeHandler.connect(buyer).commitToOffer(buyer.address, offer.id, { value: price });
+
+        // expected address of the first additional collection
+        const additionalCollectionAddress = calculateContractAddress(accountHandler.address, "2");
+        const additionalCollection = await ethers.getContractAt("BosonVoucher", additionalCollectionAddress);
+
+        // Revoke the voucher, expecting event
+        await expect(exchangeHandler.connect(assistant).revokeVoucher(exchange.id))
+          .to.emit(additionalCollection, "Transfer")
+          .withArgs(buyer.address, ethers.constants.AddressZero, tokenId);
+      });
+


I suppose it would be more logical if we organized test contexts based on previous steps required instead of always grouping them by function. This way, we could avoid repeating a lot of pre-test setups. Sometimes I also feel that our test organization leads to test duplication. For example, if we want to test a code block in commitToOfferInternal, we end up repeating the same test at least three times because we have three functions that call commitToOfferInternal (commitToOffer, commitToPreMintedOffer, and more recently commitToConditionalOffer).

I'm not saying that you should do this in this PR, just wanted to bring this topic up for future discussions

I agree that some test files are a bit out of hand, and would benefit from some refactoring.

But I'm not sure that the different grouping actually generally solves the duplication, it just turns the code around (it requires that some other part is duplicated).

For example, consider these two organizations:
opt 1:

context A setup A ... other tests with setup A ... test D with an additional setup D context B setup B ... other tests with setup B ... test D with an additional setup D

opt 2:

context D setup D test D with an additional setup A test D with an additional setup B context XY setup XY ... test X with setup A test Y with setup B

In both cases the test D is the test that is a "duplicate", except that tested functions A and B are different (let's say commitToOffer, commitToPreMintedOffer). Test D requires a setup that is a combination of setups for A and D or B and D.

Opt 1 is similar to what we have now, so the top context is based on the function and we later duplicated the code for test D.
Now if we turn this around and group it by test D, we still need two tests, where one includes A specific setup and the other has B specific setup. We sure can do that. But there will still exist other contexts that will require either setup A or setup B. So instead of setup D being duplicated across contexts, now contexts A and B will be duplicated.

And in some cases, opt 2 is indeed better (for example, if setup D is long compared to setup A/B). So if we just wanted to optimize the tests we could regroup them. And that would lead to scenario-based contexts. We had that in v1 and I can tell you it was a nightmare to maintain. When some feature was added/removed it was sometimes even hard to accurately pinpoint into which scenario(s) we should add it.

Also, I think it makes sense to stick to the same option across all tests. And if I'm choosing between what have now and setup-based contexts, I'm sticking with the function based.

But I agree a lot of parts could be refactored to reuse the same code. Because now, if we make a change that affects a "duplicate" test, we need to go and find all affected instances. Ideally, the duplicated part would be a sort of util function (can be on a file level if other tests don't need it) and the test would simply invoke that.

Utility functions make sense to me. We can start migrating to functions when we need to add or refactor a test. A separate file would be better IMO, as we could reuse it in different facets, such as ExchangeHandlerTest and OrchestrationHandlerTest. I believe this would help make the codebase more legible, as our test files are currently too large.

contracts/protocol/facets/SellerHandlerFacet.sol

anajuliabit

LGTM

mischat

This looks good to me !

mischat · 2023-07-06T16:26:11Z

contracts/interfaces/handlers/IBosonAccountHandler.sol

+     */
+    function getSellersCollections(
+        uint256 _sellerId
+    ) external view returns (address defaultVoucherAddress, BosonTypes.Collection[] memory additionalCollections);


Do we worry about the Collection[] being too big ? Do we need something like a getCollection(CollectionID) or something ?

even if this is a good idea, i wouldn't block on it ...

I also think is a good idea, nice catch.
I create an issue so this doesn't get loss
#709

anajuliabit

LGTM

anajuliabit · 2023-07-06T16:54:21Z

contracts/interfaces/handlers/IBosonAccountHandler.sol

+     */
+    function getSellersCollections(
+        uint256 _sellerId
+    ) external view returns (address defaultVoucherAddress, BosonTypes.Collection[] memory additionalCollections);


I also think is a good idea, nice catch.
I create an issue so this doesn't get loss
#709

mischat · 2023-07-06T17:38:02Z

@anajuliabit thanks for creating the issue.

zajck added 7 commits March 22, 2023 17:46

create new collection

3824281

Manage cloneAddress protocol wide

47df762

Collection struct, domain test + old tests

7728d09

seller tests

1d84e2d

createOffer tests

07b6300

exchange handler tests

14ea68b

update natspec

9e07f7d

zajck self-assigned this Mar 28, 2023

zajck added enhancement New feature or request v2.3.0 labels Mar 28, 2023

zajck and others added 2 commits April 26, 2023 16:18

Merge branch 'main' into multiple-collections

6023886

Merge branch 'main' into multiple-collections

7f18712

zajck marked this pull request as ready for review May 16, 2023 10:05

github-advanced-security bot found potential problems May 16, 2023

View reviewed changes

contracts/protocol/facets/SellerHandlerFacet.sol Fixed Show fixed Hide fixed

FIx failing tests

ddc26fe

zajck force-pushed the multiple-collections branch from bc45b66 to ddc26fe Compare May 17, 2023 10:26

add missing tests

d8dcb44

zajck requested review from mischat and anajuliabit June 1, 2023 05:44

Merge branch 'main' into multiple-collections

2b67fc4

anajuliabit suggested changes Jun 5, 2023

View reviewed changes

review fixes

2218b9e

github-advanced-security bot found potential problems Jun 6, 2023

View reviewed changes

contracts/protocol/facets/SellerHandlerFacet.sol Dismissed Show dismissed Hide dismissed

minor optimization

a7662ad

zajck force-pushed the multiple-collections branch from f991c72 to a7662ad Compare June 6, 2023 14:53

anajuliabit previously approved these changes Jun 6, 2023

View reviewed changes

zajck marked this pull request as draft June 29, 2023 14:49

zajck marked this pull request as ready for review July 4, 2023 07:44

zajck mentioned this pull request Jul 4, 2023

Clone vouchers using create2 #703

Merged

Merge branch 'main' into multiple-collections

cbdb989

zajck dismissed anajuliabit’s stale review via cbdb989 July 5, 2023 07:43

zajck and others added 3 commits July 5, 2023 10:19

fix tests

e20839d

Merge branch 'main' into multiple-collections

c7ee054

Merge branch 'main' into multiple-collections

bd773d9

mischat approved these changes Jul 6, 2023

View reviewed changes

mischat requested a review from anajuliabit July 6, 2023 16:30

anajuliabit approved these changes Jul 6, 2023

View reviewed changes

anajuliabit merged commit 6cdc94a into main Jul 6, 2023
9 checks passed

anajuliabit deleted the multiple-collections branch July 6, 2023 17:10

zajck mentioned this pull request Jul 6, 2023

Support multiple collections per seller #590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple collections per seller #592

Multiple collections per seller #592

zajck commented Mar 28, 2023 •

edited

Loading

coveralls commented Jun 5, 2023 •

edited

Loading

anajuliabit left a comment

anajuliabit Jun 5, 2023

zajck Jun 6, 2023

anajuliabit Jun 6, 2023

anajuliabit left a comment

mischat left a comment

mischat Jul 6, 2023

mischat Jul 6, 2023

anajuliabit Jul 6, 2023

anajuliabit left a comment

anajuliabit Jul 6, 2023

mischat commented Jul 6, 2023

Multiple collections per seller #592

Multiple collections per seller #592

Conversation

zajck commented Mar 28, 2023 • edited Loading

coveralls commented Jun 5, 2023 • edited Loading

anajuliabit left a comment

Choose a reason for hiding this comment

anajuliabit Jun 5, 2023

Choose a reason for hiding this comment

zajck Jun 6, 2023

Choose a reason for hiding this comment

anajuliabit Jun 6, 2023

Choose a reason for hiding this comment

anajuliabit left a comment

Choose a reason for hiding this comment

mischat left a comment

Choose a reason for hiding this comment

mischat Jul 6, 2023

Choose a reason for hiding this comment

mischat Jul 6, 2023

Choose a reason for hiding this comment

anajuliabit Jul 6, 2023

Choose a reason for hiding this comment

anajuliabit left a comment

Choose a reason for hiding this comment

anajuliabit Jul 6, 2023

Choose a reason for hiding this comment

mischat commented Jul 6, 2023

zajck commented Mar 28, 2023 •

edited

Loading

coveralls commented Jun 5, 2023 •

edited

Loading