Skip to content

Commit

Permalink
Update the withdraw method on Market contract to allow CONSIGNOR
Browse files Browse the repository at this point in the history
…withdrawals (#736)

* update existing tests

* test, snapshot, docgen

* review comments

---------

Co-authored-by: amiecorso <amie@nori.com>
  • Loading branch information
amiecorso and amiecorso authored Mar 13, 2024
1 parent 78ca153 commit 61a042c
Show file tree
Hide file tree
Showing 5 changed files with 175 additions and 91 deletions.
97 changes: 49 additions & 48 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -20,46 +20,46 @@ Certificate_supportsInterface:test() (gas: 5159)
Certificate_transferFrom:test() (gas: 48618)
Certificate_transferFrom:test_reverts_when_paused() (gas: 38088)
Certificate_transferFrom_reverts_ForbiddenTransferAfterMinting:test() (gas: 18960)
Checkout_buyingFromOneRemoval:test() (gas: 470314)
Checkout_buyingFromOneRemoval_byApproval:test() (gas: 408705)
Checkout_buyingFromTenRemovals:test() (gas: 1549359)
Checkout_buyingFromTenRemovals_singleSupplier:test() (gas: 1309225)
Checkout_buyingFromTenRemovals_singleSupplier_withoutFee:test() (gas: 1304425)
Checkout_buyingFromTenRemovals_withoutFee:test() (gas: 1317176)
Checkout_buyingFromTenSuppliers:test() (gas: 1818158)
Checkout_buyingWithAlternateERC20:test() (gas: 540964)
Checkout_buyingWithAlternateERC20_floatingPointPriceMultiple:test() (gas: 506930)
Checkout_swapRevertsWhenBuyerIsMissingSANCTION_ALLOWLIST_ROLE:test() (gas: 168693)
Checkout_swapRevertsWithDifferentPermitSignerAndMsgSender:test() (gas: 168705)
Checkout_swapWithoutFeeSpecialOrder:test() (gas: 380597)
Checkout_swapWithoutFeeSpecialOrder_specificSupplier:test() (gas: 375252)
Checkout_buyingFromOneRemoval:test() (gas: 470402)
Checkout_buyingFromOneRemoval_byApproval:test() (gas: 408749)
Checkout_buyingFromTenRemovals:test() (gas: 1549843)
Checkout_buyingFromTenRemovals_singleSupplier:test() (gas: 1309445)
Checkout_buyingFromTenRemovals_singleSupplier_withoutFee:test() (gas: 1304645)
Checkout_buyingFromTenRemovals_withoutFee:test() (gas: 1317396)
Checkout_buyingFromTenSuppliers:test() (gas: 1818545)
Checkout_buyingWithAlternateERC20:test() (gas: 541052)
Checkout_buyingWithAlternateERC20_floatingPointPriceMultiple:test() (gas: 507018)
Checkout_swapRevertsWhenBuyerIsMissingSANCTION_ALLOWLIST_ROLE:test() (gas: 168737)
Checkout_swapRevertsWithDifferentPermitSignerAndMsgSender:test() (gas: 168749)
Checkout_swapWithoutFeeSpecialOrder:test() (gas: 380619)
Checkout_swapWithoutFeeSpecialOrder_specificSupplier:test() (gas: 375274)
Checkout_swapWithoutFeeSpecialOrder_specificSupplier:test_revertsWhenSupplierDoesNotExistInMarket() (gas: 57753)
Checkout_swapWithoutFeeSpecialOrder_specificVintages:test_basicFulfillment() (gas: 642423)
Checkout_swapWithoutFeeSpecialOrder_specificVintages:test_basicFulfillment() (gas: 642489)
Checkout_swapWithoutFeeSpecialOrder_specificVintages:test_revertsWhenNoRemovalsFromSpecifiedVintages() (gas: 91044)
Checkout_swapWithoutFeeSpecialOrder_specificVintagesSpecificSupplier:test_basicFulfillment() (gas: 491863)
GasBenchmark_buyingFromManyRemovals_singleSupplier:test() (gas: 29847653)
GasBenchmark_buyingFromOneRemoval_singleSupplier:test() (gas: 322867)
Checkout_swapWithoutFeeSpecialOrder_specificVintagesSpecificSupplier:test_basicFulfillment() (gas: 491903)
GasBenchmark_buyingFromManyRemovals_singleSupplier:test() (gas: 29853285)
GasBenchmark_buyingFromOneRemoval_singleSupplier:test() (gas: 322889)
LockedNORILib_availableAmount:test() (gas: 12371)
LockedNORITest:testBatchGrantCreation() (gas: 705179)
LockedNORITest:testNormalWithdrawal() (gas: 1102743)
LockedNORITest:testReentryTokensReceived() (gas: 1102887)
LockedNORITest:testReentryTokensToSend() (gas: 1104432)
LockedNORITest:testTokensReceivedReverts() (gas: 69026)
MarketInvariantTest:invariant_callSummary() (runs: 400, calls: 6000, reverts: 4285)
MarketInvariantTest:invariant_sumOfPurchaseAmounts() (runs: 400, calls: 6000, reverts: 4268)
MarketSupplierSelectionNotUsingUpSuppliersLastRemoval:test() (gas: 644612)
MarketInvariantTest:invariant_callSummary() (runs: 400, calls: 6000, reverts: 4265)
MarketInvariantTest:invariant_sumOfPurchaseAmounts() (runs: 400, calls: 6000, reverts: 4261)
MarketSupplierSelectionNotUsingUpSuppliersLastRemoval:test() (gas: 644744)
Market_ALLOWLIST_ROLE:test() (gas: 12799)
Market_SANCTION_ALLOWLIST_ROLE:test() (gas: 12897)
Market_USDC_swap_respects_decimal_mismatch:test() (gas: 786916)
Market_USDC_swap_respects_decimal_mismatch:test() (gas: 787070)
Market__addActiveRemoval:test() (gas: 183344)
Market__addActiveRemoval:test__lis2VintagesFor1SupplierFor2SubIdentifiers() (gas: 242879)
Market__addActiveRemoval:test__list1VintageFor1Supplier() (gas: 188309)
Market__addActiveRemoval:test__list1VintageFor2Suppliers() (gas: 360670)
Market__addActiveRemoval:test__list2VintagesFor1SupplierFor1SubIdentifier() (gas: 262754)
Market__isAuthorizedWithdrawal_false:test_returnsFalseWhenAllConditionsAreFalse() (gas: 7452)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderEqualsOwner() (gas: 377)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderHasDefaultAdminRole() (gas: 96578)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderIsApprovedForAll() (gas: 8923)
Market__addActiveRemoval:test__list2VintagesFor1SupplierFor1SubIdentifier() (gas: 262709)
Market__isAuthorizedWithdrawal_false:test_returnsFalseWhenAllConditionsAreFalse() (gas: 25767)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderEqualsOwner() (gas: 17086)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderHasDefaultAdminRole() (gas: 113243)
Market__isAuthorizedWithdrawal_true:test_returnsTrueWhenMsgSenderIsApprovedForAll() (gas: 23245)
Market__multicall_empty_bytes_reverts:test() (gas: 20935)
Market__multicall_initialize_reverts:test() (gas: 33859)
Market__setPriceMultiple:test() (gas: 29132)
Expand All @@ -72,23 +72,23 @@ Market__validatePrioritySupply_buyerIsAllowlistedAndAmountExceedsPriorityRestric
Market__validatePrioritySupply_reverts_LowSupplyAllowlistRequired:test() (gas: 7692)
Market__validateSupply:test() (gas: 292)
Market__validateSupply:test_reverts_OutOfSupply() (gas: 3172)
Market_calculates_prices_using_decimal:test() (gas: 66514)
Market_calculates_prices_using_decimal:test() (gas: 66624)
Market_convertPurchasingTokenDecimalsToRemovalDecimals:test() (gas: 26029)
Market_convertRemovalDecimalsToPurchasingTokenDecimals:test() (gas: 29773)
Market_convertRemovalDecimalsToPurchasingTokenDecimals:test() (gas: 29817)
Market_getActiveSuppliers:test_1_supplier() (gas: 447828)
Market_getActiveSuppliers:test_3_suppliers() (gas: 1088942)
Market_getActiveSuppliers:test_no_suppliers() (gas: 20859)
Market_getPriceMultiple:test() (gas: 14851)
Market_getRemovalIdsForSupplier:test_1_removal() (gas: 448231)
Market_getRemovalIdsForSupplier:test_3_removals() (gas: 831646)
Market_getRemovalIdsForSupplier:test_3_removals_different_vintages() (gas: 877755)
Market_getRemovalIdsForSupplier:test_no_removals() (gas: 26044)
Market_getRemovalIdsForSupplier:test_1_removal() (gas: 448142)
Market_getRemovalIdsForSupplier:test_3_removals() (gas: 831557)
Market_getRemovalIdsForSupplier:test_3_removals_different_vintages() (gas: 877666)
Market_getRemovalIdsForSupplier:test_no_removals() (gas: 25955)
Market_onERC1155BatchReceived:test() (gas: 208124)
Market_onERC1155BatchReceived_reverts_SenderNotRemovalContract:test() (gas: 331570)
Market_onERC1155Received:test() (gas: 206036)
Market_onERC1155Received_reverts_SenderNotRemovalContract:test() (gas: 159044)
Market_purchasingTokenAddress:test() (gas: 17080)
Market_replace:test() (gas: 277991)
Market_purchasingTokenAddress:test() (gas: 17102)
Market_replace:test() (gas: 278013)
Market_replace_reverts_CertificateNotYetMinted:test() (gas: 49559)
Market_replace_reverts_ReplacementAmountExceedsNrtDeficit:test() (gas: 52590)
Market_replace_reverts_ReplacementAmountMismatch:test() (gas: 86397)
Expand All @@ -97,22 +97,23 @@ Market_setPriorityRestrictedThreshold:test() (gas: 157448)
Market_setPriorityRestrictedThreshold:test_zeroAvailable() (gas: 152423)
Market_setPurchasingTokenAndPriceMultiple:test() (gas: 1026641)
Market_setPurchasingTokenAndPriceMultiple_revertsIfNotAdmin:test() (gas: 50813)
Market_supplierSelectionUsingUpSuppliersLastRemoval:test() (gas: 641321)
Market_swap_revertsWhenUnsafeERC20TransferFails:test() (gas: 189648)
Market_supplierSelectionUsingUpSuppliersLastRemoval:test() (gas: 641453)
Market_swap_revertsWhenUnsafeERC20TransferFails:test() (gas: 189736)
Market_validates_certificate_amount:test() (gas: 596800)
Market_withdraw_1x3_center:test() (gas: 340814)
Market_withdraw_2x1_back:test() (gas: 345496)
Market_withdraw_2x1_front:test() (gas: 333831)
Market_withdraw_1x3_center:test() (gas: 346739)
Market_withdraw_2x1_back:test() (gas: 351412)
Market_withdraw_2x1_front:test() (gas: 339756)
Market_withdraw_2x1_front_relist:test() (gas: 381788)
Market_withdraw_as_DEFAULT_ADMIN_ROLE:test() (gas: 276524)
Market_withdraw_as_operator:test() (gas: 285669)
Market_withdraw_as_supplier:test() (gas: 274665)
Market_withdraw_reverts:test() (gas: 138709)
Market_withdraw_as_DEFAULT_ADMIN_ROLE:test() (gas: 282449)
Market_withdraw_as_operator:test() (gas: 291594)
Market_withdraw_as_supplier:test() (gas: 280590)
Market_withdraw_reverts:test() (gas: 144618)
Market_withdraw_to_CONSIGNOR_ROLE:test() (gas: 284393)
NORI_name:test() (gas: 17205)
NORI_permit:test() (gas: 92382)
NoriUSDC_permit:test() (gas: 122061)
RemovalQueue_getTotalBalanceFromRemovalQueue:test() (gas: 23921)
RemovalQueue_getTotalBalanceFromRemovalQueue:test_100xRemovalsOfTheDifferentVintages() (gas: 895808)
RemovalQueue_getTotalBalanceFromRemovalQueue:test_100xRemovalsOfTheDifferentVintages() (gas: 895830)
RemovalQueue_getTotalBalanceFromRemovalQueue:test_100xRemovalsOfTheSameVintage() (gas: 620321)
RemovalQueue_insertRemovalByVintage:test_insertRemovalOnce() (gas: 119613)
RemovalQueue_insertRemovalByVintage:test_insertRemovalTwice() (gas: 121103)
Expand All @@ -123,7 +124,7 @@ Removal__createRemovalData:test_reverts_InvalidData() (gas: 25711)
Removal__createRemovalDataBatch:test() (gas: 29594)
Removal__createRemovalDataBatch:test_reverts_InvalidData2() (gas: 36802)
Removal__isValidTransferAmount:testFuzz_ReturnFalse_NonMultiplesOf1e14(uint256) (runs: 256, μ: 13935, ~: 13891)
Removal__isValidTransferAmount:testFuzz_ReturnTrue_MultiplesOf1e14(uint256) (runs: 256, μ: 14315, ~: 14432)
Removal__isValidTransferAmount:testFuzz_ReturnTrue_MultiplesOf1e14(uint256) (runs: 256, μ: 14307, ~: 14432)
Removal__isValidTransferAmount:testFuzz_ReturnTrue_SmallestGranularity() (gas: 6876)
Removal__isValidTransferAmount:test_ReturnFalse_AmountIsTooGranular() (gas: 6854)
Removal__isValidTransferAmount:test_ReturnFalse_AmountIsTooGranularAndToIsTheCertificate() (gas: 4789)
Expand All @@ -135,11 +136,11 @@ Removal__validateRemoval:test() (gas: 2491)
Removal__validateRemoval:test_reverts_InvalidData() (gas: 5373)
Removal_addBalance:test() (gas: 60280)
Removal_addBalance_reverts_RemovalNotYetMinted:test() (gas: 31115)
Removal_consign_revertsForSoldRemovals:test() (gas: 896622)
Removal_consign_revertsForSoldRemovals:test() (gas: 896728)
Removal_consignorBatchTransfer:test() (gas: 297325)
Removal_consignorBatchTransfer:test_reverts_whenReceiverIsNotConsignor() (gas: 128791)
Removal_consignorBatchTransfer:test_reverts_whenSenderIsNotConsignor() (gas: 65646)
Removal_getMarketBalance:test() (gas: 907046)
Removal_getMarketBalance:test() (gas: 910292)
Removal_getOwnedTokenIds:test_multiple_tokens_with_transfer() (gas: 916416)
Removal_getOwnedTokenIds:test_no_tokens() (gas: 18683)
Removal_getProjectId:test() (gas: 19307)
Expand All @@ -157,7 +158,7 @@ Removal_mintBatch_zero_amount_removal:test() (gas: 139898)
Removal_mintBatch_zero_amount_removal_to_market_reverts:test() (gas: 61595)
Removal_multicall:test_balanceOfBatch() (gas: 320743)
Removal_release_listed:test() (gas: 356643)
Removal_release_listed_isRemovedFromMarket:test() (gas: 356996)
Removal_release_listed_isRemovedFromMarket:test() (gas: 356854)
Removal_release_partial_listed:test() (gas: 79593)
Removal_release_retired:test() (gas: 92447)
Removal_release_retired_2x:test() (gas: 98511)
Expand Down
37 changes: 25 additions & 12 deletions contracts/Market.sol
Original file line number Diff line number Diff line change
Expand Up @@ -781,19 +781,22 @@ contract Market is
}

