test: enhancing tests #64
Conversation
kupermind
commented
Dec 24, 2024
kupermind
commented
- Enhancing tests.
| if (balance == 0) { | ||
| revert ZeroValue(); | ||
| } | ||
|
|
||
| // Calculate mech payment and marketplace fee | ||
| uint256 fee = IMechMarketplace(mechMarketplace).fee(); | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
|
|
||
| // If requested balance is too small, charge the minimal fee | ||
| if (balance < FEE_BASE) { | ||
| marketplaceFee = 1; | ||
| } else { | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
| } |
There was a problem hiding this comment.
@DavidMinarsch will this work as a small amounts withdraw protection?
There was a problem hiding this comment.
Suggest this instead:
marketplaceFee = (balance * fee + (FEE_BASE - 1)) / FEE_BASE;
This uses
\text{ceil}(a, b) = \frac{a + b - 1}{b}
formula
There was a problem hiding this comment.
It will always return at least one for balance >0.
| @@ -314,6 +317,11 @@ contract MechMarketplace is IErrorsMarketplace { | |||
|
|
|||
| // Traverse all the mech types and balanceTrackers | |||
| for (uint256 i = 0; i < paymentTypes.length; ++i) { | |||
| // Check for zero value | |||
| if (paymentTypes[i] == 0) { | |||
There was a problem hiding this comment.
Need to check zero values here
| // Response timeout bounds | ||
| if (responseTimeout < minResponseTimeout || responseTimeout > maxResponseTimeout) { | ||
| revert OutOfBounds(responseTimeout, minResponseTimeout, maxResponseTimeout); | ||
| } | ||
|
|
There was a problem hiding this comment.
No change, just swapped places
| // Check if request has been delivered | ||
| if (mechDelivery.deliveryMech != address(0)) { | ||
| revert AlreadyDelivered(requestId); | ||
| } |
There was a problem hiding this comment.
Currently this never happens, but let's leave it here for possible mechs change.
| function checkMech(address mech) public view returns (address multisig) { | ||
| uint256 mechServiceId = IMech(mech).tokenId(); | ||
|
|
||
| // Check mech validity as it must be created and recorded via this marketplace | ||
| if (mapServiceIdMech[mechServiceId] != mech) { | ||
| revert UnauthorizedAccount(mech); | ||
| } | ||
|
|
||
| // Check mech service Id | ||
| multisig = checkServiceAndGetMultisig(mechServiceId); | ||
|
|
||
| // Check that service multisig is the priority mech service multisig | ||
| if (!IMech(mech).isOperator(multisig)) { | ||
| revert UnauthorizedAccount(mech); | ||
| } | ||
| // Check mech service Id and get its multisig | ||
| multisig = IMech(mech).getOperator(); |
There was a problem hiding this comment.
This is now more than enough, the rest was code duplication.
| enum RequestStatus { | ||
| DoesNotExist, | ||
| Requested, | ||
| Delivered | ||
| } |
There was a problem hiding this comment.
Leftovers from previous versions without MM
| // Domain separator type hash | ||
| bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH = | ||
| keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); | ||
| // Original domain separator value | ||
| bytes32 public immutable domainSeparator; | ||
| // Original chain Id | ||
| uint256 public immutable chainId; |
There was a problem hiding this comment.
Leftovers from previous versions without MM
| /// @dev Gets mech operator (service multisig). | ||
| /// @return Service multisig address. | ||
| function getOperator() public view returns (address) { | ||
| // Get service registry and service Id | ||
| (address serviceRegistry, uint256 serviceId) = abi.decode(readImmutable(), (address, uint256)); | ||
|
|
||
| (, address multisig, , , , , IServiceRegistry.ServiceState state) = |
There was a problem hiding this comment.
This is cleaner now, also there must be a possibility to directly get the multisig without doing all the service registry mess.
| /// @param data Self-descriptive opaque data-blob. | ||
| /// @param nonce Nonce. | ||
| /// @return requestId Corresponding request Id. | ||
| function getRequestId( |
There was a problem hiding this comment.
Leftovers from previous versions without MM
| /// @notice If marketplace is not zero, use the same function in the mech marketplace contract. | ||
| /// @param requestId Request Id. | ||
| /// @return status Request status. | ||
| function getRequestStatus(uint256 requestId) external view returns (RequestStatus status) { |
There was a problem hiding this comment.
Leftovers from previous versions without MM
| if (balance < FEE_BASE) { | ||
| marketplaceFee = 1; |
There was a problem hiding this comment.
what if balance < marketplaceFee? need to fail..
There was a problem hiding this comment.
This can't happen as balance is not zero at this point of time. But that's irrelevant already.
| if (balance == 0) { | ||
| revert ZeroValue(); | ||
| } | ||
|
|
||
| // Calculate mech payment and marketplace fee | ||
| uint256 fee = IMechMarketplace(mechMarketplace).fee(); | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
|
|
||
| // If requested balance is too small, charge the minimal fee | ||
| if (balance < FEE_BASE) { | ||
| marketplaceFee = 1; | ||
| } else { | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
| } |
There was a problem hiding this comment.
Suggest this instead:
marketplaceFee = (balance * fee + (FEE_BASE - 1)) / FEE_BASE;
This uses
\text{ceil}(a, b) = \frac{a + b - 1}{b}
formula
| if (balance == 0) { | ||
| revert ZeroValue(); | ||
| } | ||
|
|
||
| // Calculate mech payment and marketplace fee | ||
| uint256 fee = IMechMarketplace(mechMarketplace).fee(); | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
|
|
||
| // If requested balance is too small, charge the minimal fee | ||
| if (balance < FEE_BASE) { | ||
| marketplaceFee = 1; | ||
| } else { | ||
| marketplaceFee = (balance * fee) / FEE_BASE; | ||
| } |
There was a problem hiding this comment.
It will always return at least one for balance >0.
