diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a190c67..0e0ee170 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## vNEXT +- Make sponsored & non-sponsored amounts always divisible by deal volume to refund tasks fairly. (#28) - Add slither suggestions. (#26) - Drain expired vouchers and withdraw funds. (#25) - Add slither github action. (#24) diff --git a/contracts/VoucherHub.sol b/contracts/VoucherHub.sol index e6881c7c..b77460dd 100644 --- a/contracts/VoucherHub.sol +++ b/contracts/VoucherHub.sol @@ -240,6 +240,11 @@ contract VoucherHub is sponsoredAmount += workerpoolPrice; } sponsoredAmount = Math.min(balanceOf(msg.sender), sponsoredAmount * volume); + // Decrease sponsored amount to make sponsored & non-sponsored amounts + // are divisible by volume in order to refund plain amounts to voucher and + // requester (i.e. make sure there are no hidden remainders) in the event + // that tasks are claimed later. + sponsoredAmount -= sponsoredAmount % volume; if (sponsoredAmount > 0) { _burn(msg.sender, sponsoredAmount); emit VoucherDebited(msg.sender, sponsoredAmount); diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 06502c92..6c8c3b46 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -441,10 +441,7 @@ contract Voucher is Initializable, IVoucher { $._refundedTasks[taskId] = true; if (taskPrice != 0) { uint256 dealSponsoredAmount = $._sponsoredAmounts[dealId]; - // A positive remainder is possible when the voucher balance is less than - // the sponsorable amount. Min(balance, dealSponsoredAmount) is computed - // at match orders. - // TODO !! do something with the remainder. + // The division leaves no remainder. See VoucherHub#debitVoucher(). uint256 taskSponsoredAmount = dealSponsoredAmount / dealVolume; if (taskSponsoredAmount != 0) { // If the voucher did fully/partially sponsor the deal then mint voucher diff --git a/contracts/mocks/IexecPocoMock.sol b/contracts/mocks/IexecPocoMock.sol index de91e6ad..e34e1f5a 100644 --- a/contracts/mocks/IexecPocoMock.sol +++ b/contracts/mocks/IexecPocoMock.sol @@ -25,7 +25,7 @@ contract IexecPocoMock is ERC20 { IexecLibCore_v5.Deal public deal; IexecLibCore_v5.DealBoost public dealBoost; - IexecLibCore_v5.Task public task; + mapping(bytes32 => IexecLibCore_v5.Task) public tasks; constructor() ERC20("Staked RLC", "SRLC") { _mint(msg.sender, 1000000); @@ -49,8 +49,12 @@ contract IexecPocoMock is ERC20 { deal.dataset.price = datasetOrder.datasetprice; deal.workerpool.price = workerpoolOrder.workerpoolprice; deal.sponsor = msg.sender; - task.dealid = mockDealId; - task.status = IexecLibCore_v5.TaskStatusEnum.UNSET; + for (uint256 i = 0; i < deal.botSize; i++) { + bytes32 taskId = keccak256(abi.encode(mockDealId, i)); + IexecLibCore_v5.Task storage task = tasks[taskId]; + task.dealid = mockDealId; + task.status = IexecLibCore_v5.TaskStatusEnum.UNSET; + } uint256 volume = computeDealVolume(appOrder, datasetOrder, workerpoolOrder, requestOrder); deal.botSize = volume; uint256 dealPrice = (appOrder.appprice + @@ -74,7 +78,12 @@ contract IexecPocoMock is ERC20 { dealBoost.datasetPrice = uint96(datasetOrder.datasetprice); dealBoost.workerpoolPrice = uint96(workerpoolOrder.workerpoolprice); dealBoost.sponsor = msg.sender; - task.status = IexecLibCore_v5.TaskStatusEnum.UNSET; + for (uint256 i = 0; i < dealBoost.botSize; i++) { + bytes32 taskId = keccak256(abi.encode(mockDealId, i)); + IexecLibCore_v5.Task storage task = tasks[taskId]; + task.dealid = mockDealId; + task.status = IexecLibCore_v5.TaskStatusEnum.UNSET; + } uint256 volume = computeDealVolume(appOrder, datasetOrder, workerpoolOrder, requestOrder); dealBoost.botSize = uint16(volume); uint256 dealPrice = (appOrder.appprice + @@ -109,7 +118,13 @@ contract IexecPocoMock is ERC20 { revert("IexecPocoMock: Failed to claim"); } // This simulates non existent task/deal. - if (taskId != mockTaskId) { + bool knownTask; + for (uint256 i = 0; i < deal.botSize; i++) { + if (taskId == keccak256(abi.encode(mockDealId, i))) { + knownTask = true; + } + } + if (!knownTask) { revert(); // no reason, same as PoCo. } task.status = IexecLibCore_v5.TaskStatusEnum.FAILED; @@ -122,10 +137,11 @@ contract IexecPocoMock is ERC20 { revert("IexecPocoMock: Failed to claim boost"); } // This simulates non existent task/deal. - if (dealId != mockDealId || taskIndex != mockTaskIndex) { + if (dealId != mockDealId || taskIndex >= dealBoost.botSize) { revert("PocoBoost: Unknown task"); // same as PoCo. } - task.status = IexecLibCore_v5.TaskStatusEnum.FAILED; + bytes32 taskId = keccak256(abi.encode(mockDealId, taskIndex)); + tasks[taskId].status = IexecLibCore_v5.TaskStatusEnum.FAILED; // mint task price. _mint( dealBoost.sponsor, @@ -149,8 +165,8 @@ contract IexecPocoMock is ERC20 { return dealBoost; } - function viewTask(bytes32) external view returns (IexecLibCore_v5.Task memory) { - return task; + function viewTask(bytes32 taskId) external view returns (IexecLibCore_v5.Task memory) { + return tasks[taskId]; } function computeDealVolume( diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index 9c8873a5..5ed83c61 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -20,14 +20,14 @@ import { Voucher__factory, } from '../../typechain-types'; import { random } from '../utils/address-utils'; -import { TaskStatusEnum, createMockOrder } from '../utils/poco-utils'; +import { PocoMode, TaskStatusEnum, createMockOrder, getTaskId } from '../utils/poco-utils'; // TODO use srlc instead of rlc. const voucherType = 0; const duration = 3600; const description = 'Early Access'; -const voucherValue = 100; +const voucherValue = 100n; const app = random(); const dataset = random(); const workerpool = random(); @@ -72,6 +72,22 @@ describe('Voucher', function () { beforeEach('Deploy', async () => { await loadFixture(deployFixture); + // Create mock orders. + const mockOrder = createMockOrder(); + appOrder = { ...mockOrder, app: app, appprice: appPrice, volume: volume }; + datasetOrder = { + ...mockOrder, + dataset: dataset, + datasetprice: datasetPrice, + volume: volume, + }; + workerpoolOrder = { + ...mockOrder, + workerpool: workerpool, + workerpoolprice: workerpoolPrice, + volume: volume, + }; + requestOrder = { ...mockOrder, requester: requester.address, volume: volume }; }); async function deployFixture() { @@ -114,22 +130,6 @@ describe('Voucher', function () { .then(() => voucherHub.getVoucher(voucherOwner1)); voucherAsOwner = Voucher__factory.connect(voucherAddress, voucherOwner1); voucherAsAnyone = voucherAsOwner.connect(anyone); - // Create mock orders. - const mockOrder = createMockOrder(); - appOrder = { ...mockOrder, app: app, appprice: appPrice, volume: volume }; - datasetOrder = { - ...mockOrder, - dataset: dataset, - datasetprice: datasetPrice, - volume: volume, - }; - workerpoolOrder = { - ...mockOrder, - workerpool: workerpool, - workerpoolprice: workerpoolPrice, - volume: volume, - }; - requestOrder = { ...mockOrder, requester: requester.address, volume: volume }; } describe('Upgrade', function () { @@ -854,20 +854,80 @@ describe('Voucher', function () { } }); - describe('[TODO] Should claim task when deal is partially sponsored and sponsored amount is not divisible by volume', async () => { - it.skip('Classic', async () => await runTest(voucherMatchOrders, claim)); - it.skip('Boost', async () => await runTest(voucherMatchOrdersBoost, claimBoost)); + describe('Should claim task when deal is partially sponsored and sponsored amount is reduced to make it divisible by volume', async () => { + it('Classic', async () => await runTest(PocoMode.CLASSIC)); + it('Boost', async () => await runTest(PocoMode.BOOST)); - async function runTest(matchOrdersBoostOrClassic: any, claimBoostOrClassic: any) { - // Use another voucher with a small amount of credits. - const smallVoucherValue = 1n; - voucherAsOwner = await voucherHubAsVoucherCreationManager - .createVoucher(voucherOwner2, voucherType, smallVoucherValue) - .then((tx) => tx.wait()) - .then(() => voucherHub.getVoucher(voucherOwner2)) - .then((voucherAddress) => - Voucher__factory.connect(voucherAddress, voucherOwner2), + async function runTest(pocoMode: PocoMode) { + await addEligibleAssets([app, dataset, workerpool]); + const volume = 17n; + for (const order of [appOrder, datasetOrder, workerpoolOrder, requestOrder]) { + order.volume = volume; + } + const dealPrice = taskPrice * volume; + let dealSponsorableAmount = voucherValue; // dealPrice > voucherValue + // Make sure dealSponsorableAmount is not divisible by volume + expect(dealSponsorableAmount % volume).greaterThan(0); + // Remove remainder to get expected dealSponsoredAmount + let dealSponsoredAmount = dealSponsorableAmount - (dealSponsorableAmount % volume); + const dealNonSponsoredAmount = dealPrice - dealSponsoredAmount; + expect(dealSponsoredAmount % volume).equal(0); // Both amounts should be + expect(dealNonSponsoredAmount % volume).equal(0); // divisible by volume + // Deposit non-sponsored amount for requester and approve voucher. + await iexecPocoInstance + .transfer(requester, dealNonSponsoredAmount) + .then((tx) => tx.wait()); + await iexecPocoInstance + .connect(requester) + .approve(voucherAddress, dealNonSponsoredAmount) + .then((tx) => tx.wait()); + const isBoost = pocoMode == PocoMode.BOOST; + await (isBoost ? voucherMatchOrdersBoost() : voucherMatchOrders()); + const { + voucherCreditBalance: voucherCreditBalancePreClaim, + voucherRlcBalance: voucherSrlcBalancePreClaim, + requesterRlcBalance: requesterRlcBalancePreClaim, + } = await getVoucherAndRequesterBalances(); + expect(voucherCreditBalancePreClaim).equal(voucherValue - dealSponsoredAmount); + expect(requesterRlcBalancePreClaim).equal(0); + let taskSponsoredAmount = dealSponsoredAmount / volume; + let taskNonSponsoredAmount = dealNonSponsoredAmount / volume; + expect(taskSponsoredAmount).to.be.greaterThan(0); // Make sure both parties + expect(taskNonSponsoredAmount).to.be.greaterThan(0); // will expect a refund + for (let taskIndex = 0; taskIndex < volume; taskIndex++) { + await expect( + isBoost + ? voucherAsOwner.claimBoost(dealId, taskIndex) + : voucherAsOwner.claim(getTaskId(dealId, taskIndex)), + ) + .to.emit(voucherHub, 'VoucherRefunded') + .withArgs(voucherAddress, taskSponsoredAmount) + .to.emit(iexecPocoInstance, 'Transfer') + .withArgs(voucherAddress, requester.address, taskNonSponsoredAmount) + .to.emit(voucherAsOwner, 'TaskClaimedWithVoucher'); + const { + voucherCreditBalance, + voucherRlcBalance: voucherSrlcBalance, + requesterRlcBalance, + } = await getVoucherAndRequesterBalances(); + const currentVolume = BigInt(taskIndex + 1); + expect(voucherCreditBalance) + .equal(voucherCreditBalancePreClaim + taskSponsoredAmount * currentVolume) + .equal(voucherSrlcBalance); + expect(requesterRlcBalance).equal( + requesterRlcBalancePreClaim + taskNonSponsoredAmount * currentVolume, ); + } + const { + voucherCreditBalance: voucherCreditBalanceAfter, + voucherRlcBalance: voucherSrlcBalanceAfter, + requesterRlcBalance: requesterSrlcBalanceAfter, + } = await getVoucherAndRequesterBalances(); + expect(voucherCreditBalanceAfter) + .equal(voucherCreditBalancePreClaim + dealSponsoredAmount) + .equal(voucherSrlcBalanceAfter) + .equal(voucherSrlcBalancePreClaim + dealSponsoredAmount); + expect(requesterSrlcBalanceAfter).equal(dealNonSponsoredAmount); } }); @@ -1097,12 +1157,7 @@ describe('Voucher', function () { expect(await voucherAsOwner.getSponsoredAmount(dealId)).to.be.equal(dealPrice); // Claim task const badTaskIndex = 99; - const badTaskId = ethers.keccak256( - ethers.AbiCoder.defaultAbiCoder().encode( - ['bytes32', 'uint256'], - [dealId, badTaskIndex], - ), - ); + const badTaskId = getTaskId(dealId, badTaskIndex); await expect(voucherAsOwner.claim(badTaskId)).to.be.revertedWithoutReason(); }); diff --git a/test/utils/poco-utils.ts b/test/utils/poco-utils.ts index bcb30145..f8aac5ba 100644 --- a/test/utils/poco-utils.ts +++ b/test/utils/poco-utils.ts @@ -3,6 +3,11 @@ import { ethers } from 'hardhat'; +export enum PocoMode { + CLASSIC, + BOOST, +} + export enum TaskStatusEnum { UNSET, ACTIVE, @@ -38,3 +43,7 @@ export function createMockOrder() { workerpoolrestrict: ethers.ZeroAddress, }; } + +export function getTaskId(dealId: string, taskIndex: number): string { + return ethers.solidityPackedKeccak256(['bytes32', 'uint256'], [dealId, taskIndex]); +}