From 6e6e87ca64ba393e75788375103cdfe38519f434 Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Tue, 24 Dec 2024 20:15:40 +0000 Subject: [PATCH 1/6] test: enhancing tests --- .solhint.json | 2 +- contracts/BalanceTrackerFixedPriceBase.sol | 13 +- contracts/MechMarketplace.sol | 58 +++-- contracts/OlasMech.sol | 176 +++++---------- contracts/interfaces/IErrorsMech.sol | 3 + contracts/interfaces/IMech.sol | 18 +- test/MechFixedPriceNative.js | 90 +++++--- test/MechMarketplace.js | 242 +++++++++++++++++++++ 8 files changed, 414 insertions(+), 188 deletions(-) create mode 100644 test/MechMarketplace.js diff --git a/.solhint.json b/.solhint.json index 1c5e674..8c3dc23 100644 --- a/.solhint.json +++ b/.solhint.json @@ -2,7 +2,7 @@ "extends": "solhint:recommended", "plugins": [], "rules": { - "compiler-version": ["warn", "^0.8.25"], + "compiler-version": ["warn", "^0.8.28"], "func-visibility": ["warn",{"ignoreConstructors":true}], "no-empty-blocks": "off", "not-rely-on-time": "off", diff --git a/contracts/BalanceTrackerFixedPriceBase.sol b/contracts/BalanceTrackerFixedPriceBase.sol index d177c84..f13fa41 100644 --- a/contracts/BalanceTrackerFixedPriceBase.sol +++ b/contracts/BalanceTrackerFixedPriceBase.sol @@ -202,14 +202,19 @@ abstract contract BalanceTrackerFixedPriceBase { // Get mech balance uint256 balance = mapMechBalances[msg.sender]; - // TODO minimal balance value to account for the round-off - if (balance == 0 || balance < 10_000) { - revert InsufficientBalance(balance, 10_000); + 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; + } mechPayment = balance - marketplaceFee; // Check for zero value, although this must never happen diff --git a/contracts/MechMarketplace.sol b/contracts/MechMarketplace.sol index b679243..56f40ed 100644 --- a/contracts/MechMarketplace.sol +++ b/contracts/MechMarketplace.sol @@ -61,6 +61,9 @@ contract MechMarketplace is IErrorsMarketplace { // Domain separator type hash bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + // Fee base constant + uint256 public constant FEE_BASE = 10_000; + // Original domain separator value bytes32 public immutable domainSeparator; // Original chain Id @@ -157,8 +160,8 @@ contract MechMarketplace is IErrorsMarketplace { } // Check for fee value - if (newFee > 10_000) { - revert Overflow(newFee, 10_000); + if (newFee > FEE_BASE) { + revert Overflow(newFee, FEE_BASE); } // Check for sanity values @@ -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) { + revert ZeroValue(); + } + // Check for zero address if (balanceTrackers[i] == address(0)) { revert ZeroAddress(); @@ -346,15 +354,16 @@ contract MechMarketplace is IErrorsMarketplace { } _locked = 2; - // responseTimeout bounds - if (responseTimeout < minResponseTimeout || responseTimeout > maxResponseTimeout) { - revert OutOfBounds(responseTimeout, minResponseTimeout, maxResponseTimeout); - } - // responseTimeout limits + // Response timeout limits if (responseTimeout + block.timestamp > type(uint32).max) { revert Overflow(responseTimeout + block.timestamp, type(uint32).max); } + // Response timeout bounds + if (responseTimeout < minResponseTimeout || responseTimeout > maxResponseTimeout) { + revert OutOfBounds(responseTimeout, minResponseTimeout, maxResponseTimeout); + } + // Check for non-zero data if (data.length == 0) { revert ZeroValue(); @@ -438,7 +447,7 @@ contract MechMarketplace is IErrorsMarketplace { revert ZeroAddress(); } - // Check that the request is not already delivered + // Check if request has been delivered if (mechDelivery.deliveryMech != address(0)) { revert AlreadyDelivered(requestId); } @@ -517,24 +526,10 @@ contract MechMarketplace is IErrorsMarketplace { )); } - /// @dev Checks for service validity. - /// @param serviceId Service Id. - /// @return multisig Service multisig address. - function checkServiceAndGetMultisig( - uint256 serviceId - ) public view returns (address multisig) { - // Check mech service Id - IServiceRegistry.ServiceState state; - (, multisig, , , , , state) = IServiceRegistry(serviceRegistry).mapServices(serviceId); - if (state != IServiceRegistry.ServiceState.Deployed) { - revert WrongServiceState(uint256(state), serviceId); - } - } - /// @dev Checks for mech validity. /// @param mech Mech contract address. /// @return multisig Mech service multisig address. - function checkMech(address mech) public view returns (address multisig){ + 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 @@ -542,13 +537,8 @@ contract MechMarketplace is IErrorsMarketplace { 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(); } /// @dev Checks for requester validity. @@ -561,7 +551,13 @@ contract MechMarketplace is IErrorsMarketplace { ) public view { // Check for requester service if (requesterServiceId > 0) { - address multisig = checkServiceAndGetMultisig(requesterServiceId); + (, address multisig, , , , , IServiceRegistry.ServiceState state) = + IServiceRegistry(serviceRegistry).mapServices(requesterServiceId); + + // Check for correct service state + if (state != IServiceRegistry.ServiceState.Deployed) { + revert WrongServiceState(uint256(state), requesterServiceId); + } // Check staked service multisig if (multisig != requester) { diff --git a/contracts/OlasMech.sol b/contracts/OlasMech.sol index 1fb913f..1497bc6 100644 --- a/contracts/OlasMech.sol +++ b/contracts/OlasMech.sol @@ -14,21 +14,8 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { event Request(address indexed sender, uint256 requestId, bytes data); event RevokeRequest(address indexed sender, uint256 requestId); - enum RequestStatus { - DoesNotExist, - Requested, - Delivered - } - // Olas mech version number string public constant VERSION = "1.0.0"; - // 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; // Mech marketplace address address public immutable mechMarketplace; // Mech payment type @@ -99,51 +86,31 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { mechMarketplace = _mechMarketplace; maxDeliveryRate = _maxDeliveryRate; paymentType = _paymentType; - - // Record chain Id - chainId = block.chainid; - // Compute domain separator - domainSeparator = _computeDomainSeparator(); } - /// @dev Computes domain separator hash. - /// @return Hash of the domain separator based on its name, version, chain Id and contract address. - function _computeDomainSeparator() internal view returns (bytes32) { - return keccak256( - abi.encode( - DOMAIN_SEPARATOR_TYPE_HASH, - keccak256("OlasMech"), - keccak256(abi.encode(VERSION)), - block.chainid, - address(this) - ) - ); - } - - // TODO Rename account for requester /// @dev Performs actions before the delivery of a request. - /// @param account Requester address. + /// @param requester Requester address. /// @param requestId Request Id. /// @param data Self-descriptive opaque data-blob. /// @return requestData Data for the request processing. function _preDeliver( - address account, + address requester, uint256 requestId, bytes memory data ) internal virtual returns (bytes memory requestData); /// @dev Registers a request. - /// @param account Requester address. + /// @param requester Requester address. /// @param requestId Request Id. /// @param data Self-descriptive opaque data-blob. function _request( - address account, + address requester, uint256 requestId, bytes memory data ) internal virtual { - mapUndeliveredRequestsCounts[account]++; + mapUndeliveredRequestsCounts[requester]++; // Record the requestId => sender correspondence - mapRequestAddresses[requestId] = account; + mapRequestAddresses[requestId] = requester; // Record the request Id in the map // Get previous and next request Ids of the first element @@ -164,15 +131,15 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { // Increase the total number of requests numTotalRequests++; - emit Request(account, requestId, data); + emit Request(requester, requestId, data); } /// @dev Cleans the request info from all the relevant storage. - /// @param account Requester account address. + /// @param requester Requester address. /// @param requestId Request Id. - function _cleanRequestInfo(address account, uint256 requestId) internal virtual { + function _cleanRequestInfo(address requester, uint256 requestId) internal virtual { // Decrease the number of undelivered requests - mapUndeliveredRequestsCounts[account]--; + mapUndeliveredRequestsCounts[requester]--; numUndeliveredRequests--; // Remove delivered request Id from the request Ids map @@ -196,29 +163,30 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { /// @param requestId Request id. /// @param data Self-descriptive opaque data-blob. function _deliver(uint256 requestId, bytes memory data) internal virtual returns (bytes memory requestData) { - // Get an account to deliver request to - address account = mapRequestAddresses[requestId]; + // Get an requester to deliver request to + address requester = mapRequestAddresses[requestId]; // Get the mech delivery info from the mech marketplace IMechMarketplace.MechDelivery memory mechDelivery = IMechMarketplace(mechMarketplace).mapRequestIdDeliveries(requestId); - // Instantly return if the request has been delivered + // Instantly return with empty data if the request has been delivered + // This allows not to fail batch requests transactions if (mechDelivery.deliveryMech != address(0)) { - return requestData; + return ""; } - // The account is zero if the delivery mech is different from a priority mech, or if request does not exist - if (account == address(0)) { - account = mechDelivery.requester; + // The requester is zero if the delivery mech is different from a priority mech, or if request does not exist + if (requester == address(0)) { + requester = mechDelivery.requester; // Check if request exists in the mech marketplace - if (account == address(0)) { + if (requester == address(0)) { revert RequestIdNotFound(requestId); } // Note, revoking the request for the priority mech happens later via revokeRequest } else { - // The account is non-zero if it is delivered by the priority mech - _cleanRequestInfo(account, requestId); + // The requester is non-zero if it is delivered by the priority mech + _cleanRequestInfo(requester, requestId); } // Check for max delivery rate compared to requested one @@ -227,7 +195,7 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { } // Perform a pre-delivery of the data if it needs additional parsing - requestData = _preDeliver(account, requestId, data); + requestData = _preDeliver(requester, requestId, data); // Increase the total number of deliveries, as the request is delivered by this mech numTotalDeliveries++; @@ -249,17 +217,17 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { /// @dev Registers a request by a marketplace. /// @notice This function is called by the marketplace contract since this mech was specified as a priority one. - /// @param account Requester account address. + /// @param requester Requester address. /// @param data Self-descriptive opaque data-blob. /// @param requestId Request Id. - function requestFromMarketplace(address account, bytes memory data, uint256 requestId) external { + function requestFromMarketplace(address requester, bytes memory data, uint256 requestId) external { // Check for marketplace access if (msg.sender != mechMarketplace) { revert MarketplaceNotAuthorized(msg.sender); } // Perform a request - _request(account, requestId, data); + _request(requester, requestId, data); } /// @dev Revokes the request from the mech that does not deliver it. @@ -271,16 +239,16 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { revert MarketplaceNotAuthorized(msg.sender); } - address account = mapRequestAddresses[requestId]; - // This must never happen, as the priority mech recorded requestId => account info during the request - if (account == address(0)) { + address requester = mapRequestAddresses[requestId]; + // This must never happen, as the priority mech recorded requestId => requester info during the request + if (requester == address(0)) { revert ZeroAddress(); } // Clean request info - _cleanRequestInfo(account, requestId); + _cleanRequestInfo(requester, requestId); - emit RevokeRequest(account, requestId); + emit RevokeRequest(requester, requestId); } /// @dev Delivers a request by a marketplace. @@ -308,25 +276,34 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { _locked = 1; } + /// @dev Sets up a mech. + /// @param initParams Mech initial parameters. function setUp(bytes memory initParams) public override { - require(readImmutable().length == 0, "Already initialized"); + if (readImmutable().length != 0) { + revert AlreadyInitialized(); + } writeImmutable(initParams); } - function token() public view returns (address serviceRegistry) { + /// @dev Gets mech token (service registry) address. + /// @return serviceRegistry Service registry address. + function token() external view returns (address serviceRegistry) { + // Get service registry serviceRegistry = abi.decode(readImmutable(), (address)); } - function tokenId() public view returns (uint256) { - (, uint256 serviceId) = abi.decode(readImmutable(), (address, uint256)); - return serviceId; + /// @dev Gets mech token Id (service Id). + /// @return serviceId Service Id. + function tokenId() external view returns (uint256 serviceId) { + // Get service Id + (, serviceId) = abi.decode(readImmutable(), (address, uint256)); } - function isOperator(address signer) public view override returns (bool) { - (address serviceRegistry, uint256 serviceId) = abi.decode( - readImmutable(), - (address, uint256) - ); + /// @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) = IServiceRegistry(serviceRegistry).mapServices(serviceId); @@ -336,57 +313,14 @@ abstract contract OlasMech is Mech, IErrorsMech, ImmutableStorage { revert WrongServiceState(uint256(state), serviceId); } - return multisig == signer; - } - - /// @dev Gets the already computed domain separator of recomputes one if the chain Id is different. - /// @return Original or recomputed domain separator. - function getDomainSeparator() public view returns (bytes32) { - return block.chainid == chainId ? domainSeparator : _computeDomainSeparator(); - } - - /// @dev Gets the request Id. - /// @param account Account address. - /// @param data Self-descriptive opaque data-blob. - /// @param nonce Nonce. - /// @return requestId Corresponding request Id. - function getRequestId( - address account, - bytes memory data, - uint256 nonce - ) public view returns (uint256 requestId) { - requestId = uint256(keccak256( - abi.encodePacked( - "\x19\x01", - getDomainSeparator(), - keccak256( - abi.encode( - account, - data, - nonce - ) - ) - ) - )); + return multisig; } - /// @dev Gets the request Id status registered in this agent mech. - /// @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) { - // Request exists if it was recorded in the requestId => account map - if (mapRequestAddresses[requestId] != address(0)) { - // Get the request info - uint256[2] memory requestIds = mapRequestIds[requestId]; - // Check if the request Id was already delivered: previous and next request Ids are zero, - // and the zero's element previous request Id is not equal to the provided request Id - if (requestIds[0] == 0 && requestIds[1] == 0 && mapRequestIds[0][0] != requestId) { - status = RequestStatus.Delivered; - } else { - status = RequestStatus.Requested; - } - } + /// @dev Checks the mech operator (service multisig). + /// @param multisig Service multisig being checked against. + /// @return True, if mech service multisig matches the provided one. + function isOperator(address multisig) public view override returns (bool) { + return multisig == getOperator(); } /// @dev Gets the set of undelivered request Ids with Nonce. diff --git a/contracts/interfaces/IErrorsMech.sol b/contracts/interfaces/IErrorsMech.sol index 4536218..e244c82 100644 --- a/contracts/interfaces/IErrorsMech.sol +++ b/contracts/interfaces/IErrorsMech.sol @@ -8,6 +8,9 @@ interface IErrorsMech { /// @dev Provided zero value. error ZeroValue(); + /// @dev The contract is already initialized. + error AlreadyInitialized(); + /// @dev Mech marketplace is not authorized. /// @param mechMarketplace Mech marketplace address. error MarketplaceNotAuthorized(address mechMarketplace); diff --git a/contracts/interfaces/IMech.sol b/contracts/interfaces/IMech.sol index 7ec304a..df1ff06 100644 --- a/contracts/interfaces/IMech.sol +++ b/contracts/interfaces/IMech.sol @@ -3,9 +3,6 @@ pragma solidity ^0.8.28; /// @dev Mech interface interface IMech { - /// @dev Checks if the signer is the mech operator. - function isOperator(address signer) external view returns (bool); - /// @dev Registers a request by a marketplace. /// @param account Requester account address. /// @param data Self-descriptive opaque data-blob. @@ -17,8 +14,12 @@ interface IMech { /// @param requestId Request Id. function revokeRequest(uint256 requestId) external; + /// @dev Gets mech max delivery rate. + /// @return Mech maximum delivery rate. function maxDeliveryRate() external returns (uint256); + /// @dev Gets mech payment type hash. + /// @return Mech payment type hash. function paymentType() external returns (bytes32); /// @dev Gets finalized delivery rate for a request Id. @@ -26,5 +27,16 @@ interface IMech { /// @return Finalized delivery rate. function getFinalizedDeliveryRate(uint256 requestId) external returns (uint256); + /// @dev Gets mech token Id (service Id). + /// @return serviceId Service Id. function tokenId() external view returns (uint256); + + /// @dev Gets mech operator (service multisig). + /// @return Service multisig address. + function getOperator() external view returns (address); + + /// @dev Checks the mech operator (service multisig). + /// @param multisig Service multisig being checked against. + /// @return True, if mech service multisig matches the provided one. + function isOperator(address multisig) external view returns (bool); } \ No newline at end of file diff --git a/test/MechFixedPriceNative.js b/test/MechFixedPriceNative.js index 12f1d8b..3d3f165 100644 --- a/test/MechFixedPriceNative.js +++ b/test/MechFixedPriceNative.js @@ -18,14 +18,14 @@ describe("MechFixedPriceNative", function () { let signers; let deployer; const AddressZero = ethers.constants.AddressZero; - const price = 1000; + const maxDeliveryRate = 1000; const data = "0x00"; const fee = 10; const minResponseTimeout = 10; - const maxResponceTimeout = 20; + const maxResponseTimeout = 20; const mechServiceId = 1; const requesterServiceId = 0; - const priceData = ethers.utils.defaultAbiCoder.encode(["uint256"], [price]); + const mechCreationData = ethers.utils.defaultAbiCoder.encode(["uint256"], [maxDeliveryRate]); beforeEach(async function () { signers = await ethers.getSigners(); @@ -57,7 +57,7 @@ describe("MechFixedPriceNative", function () { // Deploy and initialize marketplace proxy proxyData = MechMarketplace.interface.encodeFunctionData("initialize", - [fee, minResponseTimeout, maxResponceTimeout]); + [fee, minResponseTimeout, maxResponseTimeout]); const MechMarketplaceProxy = await ethers.getContractFactory("MechMarketplaceProxy"); const mechMarketplaceProxy = await MechMarketplaceProxy.deploy(mechMarketplace.address, proxyData); await mechMarketplaceProxy.deployed(); @@ -83,7 +83,7 @@ describe("MechFixedPriceNative", function () { await serviceRegistry.setServiceOwner(requesterServiceId + 3, signers[1].address); // Create default priority mech - let tx = await mechMarketplace.create(mechServiceId, mechFactoryFixedPrice.address, priceData); + let tx = await mechMarketplace.create(mechServiceId, mechFactoryFixedPrice.address, mechCreationData); let res = await tx.wait(); // Get mech contract address from the event priorityMechAddress = "0x" + res.logs[0].topics[1].slice(26); @@ -91,7 +91,7 @@ describe("MechFixedPriceNative", function () { priorityMech = await ethers.getContractAt("MechFixedPriceNative", priorityMechAddress); // Create default delivery mech - tx = await mechMarketplace.create(mechServiceId + 1, mechFactoryFixedPrice.address, priceData); + tx = await mechMarketplace.create(mechServiceId + 1, mechFactoryFixedPrice.address, mechCreationData); res = await tx.wait(); // Get mech contract address from the event deliveryMechAddress = "0x" + res.logs[0].topics[1].slice(26); @@ -126,14 +126,14 @@ describe("MechFixedPriceNative", function () { MechFixedPriceNative.deploy(mechMarketplace.address, serviceRegistry.address, 0, 0) ).to.be.revertedWithCustomError(MechFixedPriceNative, "ZeroValue"); - // Zero price + // Zero maxDeliveryRate await expect( MechFixedPriceNative.deploy(mechMarketplace.address, serviceRegistry.address, mechServiceId, 0) ).to.be.revertedWithCustomError(MechFixedPriceNative, "ZeroValue"); // Service Id does not exist await expect( - MechFixedPriceNative.deploy(mechMarketplace.address, serviceRegistry.address, mechServiceId + 10, price) + MechFixedPriceNative.deploy(mechMarketplace.address, serviceRegistry.address, mechServiceId + 10, maxDeliveryRate) ).to.be.reverted; }); }); @@ -170,14 +170,14 @@ describe("MechFixedPriceNative", function () { mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout - 1, "0x") ).to.be.revertedWithCustomError(mechMarketplace, "OutOfBounds"); await expect( - mechMarketplace.request(data, mechServiceId, requesterServiceId, maxResponceTimeout + 1, "0x") + mechMarketplace.request(data, mechServiceId, requesterServiceId, maxResponseTimeout + 1, "0x") ).to.be.revertedWithCustomError(mechMarketplace, "OutOfBounds"); + // Change max response timeout close to type(uint32).max - //const closeToMaxUint96 = "4294967295"; - //await mechMarketplace.deploy(minResponseTimeout, closeToMaxUint96); - //await expect( - // mechMarketplace.request("0x", priorityMech.address, closeToMaxUint96) - //).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); + const closeToMaxUint32 = "4294967295"; + await expect( + mechMarketplace.request(data, mechServiceId, requesterServiceId, closeToMaxUint32, "0x") + ).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); // Try to request to a mech with an incorrect mech service Id await expect( @@ -190,7 +190,12 @@ describe("MechFixedPriceNative", function () { ).to.be.revertedWithCustomError(balanceTrackerFixedPriceNative, "InsufficientBalance"); // Create a request - await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); + + // Try to initialize the mech again + await expect( + priorityMech.setUp(data) + ).to.be.revertedWithCustomError(priorityMech, "AlreadyInitialized"); // Get the requests count let requestsCount = await mechMarketplace.mapRequestCounts(deployer.address); @@ -199,6 +204,10 @@ describe("MechFixedPriceNative", function () { expect(requestsCount).to.equal(1); requestsCount = await mechMarketplace.numTotalRequests(); expect(requestsCount).to.equal(1); + + // Get mech token value + const registry = await priorityMech.token(); + expect(registry).to.equal(serviceRegistry.address); }); }); @@ -216,9 +225,9 @@ describe("MechFixedPriceNative", function () { ).to.be.revertedWithCustomError(priorityMech, "RequestIdNotFound"); // Create a request - await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); - // Try to deliver not by the operator (agent owner) + // Try to deliver not by the service multisig (agent owner) await expect( priorityMech.connect(signers[1]).deliverToMarketplace(requestId, data) ).to.be.reverted; @@ -256,12 +265,20 @@ describe("MechFixedPriceNative", function () { const requestId = await mechMarketplace.getRequestId(deployer.address, data, 0); + // Try to deliver a non-existent request + await expect( + deliveryMech.deliverToMarketplace(requestId, data) + ).to.be.revertedWithCustomError(mechMarketplace, "RequestIdNotFound"); + await expect( + mechMarketplace.deliverMarketplace(requestId, data) + ).to.be.reverted; + // Get the non-existent request status let status = await mechMarketplace.getRequestStatus(requestId); expect(status).to.equal(0); // Create a request - await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); // Try to deliver by a delivery mech right away await expect( @@ -273,15 +290,27 @@ describe("MechFixedPriceNative", function () { expect(status).to.equal(1); // Increase the time such that the request expires for a priority mech - await helpers.time.increase(maxResponceTimeout); + await helpers.time.increase(maxResponseTimeout); // Get the request status (requested expired) status = await mechMarketplace.getRequestStatus(requestId); expect(status).to.equal(2); + // Try to deliver by a mech with bigger max Delivery rate + await deliveryMech.changeMaxDeliveryRate(maxDeliveryRate + 1); + await expect( + deliveryMech.deliverToMarketplace(requestId, data) + ).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); + + // Change max delivery rate back + await deliveryMech.changeMaxDeliveryRate(maxDeliveryRate); + // Deliver a request by the delivery mech await deliveryMech.deliverToMarketplace(requestId, data); + // Try to deliver the same request again (gets empty data) + await deliveryMech.deliverToMarketplace(requestId, data); + // Get the request status (delivered) status = await mechMarketplace.getRequestStatus(requestId); expect(status).to.equal(3); @@ -314,7 +343,7 @@ describe("MechFixedPriceNative", function () { expect(uRequestIds.length).to.equal(0); // Create a first request - await mechMarketplace.request(datas[0], mechServiceId, 0, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(datas[0], mechServiceId, 0, minResponseTimeout, "0x", {value: maxDeliveryRate}); // Check request Ids uRequestIds = await priorityMech.getUndeliveredRequestIds(0, 0); @@ -336,7 +365,7 @@ describe("MechFixedPriceNative", function () { // Stack all requests for (let i = 0; i < numRequests; i++) { - await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); } // Check request Ids @@ -360,7 +389,7 @@ describe("MechFixedPriceNative", function () { for (let i = 0; i < numRequests; i++) { requestIds[i] = await mechMarketplace.getRequestId(deployer.address, datas[i], requestCount); requestCount++; - await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); } // Deliver the first request @@ -409,7 +438,7 @@ describe("MechFixedPriceNative", function () { datas[i] = data + "00".repeat(i); requestIds[i] = await mechMarketplace.getRequestId(deployer.address, datas[i], requestCount); requestCount++; - await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); } // Deliver even requests @@ -449,7 +478,7 @@ describe("MechFixedPriceNative", function () { datas[i] = data + "00".repeat(i); requestIds[i] = await mechMarketplace.getRequestId(deployer.address, datas[i], requestCount); requestCount++; - await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: price}); + await mechMarketplace.request(datas[i], mechServiceId, requesterServiceId, minResponseTimeout, "0x", {value: maxDeliveryRate}); } // Check request Ids for just part of the batch @@ -492,12 +521,17 @@ describe("MechFixedPriceNative", function () { }); context("Changing parameters", async function () { - it("Set another minimum price", async function () { - await priorityMech.changeMaxDeliveryRate(price + 1); + it("Set another minimum maxDeliveryRate", async function () { + // Try to change zero delivery rate + await expect( + priorityMech.changeMaxDeliveryRate(0) + ).to.be.reverted; + + await priorityMech.changeMaxDeliveryRate(maxDeliveryRate + 1); - // Try to set price not by the operator (agent owner) + // Try to change delivery not by the service multisig (agent owner) await expect( - priorityMech.connect(signers[1]).changeMaxDeliveryRate(price + 2) + priorityMech.connect(signers[1]).changeMaxDeliveryRate(maxDeliveryRate + 2) ).to.be.reverted; }); }); diff --git a/test/MechMarketplace.js b/test/MechMarketplace.js new file mode 100644 index 0000000..14d5018 --- /dev/null +++ b/test/MechMarketplace.js @@ -0,0 +1,242 @@ +/*global describe, context, beforeEach, it*/ + +const { expect } = require("chai"); +const { ethers } = require("hardhat"); +const helpers = require("@nomicfoundation/hardhat-network-helpers"); + +describe("MechMarketplace", function () { + let MechMarketplace; + let priorityMechAddress; + let priorityMech; + let serviceRegistry; + let mechMarketplace; + let karma; + let mechFactoryFixedPrice; + let balanceTrackerFixedPriceNative; + let signers; + let deployer; + const AddressZero = ethers.constants.AddressZero; + const maxDeliveryRate = 1000; + const data = "0x00"; + const fee = 10; + const minResponseTimeout = 10; + const maxResponseTimeout = 20; + const mechServiceId = 1; + const requesterServiceId = 2; + let paymentTypeHash; + const mechCreationData = ethers.utils.defaultAbiCoder.encode(["uint256"], [maxDeliveryRate]); + + beforeEach(async function () { + signers = await ethers.getSigners(); + deployer = signers[0]; + + MechFixedPriceNative = await ethers.getContractFactory("MechFixedPriceNative"); + + // Karma implementation and proxy + const Karma = await ethers.getContractFactory("Karma"); + const karmaImplementation = await Karma.deploy(); + await karmaImplementation.deployed(); + + // Initialize karma + let proxyData = karmaImplementation.interface.encodeFunctionData("initialize", []); + const KarmaProxy = await ethers.getContractFactory("KarmaProxy"); + const karmaProxy = await KarmaProxy.deploy(karmaImplementation.address, proxyData); + await karmaProxy.deployed(); + + karma = await ethers.getContractAt("Karma", karmaProxy.address); + + const ServiceRegistry = await ethers.getContractFactory("MockServiceRegistry"); + serviceRegistry = await ServiceRegistry.deploy(); + await serviceRegistry.deployed(); + + // Wrapped native token and buy back burner are not relevant for now + MechMarketplace = await ethers.getContractFactory("MechMarketplace"); + mechMarketplace = await MechMarketplace.deploy(serviceRegistry.address, karma.address); + await mechMarketplace.deployed(); + + // Deploy and initialize marketplace proxy + proxyData = MechMarketplace.interface.encodeFunctionData("initialize", + [fee, minResponseTimeout, maxResponseTimeout]); + const MechMarketplaceProxy = await ethers.getContractFactory("MechMarketplaceProxy"); + const mechMarketplaceProxy = await MechMarketplaceProxy.deploy(mechMarketplace.address, proxyData); + await mechMarketplaceProxy.deployed(); + + mechMarketplace = await ethers.getContractAt("MechMarketplace", mechMarketplaceProxy.address); + + // Deploy mech factory + const MechFactoryFixedPrice = await ethers.getContractFactory("MechFactoryFixedPriceNative"); + mechFactoryFixedPrice = await MechFactoryFixedPrice.deploy(mechMarketplace.address); + await mechFactoryFixedPrice.deployed(); + + // Whitelist mech factory + await mechMarketplace.setMechFactoryStatuses([mechFactoryFixedPrice.address], [true]); + + // Whitelist marketplace in the karma proxy + await karma.setMechMarketplaceStatuses([mechMarketplace.address], [true]); + + // Pseudo-create two services + await serviceRegistry.setServiceOwner(mechServiceId, deployer.address); + + // Pseudo-create a requester service + await serviceRegistry.setServiceOwner(requesterServiceId, signers[1].address); + + // Create default priority mech + let tx = await mechMarketplace.create(mechServiceId, mechFactoryFixedPrice.address, mechCreationData); + let res = await tx.wait(); + // Get mech contract address from the event + priorityMechAddress = "0x" + res.logs[0].topics[1].slice(26); + // Get mech contract instance + priorityMech = await ethers.getContractAt("MechFixedPriceNative", priorityMechAddress); + + // Deploy + const BalanceTrackerFixedPriceNative = await ethers.getContractFactory("BalanceTrackerFixedPriceNative"); + balanceTrackerFixedPriceNative = await BalanceTrackerFixedPriceNative.deploy(mechMarketplace.address, + deployer.address, deployer.address); + await balanceTrackerFixedPriceNative.deployed(); + + // Whitelist balance tracker + paymentTypeHash = await priorityMech.paymentType(); + await mechMarketplace.setPaymentTypeBalanceTrackers([paymentTypeHash], [balanceTrackerFixedPriceNative.address]); + }); + + context("Initialization", async function () { + it("Checking for arguments passed to the constructor", async function () { + // Zero service registry + await expect( + MechMarketplace.deploy(AddressZero, AddressZero) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + + // Zero karma + await expect( + MechMarketplace.deploy(serviceRegistry.address, AddressZero) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + + // Try to initialize again + await expect( + mechMarketplace.initialize(fee, minResponseTimeout, maxResponseTimeout) + ).to.be.revertedWithCustomError(mechMarketplace, "AlreadyInitialized"); + }); + + it("Change owner", async function () { + // Trying to change owner from a non-owner account address + await expect( + mechMarketplace.connect(signers[1]).changeOwner(signers[1].address) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + // Trying to change owner for the zero address + await expect( + mechMarketplace.connect(deployer).changeOwner(AddressZero) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + + // Changing the owner + await mechMarketplace.connect(deployer).changeOwner(signers[1].address); + + // Trying to change owner from the previous owner address + await expect( + mechMarketplace.connect(deployer).changeOwner(deployer.address) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + // Change the owner back + await mechMarketplace.connect(signers[1]).changeOwner(deployer.address); + }); + + it("Change implementation", async function () { + // Trying to change implementation from a non-owner account address + await expect( + mechMarketplace.connect(signers[1]).changeImplementation(mechMarketplace.address) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + // Trying to change implementation for the zero address + await expect( + mechMarketplace.connect(deployer).changeImplementation(AddressZero) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + + // Changing the implementation + await mechMarketplace.connect(deployer).changeOwner(mechMarketplace.address); + }); + + it("Change marketplace params", async function () { + // Trying to change params not by the owner + await expect( + mechMarketplace.connect(signers[1]).changeMarketplaceParams(0, 0, 0) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + // Trying to change params to zeros + await expect( + mechMarketplace.changeMarketplaceParams(0, 0, 0) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroValue"); + await expect( + mechMarketplace.changeMarketplaceParams(10, 0, 0) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroValue"); + await expect( + mechMarketplace.changeMarketplaceParams(10, 10, 0) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroValue"); + + // Trying to change fee bigger than allowed + await expect( + mechMarketplace.changeMarketplaceParams(10001, 1, 2) + ).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); + + // Trying to set the min response bigger than max response timeout + await expect( + mechMarketplace.changeMarketplaceParams(10, 10, 1) + ).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); + + // Trying to set max response timeout bigger than the limit + const maxUint96 = "4294967297"; + await expect( + mechMarketplace.changeMarketplaceParams(10, 10, maxUint96) + ).to.be.revertedWithCustomError(mechMarketplace, "Overflow"); + }); + + it("Factories and balance trackers", async function () { + // Trying to call create not by the authorized factory + await expect( + mechMarketplace.create(mechServiceId, deployer.address, mechCreationData) + ).to.be.revertedWithCustomError(mechMarketplace, "UnauthorizedAccount"); + + // Trying to set mech factory statuses and balance trackersnot by the owner + await expect( + mechMarketplace.connect(signers[1]).setMechFactoryStatuses([AddressZero], [true]) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + await expect( + mechMarketplace.connect(signers[1]).setPaymentTypeBalanceTrackers([ethers.constants.HashZero], [AddressZero]) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + // Wrong array lengths + await expect( + mechMarketplace.setMechFactoryStatuses([AddressZero], []) + ).to.be.revertedWithCustomError(mechMarketplace, "WrongArrayLength"); + await expect( + mechMarketplace.setPaymentTypeBalanceTrackers([paymentTypeHash], []) + ).to.be.revertedWithCustomError(mechMarketplace, "WrongArrayLength"); + + // Zero addresses + await expect( + mechMarketplace.setMechFactoryStatuses([AddressZero], [true]) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + await expect( + mechMarketplace.setPaymentTypeBalanceTrackers([paymentTypeHash], [AddressZero]) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroAddress"); + // Zero value + await expect( + mechMarketplace.setPaymentTypeBalanceTrackers([ethers.constants.HashZero], [deployer.address]) + ).to.be.revertedWithCustomError(mechMarketplace, "ZeroValue"); + }); + }); + + context("Request checks", async function () { + it("Check mech and requester", async function () { + // Mech that was not registered by the whitelisted factory + await expect( + mechMarketplace.checkMech(deployer.address) + ).to.be.reverted; + + // Mech that has a different service Id + await expect( + mechMarketplace.checkRequester(deployer.address, requesterServiceId) + ).to.be.revertedWithCustomError(mechMarketplace, "OwnerOnly"); + + }); + }); +}); From 2297cb80f507d908c181873c8cf40ab518d6687d Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Tue, 24 Dec 2024 20:35:06 +0000 Subject: [PATCH 2/6] chore: linters --- test/MechMarketplace.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/MechMarketplace.js b/test/MechMarketplace.js index 14d5018..b64661b 100644 --- a/test/MechMarketplace.js +++ b/test/MechMarketplace.js @@ -2,7 +2,6 @@ const { expect } = require("chai"); const { ethers } = require("hardhat"); -const helpers = require("@nomicfoundation/hardhat-network-helpers"); describe("MechMarketplace", function () { let MechMarketplace; @@ -17,7 +16,6 @@ describe("MechMarketplace", function () { let deployer; const AddressZero = ethers.constants.AddressZero; const maxDeliveryRate = 1000; - const data = "0x00"; const fee = 10; const minResponseTimeout = 10; const maxResponseTimeout = 20; @@ -30,8 +28,6 @@ describe("MechMarketplace", function () { signers = await ethers.getSigners(); deployer = signers[0]; - MechFixedPriceNative = await ethers.getContractFactory("MechFixedPriceNative"); - // Karma implementation and proxy const Karma = await ethers.getContractFactory("Karma"); const karmaImplementation = await Karma.deploy(); From 5c3350b336c0faa69d2eb1e78deac2b6f652574d Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Wed, 25 Dec 2024 12:51:17 +0000 Subject: [PATCH 3/6] refactor: fee formula as suggested --- contracts/BalanceTrackerFixedPriceBase.sol | 14 +++++++------- contracts/MechMarketplace.sol | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/BalanceTrackerFixedPriceBase.sol b/contracts/BalanceTrackerFixedPriceBase.sol index f13fa41..469885f 100644 --- a/contracts/BalanceTrackerFixedPriceBase.sol +++ b/contracts/BalanceTrackerFixedPriceBase.sol @@ -52,8 +52,8 @@ abstract contract BalanceTrackerFixedPriceBase { event Withdraw(address indexed account, address indexed token, uint256 amount); event Drained(address indexed token, uint256 collectedFees); - // Fee base constant - uint256 public constant FEE_BASE = 10_000; + // Max marketplace fee + uint256 public constant MAX_FEE = 10_000; // Mech marketplace address address public immutable mechMarketplace; @@ -210,11 +210,11 @@ abstract contract BalanceTrackerFixedPriceBase { uint256 fee = IMechMarketplace(mechMarketplace).fee(); // If requested balance is too small, charge the minimal fee - if (balance < FEE_BASE) { - marketplaceFee = 1; - } else { - marketplaceFee = (balance * fee) / FEE_BASE; - } + // ceil(a, b) = (a + b - 1) / b + // This formula will always get at least a fee of 1 + marketplaceFee = (balance * fee + (MAX_FEE - 1)) / MAX_FEE; + + // Calculate mech payment mechPayment = balance - marketplaceFee; // Check for zero value, although this must never happen diff --git a/contracts/MechMarketplace.sol b/contracts/MechMarketplace.sol index 56f40ed..7a26c45 100644 --- a/contracts/MechMarketplace.sol +++ b/contracts/MechMarketplace.sol @@ -61,8 +61,8 @@ contract MechMarketplace is IErrorsMarketplace { // Domain separator type hash bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - // Fee base constant - uint256 public constant FEE_BASE = 10_000; + // Max marketplace fee + uint256 public constant MAX_FEE = 10_000; // Original domain separator value bytes32 public immutable domainSeparator; @@ -160,8 +160,8 @@ contract MechMarketplace is IErrorsMarketplace { } // Check for fee value - if (newFee > FEE_BASE) { - revert Overflow(newFee, FEE_BASE); + if (newFee > MAX_FEE) { + revert Overflow(newFee, MAX_FEE); } // Check for sanity values From 0a3e4b70d669841e8598e78848a22c16f5be7e4e Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Thu, 26 Dec 2024 13:52:42 +0000 Subject: [PATCH 4/6] fix: delivery recording bug --- contracts/BalanceTrackerFixedPriceBase.sol | 7 ++-- contracts/MechMarketplace.sol | 19 ++++----- .../BalanceTrackerNvmSubscription.sol | 6 +-- contracts/interfaces/IBalanceTracker.sol | 2 +- test/MechFixedPriceNative.js | 40 +++++++++++++++++++ 5 files changed, 55 insertions(+), 19 deletions(-) diff --git a/contracts/BalanceTrackerFixedPriceBase.sol b/contracts/BalanceTrackerFixedPriceBase.sol index 469885f..600e261 100644 --- a/contracts/BalanceTrackerFixedPriceBase.sol +++ b/contracts/BalanceTrackerFixedPriceBase.sol @@ -88,8 +88,8 @@ abstract contract BalanceTrackerFixedPriceBase { // Check and record delivery rate function checkAndRecordDeliveryRate( - address mech, address requester, + uint256 maxDeliveryRate, bytes memory ) external payable { // Reentrancy guard @@ -109,9 +109,6 @@ abstract contract BalanceTrackerFixedPriceBase { // Get account balance uint256 balance = mapRequesterBalances[requester] + initAmount; - // Get mech max delivery rate - uint256 maxDeliveryRate = IMech(mech).maxDeliveryRate(); - // Check the request delivery rate for a fixed price if (balance < maxDeliveryRate) { // Get balance difference @@ -150,12 +147,14 @@ abstract contract BalanceTrackerFixedPriceBase { revert ZeroValue(); } + // Check for delivery rate difference uint256 rateDiff; if (maxDeliveryRate > actualDeliveryRate) { // Return back requester overpayment debit rateDiff = maxDeliveryRate - actualDeliveryRate; mapRequesterBalances[requester] += rateDiff; } else { + // Limit the rate by the max chosen one as that is what the requester agreed on actualDeliveryRate = maxDeliveryRate; } diff --git a/contracts/MechMarketplace.sol b/contracts/MechMarketplace.sol index 7a26c45..6865ca0 100644 --- a/contracts/MechMarketplace.sol +++ b/contracts/MechMarketplace.sol @@ -381,13 +381,6 @@ contract MechMarketplace is IErrorsMarketplace { // Get the request Id requestId = getRequestId(msg.sender, data, mapNonces[msg.sender]); - // Get balance tracker address - bytes32 mechPaymentType = IMech(priorityMech).paymentType(); - address balanceTracker = mapPaymentTypeBalanceTrackers[mechPaymentType]; - - // Check and record mech delivery rate - IBalanceTracker(balanceTracker).checkAndRecordDeliveryRate{value: msg.value}(priorityMech, msg.sender, paymentData); - // Update requester nonce mapNonces[msg.sender]++; @@ -400,8 +393,16 @@ contract MechMarketplace is IErrorsMarketplace { mechDelivery.responseTimeout = responseTimeout + block.timestamp; // Record request account mechDelivery.requester = msg.sender; - // Record deliveryRate for request - mechDelivery.deliveryRate = msg.value; + // Record deliveryRate for request as priority mech max delivery rate + mechDelivery.deliveryRate = IMech(priorityMech).maxDeliveryRate(); + + // Get balance tracker address + bytes32 mechPaymentType = IMech(priorityMech).paymentType(); + address balanceTracker = mapPaymentTypeBalanceTrackers[mechPaymentType]; + + // Check and record mech delivery rate + IBalanceTracker(balanceTracker).checkAndRecordDeliveryRate{value: msg.value}(msg.sender, + mechDelivery.deliveryRate, paymentData); // Increase mech requester karma IKarma(karma).changeRequesterMechKarma(msg.sender, priorityMech, 1); diff --git a/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol b/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol index d51ecce..4c39c1d 100644 --- a/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol +++ b/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol @@ -88,9 +88,8 @@ contract BalanceTrackerNvmSubscription { // Check and record delivery rate function checkAndRecordDeliveryRate( - address mech, address requester, - uint256, + uint256 maxDeliveryRate, bytes memory ) external payable { // Check for marketplace access @@ -98,9 +97,6 @@ contract BalanceTrackerNvmSubscription { revert MarketplaceOnly(msg.sender, mechMarketplace); } - // Get mech max delivery rate - uint256 maxDeliveryRate = IMech(mech).maxDeliveryRate(); - // Check that there is no incoming deposit if (msg.value > 0) { revert NoDepositAllowed(msg.value); diff --git a/contracts/interfaces/IBalanceTracker.sol b/contracts/interfaces/IBalanceTracker.sol index 732ef38..624175d 100644 --- a/contracts/interfaces/IBalanceTracker.sol +++ b/contracts/interfaces/IBalanceTracker.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.28; interface IBalanceTracker { // Check and record delivery rate /// @param paymentData Additional payment-related request data, if applicable. - function checkAndRecordDeliveryRate(address mech, address requester, bytes memory paymentData) external payable; + function checkAndRecordDeliveryRate(address requester, uint256 maxDeliveryRate, bytes memory paymentData) external payable; /// @dev Finalizes mech delivery rate based on requested and actual ones. /// @param mech Delivery mech address. diff --git a/test/MechFixedPriceNative.js b/test/MechFixedPriceNative.js index 3d3f165..45f5fe7 100644 --- a/test/MechFixedPriceNative.js +++ b/test/MechFixedPriceNative.js @@ -259,6 +259,46 @@ describe("MechFixedPriceNative", function () { expect(mechKarma).to.equal(1); }); + it("Delivering a request by a priority mech with pre-paid logic", async function () { + // Get request Id + const requestId = await mechMarketplace.getRequestId(deployer.address, data, 0); + + // Pre-pay the contract insufficient amount for posting a request + await deployer.sendTransaction({to: balanceTrackerFixedPriceNative.address, value: maxDeliveryRate - 1}); + + // Try to create request with insufficient pre-paid amount + await expect( + mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x") + ).to.be.revertedWithCustomError(balanceTrackerFixedPriceNative, "InsufficientBalance"); + + // Pre-pay the contract more for posting a request + await deployer.sendTransaction({to: balanceTrackerFixedPriceNative.address, value: maxDeliveryRate}); + + // Post a request + await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x"); + + // Get the request status (requested priority) + status = await mechMarketplace.getRequestStatus(requestId); + expect(status).to.equal(1); + + // Deliver a request + await priorityMech.deliverToMarketplace(requestId, data); + + // Get the request status (delivered) + status = await mechMarketplace.getRequestStatus(requestId); + expect(status).to.equal(3); + + // Try to deliver the same request again + await priorityMech.deliverToMarketplace(requestId, data); + + // Check mech karma + let mechKarma = await karma.mapMechKarma(priorityMech.address); + expect(mechKarma).to.equal(1); + // Check requester mech karma + mechKarma = await karma.mapRequesterMechKarma(deployer.address, priorityMech.address); + expect(mechKarma).to.equal(1); + }); + it("Delivering a request by a different mech", async function () { // Take a snapshot of the current state of the blockchain const snapshot = await helpers.takeSnapshot(); From cb283c8ac93e527ae1e5f8dd51f9553102df8b51 Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Thu, 26 Dec 2024 14:39:04 +0000 Subject: [PATCH 5/6] fix: more bugs --- contracts/BalanceTrackerFixedPriceBase.sol | 43 ++++++++++++++----- contracts/BalanceTrackerFixedPriceNative.sol | 8 ++-- contracts/BalanceTrackerFixedPriceToken.sol | 8 ++-- contracts/MechMarketplace.sol | 8 ++-- .../BalanceTrackerNvmSubscription.sol | 3 +- test/MechFixedPriceNative.js | 27 ++++++++++++ 6 files changed, 74 insertions(+), 23 deletions(-) diff --git a/contracts/BalanceTrackerFixedPriceBase.sol b/contracts/BalanceTrackerFixedPriceBase.sol index 600e261..b64f4e8 100644 --- a/contracts/BalanceTrackerFixedPriceBase.sol +++ b/contracts/BalanceTrackerFixedPriceBase.sol @@ -52,8 +52,8 @@ abstract contract BalanceTrackerFixedPriceBase { event Withdraw(address indexed account, address indexed token, uint256 amount); event Drained(address indexed token, uint256 collectedFees); - // Max marketplace fee - uint256 public constant MAX_FEE = 10_000; + // Max marketplace fee factor (100%) + uint256 public constant MAX_FEE_FACTOR = 10_000; // Mech marketplace address address public immutable mechMarketplace; @@ -189,10 +189,33 @@ abstract contract BalanceTrackerFixedPriceBase { _locked = 1; } - function _withdraw(uint256 balance) internal virtual; + function _withdraw(address mech, uint256 balance) internal virtual; - /// @dev Processes mech payment by withdrawing funds. - function processPayment() external returns (uint256 mechPayment, uint256 marketplaceFee) { + /// @dev Processes mech payment by mech service multisig. + /// @param mech Mech address. + /// @return Mech payment. + /// @return Marketplace fee. + function processPaymentByMultisig(address mech) external returns (uint256, uint256) { + // Check for mech service multisig address + if (!IMech(mech).isOperator(msg.sender)) { + revert UnauthorizedAccount(msg.sender); + } + + return _processPayment(mech); + } + + /// @dev Processes mech payment. + /// @return Mech payment. + /// @return Marketplace fee. + function processPayment() external returns (uint256, uint256) { + return _processPayment(msg.sender); + } + + /// @dev Process mech payment. + /// @param mech Mech address. + /// @return mechPayment Mech payment. + /// @return marketplaceFee Marketplace fee. + function _processPayment(address mech) internal returns (uint256 mechPayment, uint256 marketplaceFee) { // Reentrancy guard if (_locked > 1) { revert ReentrancyGuard(); @@ -200,7 +223,7 @@ abstract contract BalanceTrackerFixedPriceBase { _locked = 2; // Get mech balance - uint256 balance = mapMechBalances[msg.sender]; + uint256 balance = mapMechBalances[mech]; if (balance == 0) { revert ZeroValue(); } @@ -211,7 +234,7 @@ abstract contract BalanceTrackerFixedPriceBase { // If requested balance is too small, charge the minimal fee // ceil(a, b) = (a + b - 1) / b // This formula will always get at least a fee of 1 - marketplaceFee = (balance * fee + (MAX_FEE - 1)) / MAX_FEE; + marketplaceFee = (balance * fee + (MAX_FEE_FACTOR - 1)) / MAX_FEE_FACTOR; // Calculate mech payment mechPayment = balance - marketplaceFee; @@ -225,10 +248,10 @@ abstract contract BalanceTrackerFixedPriceBase { collectedFees += marketplaceFee; // Clear balances - mapMechBalances[msg.sender] = 0; + mapMechBalances[mech] = 0; // Process withdraw - _withdraw(balance); + _withdraw(mech, mechPayment); _locked = 1; } @@ -251,7 +274,7 @@ abstract contract BalanceTrackerFixedPriceBase { mapRequesterBalances[msg.sender] = 0; // Process withdraw - _withdraw(balance); + _withdraw(msg.sender, balance); _locked = 1; } diff --git a/contracts/BalanceTrackerFixedPriceNative.sol b/contracts/BalanceTrackerFixedPriceNative.sol index 067f1ee..47be330 100644 --- a/contracts/BalanceTrackerFixedPriceNative.sol +++ b/contracts/BalanceTrackerFixedPriceNative.sol @@ -61,16 +61,16 @@ contract BalanceTrackerFixedPriceNative is BalanceTrackerFixedPriceBase { emit Drained(wrappedNativeToken, amount); } - function _withdraw(uint256 balance) internal virtual override { + function _withdraw(address account, uint256 amount) internal virtual override { // solhint-disable-next-line avoid-low-level-calls - (bool success, ) = msg.sender.call{value: balance}(""); + (bool success, ) = account.call{value: amount}(""); // Check transfer if (!success) { - revert TransferFailed(address(0), address(this), msg.sender, balance); + revert TransferFailed(address(0), address(this), account, amount); } - emit Withdraw(msg.sender, address(0), balance); + emit Withdraw(msg.sender, address(0), amount); } // Deposits funds for requester. diff --git a/contracts/BalanceTrackerFixedPriceToken.sol b/contracts/BalanceTrackerFixedPriceToken.sol index 12239f5..f9536c8 100644 --- a/contracts/BalanceTrackerFixedPriceToken.sol +++ b/contracts/BalanceTrackerFixedPriceToken.sol @@ -76,15 +76,15 @@ contract BalanceTrackerFixedPriceToken is BalanceTrackerFixedPriceBase { emit Drained(olas, amount); } - function _withdraw(uint256 balance) internal virtual override { - bool success = IToken(olas).transfer(msg.sender, balance); + function _withdraw(address account, uint256 amount) internal virtual override { + bool success = IToken(olas).transfer(account, amount); // Check transfer if (!success) { - revert TransferFailed(olas, address(this), msg.sender, balance); + revert TransferFailed(olas, address(this), account, amount); } - emit Withdraw(msg.sender, olas, balance); + emit Withdraw(msg.sender, olas, amount); } // Deposits token funds for requester. diff --git a/contracts/MechMarketplace.sol b/contracts/MechMarketplace.sol index 6865ca0..1f8580d 100644 --- a/contracts/MechMarketplace.sol +++ b/contracts/MechMarketplace.sol @@ -61,8 +61,8 @@ contract MechMarketplace is IErrorsMarketplace { // Domain separator type hash bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - // Max marketplace fee - uint256 public constant MAX_FEE = 10_000; + // Max marketplace fee factor (100%) + uint256 public constant MAX_FEE_FACTOR = 10_000; // Original domain separator value bytes32 public immutable domainSeparator; @@ -160,8 +160,8 @@ contract MechMarketplace is IErrorsMarketplace { } // Check for fee value - if (newFee > MAX_FEE) { - revert Overflow(newFee, MAX_FEE); + if (newFee > MAX_FEE_FACTOR) { + revert Overflow(newFee, MAX_FEE_FACTOR); } // Check for sanity values diff --git a/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol b/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol index 4c39c1d..a85946a 100644 --- a/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol +++ b/contracts/integrations/nevermined/BalanceTrackerNvmSubscription.sol @@ -172,7 +172,8 @@ contract BalanceTrackerNvmSubscription { _locked = 1; } - /// @dev Processes payment. + /// @dev Processes requester credits. + /// @param requester Requester address. function processPayment(address requester) external { // Reentrancy guard if (_locked > 1) { diff --git a/test/MechFixedPriceNative.js b/test/MechFixedPriceNative.js index 45f5fe7..9130c40 100644 --- a/test/MechFixedPriceNative.js +++ b/test/MechFixedPriceNative.js @@ -297,6 +297,33 @@ describe("MechFixedPriceNative", function () { // Check requester mech karma mechKarma = await karma.mapRequesterMechKarma(deployer.address, priorityMech.address); expect(mechKarma).to.equal(1); + + // Check priority mech balance now + let mechBalance = await balanceTrackerFixedPriceNative.mapMechBalances(priorityMech.address); + expect(mechBalance).to.equal(maxDeliveryRate); + + const balanceBefore = await ethers.provider.getBalance(priorityMech.address); + // Process payment for mech + await balanceTrackerFixedPriceNative.processPaymentByMultisig(priorityMech.address); + const balanceAfter = await ethers.provider.getBalance(priorityMech.address); + + // Check charged fee + const collectedFees = await balanceTrackerFixedPriceNative.collectedFees(); + // Since the delivery rate is smaller than MAX_FEE_FACTOR, the minimal fee was charged + expect(collectedFees).to.equal(1); + + // Check mech payout: payment - fee + const balanceDiff = balanceAfter.sub(balanceBefore); + expect(balanceDiff).to.equal(maxDeliveryRate - 1); + + // Check requester leftover balance + let requesterBalance = await balanceTrackerFixedPriceNative.mapRequesterBalances(deployer.address); + expect(requesterBalance).to.equal(maxDeliveryRate - 1); + + // Withdraw requester balances + await balanceTrackerFixedPriceNative.withdraw(); + requesterBalance = await balanceTrackerFixedPriceNative.mapRequesterBalances(deployer.address); + expect(requesterBalance).to.equal(0); }); it("Delivering a request by a different mech", async function () { From eeb209333b3eb375a4d161acc784e744f3183cad Mon Sep 17 00:00:00 2001 From: Aleksandr Kuperman Date: Thu, 26 Dec 2024 14:44:15 +0000 Subject: [PATCH 6/6] chore: linters --- test/MechFixedPriceNative.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/MechFixedPriceNative.js b/test/MechFixedPriceNative.js index 9130c40..38879c5 100644 --- a/test/MechFixedPriceNative.js +++ b/test/MechFixedPriceNative.js @@ -278,7 +278,7 @@ describe("MechFixedPriceNative", function () { await mechMarketplace.request(data, mechServiceId, requesterServiceId, minResponseTimeout, "0x"); // Get the request status (requested priority) - status = await mechMarketplace.getRequestStatus(requestId); + let status = await mechMarketplace.getRequestStatus(requestId); expect(status).to.equal(1); // Deliver a request