Allows DR creation without the need for activation by protocol admin #466

anajuliabit · 2022-11-14T20:23:38Z

This PR allows DR creation without the need for activation by protocol admin.

Contracts changes:

Remove activateDisputeResolver function
createDisputeResolver:
- Reverts if active = false
- Reverts if feeAmount != 0
addFeesToDisputeResolver:
- Reverts if feeAmount != 0
Comment code related to feeAmount on storeOffer

Scripts changes

Rename create-and-activate-DR -> create-DR and refactor the code to remove activeDisputeResolver calls
Remove activateDisputeResolver from estimate-limits

Unit tests

Fixes broken unit tests
Add new tests related to new revert reasons on createDisputeResolver and addFeesToDisputeResolver
Skip tests that depend on feeAmount != 0

cliffhall

Just a few cosmetic things and a couple of questions.

cliffhall · 2022-11-15T19:49:17Z

contracts/interfaces/handlers/IBosonAccountHandler.sol

     *
     * @param _disputeResolverId - id of the dispute resolver
     * @param _disputeResolverFees - list of fees dispute resolver charges per token type. Zero address is native currency. See {BosonTypes.DisputeResolverFee}
+                                     feeAmount will be ignored because protocol doesn't yet support fees yet but DR still needs to provide array of fees to choose supported tokens


This long line should be broken into multiple lines with in the comment each with * on the left like normal comment lines.

cliffhall · 2022-11-15T19:51:08Z

contracts/protocol/bases/OfferBase.sol