/**
* @notice Withdraws a removal to the supplier.
* @dev Withdraws a removal to the supplier address encoded in the removal ID.
* @notice Withdraws a removal from the Market.
* @dev Withdraws a removal to the specified address, which must be the supplier of the removal
* or have the `Removal.CONSIGNOR_ROLE`.
*
* ##### Requirements:
*
* - Can only be used when this contract is not paused.
* - Can only withdraw to the supplier of the removal or an address with the `Removal.CONSIGNOR_ROLE`.
* @param removalId The ID of the removal to withdraw from the market.
* @param to The address to which the removal will be transferred.
*/
function withdraw(uint256 removalId) external whenNotPaused {
function withdraw(uint256 removalId, address to) external whenNotPaused {
address supplierAddress = RemovalIdLib.supplierAddress({
removalId: removalId
});
if (_isAuthorizedWithdrawal({owner: supplierAddress})) {
if (_isAuthorizedWithdrawal({supplier: supplierAddress, to: to})) {
_removeActiveRemoval({
removalId: removalId,
supplierAddress: supplierAddress
Expand Down Expand Up @@ -1349,16 +1352,26 @@ contract Market is
}

/**
* @dev Authorizes withdrawal for the removal. Reverts if the caller is not the owner of the removal and
* does not have the role `MARKET_ADMIN_ROLE`.
* @param owner The owner of the removal.
* @return Returns true if the caller is the owner, an approved spender, or has the role `MARKET_ADMIN_ROLE`,
* false otherwise.
* @dev Authorizes withdrawal for the removal. Reverts if the caller is not the supplier of the removal and
* does not have the role `MARKET_ADMIN_ROLE`, or if the recipient of the removal is either not the supplier
* of the removal or does not have the `Removal.CONSIGNOR_ROLE`.
* @param supplier The supplier of the removal.
* @param to The recipient of the removal.
* @return Returns true if the caller is the supplier, an approved spender, or has the role `MARKET_ADMIN_ROLE`,
* and the recipient is the removal supplier or has the `Removal.CONSIGNOR_ROLE`, false otherwise.
*/
function _isAuthorizedWithdrawal(address owner) internal view returns (bool) {
return (_msgSender() == owner ||
function _isAuthorizedWithdrawal(
address supplier,
address to
) internal view returns (bool) {
bool isApprovedCaller = _msgSender() == supplier ||
hasRole({role: MARKET_ADMIN_ROLE, account: _msgSender()}) ||
_removal.isApprovedForAll({account: owner, operator: _msgSender()}));
_removal.isApprovedForAll({account: supplier, operator: _msgSender()});
bool isApprovedRecipient = _removal.hasRole({
role: _removal.CONSIGNOR_ROLE(),
account: to
}) || supplier == to;
return isApprovedCaller && isApprovedRecipient;
}

/**
Expand Down
23 changes: 14 additions & 9 deletions docs/Market.md
Original file line number Diff line number Diff line change
Expand Up @@ -671,20 +671,23 @@ contract to the specified recipient and the ERC20 is distributed to the supplier
### withdraw

```solidity
function withdraw(uint256 removalId) external
function withdraw(uint256 removalId, address to) external
```

Withdraws a removal to the supplier.
Withdraws a removal from the Market.

<i>Withdraws a removal to the supplier address encoded in the removal ID.
<i>Withdraws a removal to the specified address, which must be the supplier of the removal
or have the `Removal.CONSIGNOR_ROLE`.

##### Requirements:

- Can only be used when this contract is not paused.</i>
- Can only be used when this contract is not paused.
- Can only withdraw to the supplier of the removal or an address with the `Removal.CONSIGNOR_ROLE`.</i>

| Name | Type | Description |
| ---- | ---- | ----------- |
| removalId | uint256 | The ID of the removal to withdraw from the market. |
| to | address | The address to which the removal will be transferred. |


### getPriceMultiple
Expand Down Expand Up @@ -1165,20 +1168,22 @@ Validates that the listed supply is enough to fulfill the purchase given the pri
### _isAuthorizedWithdrawal

```solidity
function _isAuthorizedWithdrawal(address owner) internal view returns (bool)
function _isAuthorizedWithdrawal(address supplier, address to) internal view returns (bool)
```


<i>Authorizes withdrawal for the removal. Reverts if the caller is not the owner of the removal and
does not have the role `MARKET_ADMIN_ROLE`.</i>
<i>Authorizes withdrawal for the removal. Reverts if the caller is not the supplier of the removal and
does not have the role `MARKET_ADMIN_ROLE`, or if the recipient of the removal is either not the supplier
of the removal or does not have the `Removal.CONSIGNOR_ROLE`.</i>

| Name | Type | Description |
| ---- | ---- | ----------- |
| owner | address | The owner of the removal. |
| supplier | address | The supplier of the removal. |
| to | address | The recipient of the removal. |

| Name | Type | Description |
| ---- | ---- | ----------- |
| [0] | bool | Returns true if the caller is the owner, an approved spender, or has the role &#x60;MARKET_ADMIN_ROLE&#x60;, false otherwise. |
| [0] | bool | Returns true if the caller is the supplier, an approved spender, or has the role &#x60;MARKET_ADMIN_ROLE&#x60;, and the recipient is the removal supplier or has the &#x60;Removal.CONSIGNOR_ROLE&#x60;, false otherwise. |

### _validateReplacementAmounts

Expand Down
Loading

0 comments on commit 61a042c

Please sign in to comment.