@@ -177,15 +173,16 @@ contract OfferBase is ProtocolBase, IBosonOfferEvents {
                uint256 feeIndex = lookups.disputeResolverFeeTokenIndex[_disputeResolverId][_offer.exchangeToken];
                require(feeIndex > 0, DR_UNSUPPORTED_FEE);

-                uint256 feeAmount = disputeResolverFees[feeIndex - 1].feeAmount;
+                // uint256 feeAmount = disputeResolverFees[feeIndex - 1].feeAmount;


add the // Protocol doesn't yet support DR fees comment above

I believe that we should simply keep this in. Yes, it is a redundant operation, since feeAmount will be always zero (which is effectively the same as it is now). But if we comment this out and want that the change is actually applied to diamon, we need to upgrade both OfferHandler and OrchestrationHandler.

Then when we finally enable non-zero fee amount, we'd need to do the upgrade again (or at least do the cuts to point back to the current implementation).

I put it back

cliffhall · 2022-11-15T19:54:38Z

contracts/protocol/facets/DisputeResolverHandlerFacet.sol

     *
     * @param _disputeResolver - the fully populated struct with dispute resolver id set to 0x0
-     * @param _disputeResolverFees - array of fees dispute resolver charges per token type. Zero address is native currency. Can be empty.
+     * @param _disputeResolverFees - list of fees dispute resolver charges per token type. Zero address is native currency. See {BosonTypes.DisputeResolverFee}
+                                     feeAmount will be ignored because protocol doesn't yet support DR fees but DR still needs to provide array of fees to choose supported tokens


See the comment for this function in the IBosonAccountHandler interface 👆

cliffhall · 2022-11-15T19:57:45Z

contracts/protocol/facets/DisputeResolverHandlerFacet.sol

     * @param _sellerAllowList - list of ids of sellers that can choose this dispute resolver. If empty, there are no restrictions on which seller can chose it.
     */
    function createDisputeResolver(
        DisputeResolver memory _disputeResolver,
-        DisputeResolverFee[] calldata _disputeResolverFees,
+        DisputeResolverFee[] memory _disputeResolverFees,


Why does _disputeResolverFees change from calldata to memory?

I changed it because at first I was setting feeAmount = 0 but I forgot to put it back to calldata after we decided to revert instead of set to 0. Fixed

cliffhall · 2022-11-15T20:01:20Z

contracts/protocol/facets/DisputeResolverHandlerFacet.sol

     *
     * @param _disputeResolverId - id of the dispute resolver
     * @param _disputeResolverFees - list of fees dispute resolver charges per token type. Zero address is native currency. See {BosonTypes.DisputeResolverFee}
+                                     feeAmount will be ignored because protocol doesn't yet support DR fees but DR still needs to provide array of fees to choose supported tokens


See the comment in the IBosonAccountHandler interface 👆

test/protocol/OfferHandlerTest.js

cliffhall · 2022-11-15T20:19:03Z

test/protocol/OrchestrationHandlerTest.js

@@ -253,13 +253,13 @@ describe("IBosonOrchestrationHandler", function () {
        adminDR.address,
        clerkDR.address,
        treasuryDR.address,
-        false
+        true
      );
      expect(disputeResolver.isValid()).is.true;

      //Create DisputeResolverFee array so offer creation will succeed


I know it was pre-existing, but could you also fix this while you're at it? I've noticed these are randomly sprinkled around, we should fix as we go.
~~//Create~~ // Create

test/protocol/OrchestrationHandlerTest.js

contracts/interfaces/handlers/IBosonAccountHandler.sol

zajck · 2022-11-16T10:42:16Z

contracts/protocol/bases/OfferBase.sol

@@ -177,15 +173,16 @@ contract OfferBase is ProtocolBase, IBosonOfferEvents {
                uint256 feeIndex = lookups.disputeResolverFeeTokenIndex[_disputeResolverId][_offer.exchangeToken];
                require(feeIndex > 0, DR_UNSUPPORTED_FEE);

-                uint256 feeAmount = disputeResolverFees[feeIndex - 1].feeAmount;
+                // uint256 feeAmount = disputeResolverFees[feeIndex - 1].feeAmount;


I believe that we should simply keep this in. Yes, it is a redundant operation, since feeAmount will be always zero (which is effectively the same as it is now). But if we comment this out and want that the change is actually applied to diamon, we need to upgrade both OfferHandler and OrchestrationHandler.

Then when we finally enable non-zero fee amount, we'd need to do the upgrade again (or at least do the cuts to point back to the current implementation).

scripts/util/create-DR.js

scripts/util/estimate-limits.js

cliffhall · 2022-11-16T16:01:59Z

scripts/util/create-DR.js

@@ -44,7 +43,7 @@ const addressNotFound = (address) => {
  process.exit(1);
 };

-const createAndActivateDR = async (path, createOnly, activateOnly) => {
+const createDR = async (path) => {


Since this method name is shorter now, I believe we should call it createDisputeResolver. We don't want to abbreviate unless we have to.

cliffhall

LGTM! 👍

anajuliabit added 3 commits November 11, 2022 13:42

Allow DR to be actived when creating and disable fees functionality

779a918

Fix broken uni tests on DispoteResolverHandler

b6243b3

Fix broken unit tests

b18dda8

anajuliabit changed the title ~~Dr actived~~ Allow DR to be activated when creating Nov 15, 2022

Fix broken tests on DisputeHandlerTest

b450bc3

anajuliabit force-pushed the dr_actived branch from 5f96245 to b450bc3 Compare November 15, 2022 13:29

Fixing broken scripts

4cb5e71

anajuliabit force-pushed the dr_actived branch from 887c838 to 4cb5e71 Compare November 15, 2022 18:10

anajuliabit changed the title ~~Allow DR to be activated when creating~~ Allows DR creation without the need for activation by protocol admin Nov 15, 2022

anajuliabit added 2 commits November 15, 2022 15:15

Merge branch 'main' into dr_actived

d2e0d2c

Add missing unit tests to DisputeResolverHandler

c83649f

anajuliabit marked this pull request as ready for review November 15, 2022 18:19

anajuliabit requested review from hswopeams, zajck and cliffhall November 15, 2022 18:20

anajuliabit self-assigned this Nov 15, 2022

Run fix natspec interfaceId

480fe12

anajuliabit mentioned this pull request Nov 15, 2022

Script to verify upgrade did not corrupt old contract state #465

Merged

7 tasks

Fix integration tests

a10106f

cliffhall suggested changes Nov 15, 2022

View reviewed changes

zajck requested changes Nov 16, 2022

View reviewed changes

Review changes

1786133

anajuliabit requested review from zajck and cliffhall November 16, 2022 12:08

hswopeams suggested changes Nov 16, 2022

View reviewed changes

scripts/util/create-DR.js Outdated Show resolved Hide resolved

scripts/util/estimate-limits.js Show resolved Hide resolved

Fix estimate and create DR scripts

cb3a3b0

anajuliabit requested a review from hswopeams November 16, 2022 15:05

zajck previously approved these changes Nov 16, 2022

View reviewed changes

cliffhall suggested changes Nov 16, 2022

View reviewed changes

hswopeams previously approved these changes Nov 16, 2022

View reviewed changes

Rename create dispute resolver script file

bd3f82f

anajuliabit requested a review from cliffhall November 17, 2022 13:59

anajuliabit dismissed stale reviews from hswopeams and zajck via bd3f82f November 17, 2022 13:59

hswopeams approved these changes Nov 17, 2022

View reviewed changes

zajck approved these changes Nov 17, 2022

View reviewed changes

cliffhall approved these changes Nov 17, 2022

View reviewed changes

cliffhall merged commit 5489ea6 into main Nov 17, 2022

zajck deleted the dr_actived branch November 17, 2022 16:23

anajuliabit mentioned this pull request Dec 30, 2022

Add tests upgrade to v2.2.0 #498

Merged

38 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows DR creation without the need for activation by protocol admin #466

Allows DR creation without the need for activation by protocol admin #466

anajuliabit commented Nov 14, 2022 •

edited

Loading

cliffhall left a comment

cliffhall Nov 15, 2022

anajuliabit Nov 16, 2022

cliffhall Nov 15, 2022

zajck Nov 16, 2022

anajuliabit Nov 16, 2022

cliffhall Nov 15, 2022

anajuliabit Nov 16, 2022

cliffhall Nov 15, 2022

anajuliabit Nov 16, 2022 •

edited

Loading

cliffhall Nov 15, 2022

anajuliabit Nov 16, 2022

cliffhall Nov 15, 2022

anajuliabit Nov 16, 2022

zajck Nov 16, 2022

cliffhall Nov 16, 2022

cliffhall left a comment

Allows DR creation without the need for activation by protocol admin #466

Allows DR creation without the need for activation by protocol admin #466

Conversation

anajuliabit commented Nov 14, 2022 • edited Loading

Contracts changes:

Scripts changes

Unit tests

cliffhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anajuliabit Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliffhall left a comment

Choose a reason for hiding this comment

anajuliabit commented Nov 14, 2022 •

edited

Loading

anajuliabit Nov 16, 2022 •

edited

Loading