From ad0a009a173c75f3398d4e86bbe09f2ec6fb12a7 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 30 Mar 2021 12:02:24 -0300 Subject: [PATCH 1/9] disputes: refactor functions to transfer tokens --- contracts/disputes/DisputeManager.sol | 125 +++++++++++++------------- 1 file changed, 65 insertions(+), 60 deletions(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 1bf2e53e3..038698d5e 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -40,16 +40,16 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa // -- EIP-712 -- - bytes32 private constant DOMAIN_TYPE_HASH = keccak256( - "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)" - ); + bytes32 private constant DOMAIN_TYPE_HASH = + keccak256( + "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)" + ); bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Protocol"); bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0"); - bytes32 - private constant DOMAIN_SALT = 0xa070ffb1cd7409649bf77822cce74495468e06dbfaef09556838bf188679b9c2; - bytes32 private constant RECEIPT_TYPE_HASH = keccak256( - "Receipt(bytes32 requestCID,bytes32 responseCID,bytes32 subgraphDeploymentID)" - ); + bytes32 private constant DOMAIN_SALT = + 0xa070ffb1cd7409649bf77822cce74495468e06dbfaef09556838bf188679b9c2; + bytes32 private constant RECEIPT_TYPE_HASH = + keccak256("Receipt(bytes32 requestCID,bytes32 responseCID,bytes32 subgraphDeploymentID)"); // -- Constants -- @@ -264,7 +264,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @notice Return if dispute with ID `_disputeID` exists * @param _disputeID True if dispute already exists */ - function isDisputeCreated(bytes32 _disputeID) public override view returns (bool) { + function isDisputeCreated(bytes32 _disputeID) public view override returns (bool) { return disputes[_disputeID].fisherman != address(0); } @@ -276,7 +276,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @param _receipt Receipt returned by indexer and submitted by fisherman * @return Message hash used to sign the receipt */ - function encodeHashReceipt(Receipt memory _receipt) public override view returns (bytes32) { + function encodeHashReceipt(Receipt memory _receipt) public view override returns (bytes32) { return keccak256( abi.encodePacked( @@ -304,7 +304,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa function areConflictingAttestations( Attestation memory _attestation1, Attestation memory _attestation2 - ) public override pure returns (bool) { + ) public pure override returns (bool) { return (_attestation1.requestCID == _attestation2.requestCID && _attestation1.subgraphDeploymentID == _attestation2.subgraphDeploymentID && _attestation1.responseCID != _attestation2.responseCID); @@ -317,8 +317,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa */ function getAttestationIndexer(Attestation memory _attestation) public - override view + override returns (address) { // Get attestation signer, allocationID @@ -339,7 +339,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @param _indexer Indexer to be slashed * @return Reward calculated as percentage of the indexer slashed funds */ - function getTokensToReward(address _indexer) public override view returns (uint256) { + function getTokensToReward(address _indexer) public view override returns (uint256) { uint256 tokens = getTokensToSlash(_indexer); if (tokens == 0) { return 0; @@ -352,7 +352,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @param _indexer Address of the indexer * @return Amount of tokens to slash */ - function getTokensToSlash(address _indexer) public override view returns (uint256) { + function getTokensToSlash(address _indexer) public view override returns (uint256) { uint256 tokens = staking().getIndexerStakedTokens(_indexer); // slashable tokens if (tokens == 0) { return 0; @@ -415,18 +415,10 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa // Create the disputes // The deposit is zero for conflicting attestations - bytes32 dID1 = _createQueryDisputeWithAttestation( - fisherman, - 0, - attestation1, - _attestationData1 - ); - bytes32 dID2 = _createQueryDisputeWithAttestation( - fisherman, - 0, - attestation2, - _attestationData2 - ); + bytes32 dID1 = + _createQueryDisputeWithAttestation(fisherman, 0, attestation1, _attestationData1); + bytes32 dID2 = + _createQueryDisputeWithAttestation(fisherman, 0, attestation2, _attestationData2); // Store the linked disputes to be resolved disputes[dID1].relatedDisputeID = dID2; @@ -462,15 +454,16 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa require(staking().hasStake(indexer), "Dispute indexer has no stake"); // Create a disputeID - bytes32 disputeID = keccak256( - abi.encodePacked( - _attestation.requestCID, - _attestation.responseCID, - _attestation.subgraphDeploymentID, - indexer, - _fisherman - ) - ); + bytes32 disputeID = + keccak256( + abi.encodePacked( + _attestation.requestCID, + _attestation.responseCID, + _attestation.subgraphDeploymentID, + indexer, + _fisherman + ) + ); // Only one dispute for a (indexer, subgraphDeploymentID) at a time require(!isDisputeCreated(disputeID), "Dispute already created"); @@ -560,12 +553,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa uint256 tokensToReward = _slashIndexer(dispute.indexer, dispute.fisherman); // Give the fisherman their deposit back - if (dispute.deposit > 0) { - require( - graphToken().transfer(dispute.fisherman, dispute.deposit), - "Error sending dispute deposit" - ); - } + _pushTokens(dispute.fisherman, dispute.deposit); // Resolve the conflicting dispute if any _resolveDisputeInConflict(dispute); @@ -609,12 +597,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa Dispute memory dispute = _resolveDispute(_disputeID); // Return deposit to the fisherman - if (dispute.deposit > 0) { - require( - graphToken().transfer(dispute.fisherman, dispute.deposit), - "Error sending dispute deposit" - ); - } + _pushTokens(dispute.fisherman, dispute.deposit); // Resolve the conflicting dispute if any _resolveDisputeInConflict(dispute); @@ -670,10 +653,33 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa require(_deposit >= minimumDeposit, "Dispute deposit is under minimum required"); // Transfer tokens to deposit from fisherman to this contract - require( - graphToken().transferFrom(msg.sender, address(this), _deposit), - "Cannot transfer tokens to deposit" - ); + _pullTokens(msg.sender, _deposit); + } + + /** + * @dev Pull tokens from an address to this contract. + * @param _from Address sending the tokens + * @param _amount Amount of tokens to transfer + */ + function _pullTokens(address _from, uint256 _amount) private { + if (_amount > 0) { + // Transfer tokens to deposit from fisherman to this contract + require( + graphToken().transferFrom(_from, address(this), _amount), + "Cannot transfer tokens" + ); + } + } + + /** + * @dev Push tokens from this contract to a receiving address. + * @param _to Address receiving the tokens + * @param _amount Amount of tokens to transfer + */ + function _pushTokens(address _to, uint256 _amount) private { + if (_amount > 0) { + require(graphToken().transfer(_to, _amount), "Cannot transfer tokens"); + } } /** @@ -706,11 +712,12 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa returns (address) { // Obtain the hash of the fully-encoded message, per EIP-712 encoding - Receipt memory receipt = Receipt( - _attestation.requestCID, - _attestation.responseCID, - _attestation.subgraphDeploymentID - ); + Receipt memory receipt = + Receipt( + _attestation.requestCID, + _attestation.responseCID, + _attestation.subgraphDeploymentID + ); bytes32 messageHash = encodeHashReceipt(receipt); // Obtain the signer of the fully-encoded EIP-712 message hash @@ -743,10 +750,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa require(_data.length == ATTESTATION_SIZE_BYTES, "Attestation must be 161 bytes long"); // Decode receipt - (bytes32 requestCID, bytes32 responseCID, bytes32 subgraphDeploymentID) = abi.decode( - _data, - (bytes32, bytes32, bytes32) - ); + (bytes32 requestCID, bytes32 responseCID, bytes32 subgraphDeploymentID) = + abi.decode(_data, (bytes32, bytes32, bytes32)); // Decode signature // Signature is expected to be in the order defined in the Attestation struct From a11ada95db17bd90c483911f1d4fc8a3fa43f779 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Wed, 31 Mar 2021 21:46:31 -0300 Subject: [PATCH 2/9] disputes: separate slashing percentages for query and indexing disputes --- contracts/disputes/DisputeManager.sol | 121 ++++++++++--------- contracts/disputes/DisputeManagerStorage.sol | 3 +- contracts/disputes/IDisputeManager.sol | 9 +- test/disputes/configuration.test.ts | 21 ++-- test/disputes/query.test.ts | 41 +++---- test/lib/deployment.ts | 6 +- 6 files changed, 108 insertions(+), 93 deletions(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 038698d5e..10e5fd9c6 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -14,7 +14,7 @@ import "./IDisputeManager.sol"; /* * @title DisputeManager - * @dev Provides a way to align the incentives of participants by having slashing as deterrent + * @notice Provides a way to align the incentives of participants by having slashing as deterrent * for incorrect behaviour. * * There are two types of disputes that can be created: Query disputes and Indexing disputes. @@ -149,14 +149,16 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @param _arbitrator Arbitrator role * @param _minimumDeposit Minimum deposit required to create a Dispute * @param _fishermanRewardPercentage Percent of slashed funds for fisherman (ppm) - * @param _slashingPercentage Percentage of indexer stake slashed (ppm) + * @param _qrySlashingPercentage Percentage of indexer stake slashed for query disputes (ppm) + * @param _idxSlashingPercentage Percentage of indexer stake slashed for indexing disputes (ppm) */ function initialize( address _controller, address _arbitrator, uint256 _minimumDeposit, uint32 _fishermanRewardPercentage, - uint32 _slashingPercentage + uint32 _qrySlashingPercentage, + uint32 _idxSlashingPercentage ) external onlyImpl { Managed._initialize(_controller); @@ -164,7 +166,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa _setArbitrator(_arbitrator); _setMinimumDeposit(_minimumDeposit); _setFishermanRewardPercentage(_fishermanRewardPercentage); - _setSlashingPercentage(_slashingPercentage); + _setSlashingPercentage(_qrySlashingPercentage, _idxSlashingPercentage); // EIP-712 domain separator DOMAIN_SEPARATOR = keccak256( @@ -242,20 +244,30 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa /** * @dev Set the percentage used for slashing indexers. - * @param _percentage Percentage used for slashing + * @param _qryPercentage Percentage slashing for query disputes + * @param _idxPercentage Percentage slashing for indexing disputes */ - function setSlashingPercentage(uint32 _percentage) external override onlyGovernor { - _setSlashingPercentage(_percentage); + function setSlashingPercentage(uint32 _qryPercentage, uint32 _idxPercentage) + external + override + onlyGovernor + { + _setSlashingPercentage(_qryPercentage, _idxPercentage); } /** * @dev Internal: Set the percentage used for slashing indexers. - * @param _percentage Percentage used for slashing + * @param _qryPercentage Percentage slashing for query disputes + * @param _idxPercentage Percentage slashing for indexing disputes */ - function _setSlashingPercentage(uint32 _percentage) private { + function _setSlashingPercentage(uint32 _qryPercentage, uint32 _idxPercentage) private { // Must be within 0% to 100% (inclusive) - require(_percentage <= MAX_PPM, "Slashing percentage must be below or equal to MAX_PPM"); - slashingPercentage = _percentage; + require( + _qryPercentage <= MAX_PPM && _idxPercentage <= MAX_PPM, + "Slashing percentage must be below or equal to MAX_PPM" + ); + qrySlashingPercentage = _qryPercentage; + idxSlashingPercentage = _idxPercentage; emit ParameterUpdated("slashingPercentage"); } @@ -321,7 +333,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa override returns (address) { - // Get attestation signer, allocationID + // Get attestation signer. Indexers signs with the allocationID address allocationID = _recoverAttestationSigner(_attestation); IStaking.Allocation memory alloc = staking().getAllocation(allocationID); @@ -333,33 +345,6 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa return alloc.indexer; } - /** - * @dev Get the fisherman reward for a given indexer stake. - * @notice Return the fisherman reward based on the `_indexer` stake - * @param _indexer Indexer to be slashed - * @return Reward calculated as percentage of the indexer slashed funds - */ - function getTokensToReward(address _indexer) public view override returns (uint256) { - uint256 tokens = getTokensToSlash(_indexer); - if (tokens == 0) { - return 0; - } - return uint256(fishermanRewardPercentage).mul(tokens).div(MAX_PPM); - } - - /** - * @dev Get the amount of tokens to slash for an indexer based on the current stake. - * @param _indexer Address of the indexer - * @return Amount of tokens to slash - */ - function getTokensToSlash(address _indexer) public view override returns (uint256) { - uint256 tokens = staking().getIndexerStakedTokens(_indexer); // slashable tokens - if (tokens == 0) { - return 0; - } - return uint256(slashingPercentage).mul(tokens).div(MAX_PPM); - } - /** * @dev Create a query dispute for the arbitrator to resolve. * This function is called by a fisherman that will need to `_deposit` at @@ -451,7 +436,7 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa address indexer = getAttestationIndexer(_attestation); // The indexer is disputable - require(staking().hasStake(indexer), "Dispute indexer has no stake"); + require(staking().getIndexerStakedTokens(indexer) > 0, "Dispute indexer has no stake"); // Create a disputeID bytes32 disputeID = @@ -473,7 +458,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa indexer, _fisherman, _deposit, - 0 // no related dispute + 0, // no related dispute, + DisputeType.QueryDispute ); emit QueryDisputeCreated( @@ -527,14 +513,21 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa require(!isDisputeCreated(disputeID), "Dispute already created"); // Allocation must exist - IStaking.Allocation memory alloc = staking().getAllocation(_allocationID); + IStaking staking = staking(); + IStaking.Allocation memory alloc = staking.getAllocation(_allocationID); require(alloc.indexer != address(0), "Dispute allocation must exist"); // The indexer must be disputable - require(staking().hasStake(alloc.indexer), "Dispute indexer has no stake"); + require(staking.getIndexerStakedTokens(alloc.indexer) > 0, "Dispute indexer has no stake"); // Store dispute - disputes[disputeID] = Dispute(alloc.indexer, _fisherman, _deposit, 0); + disputes[disputeID] = Dispute( + alloc.indexer, + _fisherman, + _deposit, + 0, + DisputeType.IndexingDispute + ); emit IndexingDisputeCreated(disputeID, alloc.indexer, _fisherman, _deposit, _allocationID); @@ -550,7 +543,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa Dispute memory dispute = _resolveDispute(_disputeID); // Slash - uint256 tokensToReward = _slashIndexer(dispute.indexer, dispute.fisherman); + (, uint256 tokensToReward) = + _slashIndexer(dispute.indexer, dispute.fisherman, dispute.disputeType); // Give the fisherman their deposit back _pushTokens(dispute.fisherman, dispute.deposit); @@ -687,18 +681,35 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * Give the challenger a reward equal to the fishermanRewardPercentage of slashed amount * @param _indexer Address of the indexer * @param _challenger Address of the challenger - * @return Dispute reward tokens - */ - function _slashIndexer(address _indexer, address _challenger) private returns (uint256) { - // Have staking contract slash the indexer and reward the fisherman - // Give the fisherman a reward equal to the fishermanRewardPercentage of slashed amount - uint256 tokensToSlash = getTokensToSlash(_indexer); - uint256 tokensToReward = getTokensToReward(_indexer); + * @param _disputeType Type of dispute + * @return slashAmount Dispute slash amount and reward tokens + */ + function _slashIndexer( + address _indexer, + address _challenger, + DisputeType _disputeType + ) private returns (uint256 slashAmount, uint256 rewardsAmount) { + IStaking staking = staking(); + + // Get slashable amount for indexer + uint256 slashableAmount = staking.getIndexerStakedTokens(_indexer); // slashable tokens + + // Get slash amount + uint256 slashingPercentage = 0; + if (_disputeType == DisputeType.QueryDispute) { + slashingPercentage = qrySlashingPercentage; + } else if (_disputeType == DisputeType.IndexingDispute) { + slashingPercentage = idxSlashingPercentage; + } + slashAmount = uint256(slashingPercentage).mul(slashableAmount).div(MAX_PPM); + require(slashAmount > 0, "Dispute has zero tokens to slash"); - require(tokensToSlash > 0, "Dispute has zero tokens to slash"); - staking().slash(_indexer, tokensToSlash, tokensToReward, _challenger); + // Get rewards amount + rewardsAmount = uint256(fishermanRewardPercentage).mul(slashAmount).div(MAX_PPM); - return tokensToReward; + // Have staking contract slash the indexer and reward the fisherman + // Give the fisherman a reward equal to the fishermanRewardPercentage of slashed amount + staking.slash(_indexer, slashAmount, rewardsAmount, _challenger); } /** diff --git a/contracts/disputes/DisputeManagerStorage.sol b/contracts/disputes/DisputeManagerStorage.sol index 30b0e55ba..66c182d26 100644 --- a/contracts/disputes/DisputeManagerStorage.sol +++ b/contracts/disputes/DisputeManagerStorage.sol @@ -23,7 +23,8 @@ contract DisputeManagerV1Storage is Managed { // Percentage of indexer stake to slash on disputes // Parts per million. (Allows for 4 decimal points, 999,999 = 99.9999%) - uint32 public slashingPercentage; + uint32 public qrySlashingPercentage; + uint32 public idxSlashingPercentage; // Disputes created : disputeID => Dispute // disputeID - check creation functions to see how disputeID is built diff --git a/contracts/disputes/IDisputeManager.sol b/contracts/disputes/IDisputeManager.sol index c57a04217..e8d940e97 100644 --- a/contracts/disputes/IDisputeManager.sol +++ b/contracts/disputes/IDisputeManager.sol @@ -6,12 +6,15 @@ pragma experimental ABIEncoderV2; interface IDisputeManager { // -- Dispute -- + enum DisputeType { IndexingDispute, QueryDispute } + // Disputes contain info necessary for the Arbitrator to verify and resolve struct Dispute { address indexer; address fisherman; uint256 deposit; bytes32 relatedDisputeID; + DisputeType disputeType; } // -- Attestation -- @@ -41,7 +44,7 @@ interface IDisputeManager { function setFishermanRewardPercentage(uint32 _percentage) external; - function setSlashingPercentage(uint32 _percentage) external; + function setSlashingPercentage(uint32 _qryPercentage, uint32 _idxPercentage) external; // -- Getters -- @@ -56,10 +59,6 @@ interface IDisputeManager { function getAttestationIndexer(Attestation memory _attestation) external view returns (address); - function getTokensToReward(address _indexer) external view returns (uint256); - - function getTokensToSlash(address _indexer) external view returns (uint256); - // -- Dispute -- function createQueryDispute(bytes calldata _attestationData, uint256 _deposit) diff --git a/test/disputes/configuration.test.ts b/test/disputes/configuration.test.ts index ad24f192a..ee49529f2 100644 --- a/test/disputes/configuration.test.ts +++ b/test/disputes/configuration.test.ts @@ -103,23 +103,30 @@ describe('DisputeManager:Config', () => { describe('slashingPercentage', function () { it('should set `slashingPercentage`', async function () { - const newValue = defaults.dispute.slashingPercentage + const qryNewValue = defaults.dispute.qrySlashingPercentage + const idxNewValue = defaults.dispute.idxSlashingPercentage // Set right in the constructor - expect(await disputeManager.slashingPercentage()).eq(newValue) + expect(await disputeManager.qrySlashingPercentage()).eq(qryNewValue) + expect(await disputeManager.idxSlashingPercentage()).eq(idxNewValue) // Set new value - await disputeManager.connect(governor.signer).setSlashingPercentage(0) - await disputeManager.connect(governor.signer).setSlashingPercentage(newValue) + await disputeManager.connect(governor.signer).setSlashingPercentage(0, 0) + await disputeManager + .connect(governor.signer) + .setSlashingPercentage(qryNewValue, idxNewValue) }) it('reject set `slashingPercentage` if out of bounds', async function () { - const tx = disputeManager.connect(governor.signer).setSlashingPercentage(MAX_PPM + 1) - await expect(tx).revertedWith('Slashing percentage must be below or equal to MAX_PPM') + const tx1 = disputeManager.connect(governor.signer).setSlashingPercentage(0, MAX_PPM + 1) + await expect(tx1).revertedWith('Slashing percentage must be below or equal to MAX_PPM') + + const tx2 = disputeManager.connect(governor.signer).setSlashingPercentage(MAX_PPM + 1, 0) + await expect(tx2).revertedWith('Slashing percentage must be below or equal to MAX_PPM') }) it('reject set `slashingPercentage` if not allowed', async function () { - const tx = disputeManager.connect(me.signer).setSlashingPercentage(50) + const tx = disputeManager.connect(me.signer).setSlashingPercentage(50, 50) await expect(tx).revertedWith('Caller must be Controller governor') }) }) diff --git a/test/disputes/query.test.ts b/test/disputes/query.test.ts index 145fa1e3c..792869559 100644 --- a/test/disputes/query.test.ts +++ b/test/disputes/query.test.ts @@ -111,6 +111,16 @@ describe('DisputeManager:Query', async () => { return attestation } + async function calculateSlashConditions(indexerAddress: string) { + const qrySlashingPercentage = await disputeManager.qrySlashingPercentage() + const fishermanRewardPercentage = await disputeManager.fishermanRewardPercentage() + const stakeAmount = await staking.getIndexerStakedTokens(indexerAddress) + const slashAmount = stakeAmount.mul(qrySlashingPercentage).div(toBN(MAX_PPM)) + const rewardsAmount = slashAmount.mul(fishermanRewardPercentage).div(toBN(MAX_PPM)) + + return { slashAmount, rewardsAmount } + } + async function setupIndexers() { // Dispute manager is allowed to slash await staking.connect(governor.signer).setSlasher(disputeManager.address, true) @@ -257,22 +267,6 @@ describe('DisputeManager:Query', async () => { await setupIndexers() }) - describe('reward calculation', function () { - it('should calculate the reward for a stake', async function () { - const fishermanRewardPercentage = await disputeManager.fishermanRewardPercentage() - const slashingPercentage = await disputeManager.slashingPercentage() - - const stakedAmount = indexerTokens - const trueReward = stakedAmount - .mul(slashingPercentage) - .div(toBN(MAX_PPM)) - .mul(fishermanRewardPercentage) - .div(toBN(MAX_PPM)) - const funcReward = await disputeManager.getTokensToReward(indexer.address) - expect(funcReward).eq(trueReward.toString()) - }) - }) - describe('create dispute', function () { it('reject fisherman deposit below minimum required', async function () { // Minimum deposit a fisherman is required to do should be >= reward @@ -395,7 +389,9 @@ describe('DisputeManager:Query', async () => { }) it('reject to accept a dispute if zero tokens to slash', async function () { - await disputeManager.connect(governor.signer).setSlashingPercentage(toBN('0')) + await disputeManager + .connect(governor.signer) + .setSlashingPercentage(toBN('0'), toBN('0')) const tx = disputeManager.connect(arbitrator.signer).acceptDispute(dispute.id) await expect(tx).revertedWith('Dispute has zero tokens to slash') }) @@ -407,8 +403,7 @@ describe('DisputeManager:Query', async () => { const beforeTotalSupply = await grt.totalSupply() // Calculations - const tokensToSlash = await disputeManager.getTokensToSlash(indexer.address) - const reward = await disputeManager.getTokensToReward(indexer.address) + const { slashAmount, rewardsAmount } = await calculateSlashConditions(indexer.address) // Perform transaction (accept) const tx = disputeManager.connect(arbitrator.signer).acceptDispute(dispute.id) @@ -418,7 +413,7 @@ describe('DisputeManager:Query', async () => { dispute.id, dispute.indexerAddress, fisherman.address, - fishermanDeposit.add(reward), + fishermanDeposit.add(rewardsAmount), ) // After state @@ -428,12 +423,12 @@ describe('DisputeManager:Query', async () => { // Fisherman reward properly assigned + deposit returned expect(afterFishermanBalance).eq( - beforeFishermanBalance.add(fishermanDeposit).add(reward), + beforeFishermanBalance.add(fishermanDeposit).add(rewardsAmount), ) // Indexer slashed - expect(afterIndexerStake).eq(beforeIndexerStake.sub(tokensToSlash)) + expect(afterIndexerStake).eq(beforeIndexerStake.sub(slashAmount)) // Slashed funds burned - const tokensToBurn = tokensToSlash.sub(reward) + const tokensToBurn = slashAmount.sub(rewardsAmount) expect(afterTotalSupply).eq(beforeTotalSupply.sub(tokensToBurn)) }) }) diff --git a/test/lib/deployment.ts b/test/lib/deployment.ts index f8976a3e3..5e9348d1a 100644 --- a/test/lib/deployment.ts +++ b/test/lib/deployment.ts @@ -33,7 +33,8 @@ export const defaults = { dispute: { minimumDeposit: toGRT('100'), fishermanRewardPercentage: toBN('1000'), // in basis points - slashingPercentage: toBN('1000'), // in basis points + qrySlashingPercentage: toBN('1000'), // in basis points + idxSlashingPercentage: toBN('100000'), // in basis points }, epochs: { lengthInBlocks: toBN((15 * 60) / 15), // 15 minutes in blocks @@ -149,7 +150,8 @@ export async function deployDisputeManager( arbitrator, defaults.dispute.minimumDeposit.toString(), defaults.dispute.fishermanRewardPercentage.toString(), - defaults.dispute.slashingPercentage.toString(), + defaults.dispute.qrySlashingPercentage.toString(), + defaults.dispute.idxSlashingPercentage.toString(), ], deployer, false, From 9d8c177f5af20d2d9427d9208a6cabef76eec575 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 6 Apr 2021 03:08:19 -0300 Subject: [PATCH 3/9] disputes: improve function to get slashing percentage --- contracts/disputes/DisputeManager.sol | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 10e5fd9c6..868a73cfe 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -682,7 +682,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa * @param _indexer Address of the indexer * @param _challenger Address of the challenger * @param _disputeType Type of dispute - * @return slashAmount Dispute slash amount and reward tokens + * @return slashAmount Dispute slash amount + * @return rewardsAmount Dispute rewards amount */ function _slashIndexer( address _indexer, @@ -695,13 +696,9 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa uint256 slashableAmount = staking.getIndexerStakedTokens(_indexer); // slashable tokens // Get slash amount - uint256 slashingPercentage = 0; - if (_disputeType == DisputeType.QueryDispute) { - slashingPercentage = qrySlashingPercentage; - } else if (_disputeType == DisputeType.IndexingDispute) { - slashingPercentage = idxSlashingPercentage; - } - slashAmount = uint256(slashingPercentage).mul(slashableAmount).div(MAX_PPM); + slashAmount = _getSlashingPercentageForDisputeType(_disputeType).mul(slashableAmount).div( + MAX_PPM + ); require(slashAmount > 0, "Dispute has zero tokens to slash"); // Get rewards amount @@ -712,6 +709,21 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa staking.slash(_indexer, slashAmount, rewardsAmount, _challenger); } + /** + * @dev Recover the signer address of the `_attestation`. + * @param _disputeType Dispute type + * @return Slashing percentage to use for the dispute type + */ + function _getSlashingPercentageForDisputeType(DisputeType _disputeType) + private + view + returns (uint256) + { + if (_disputeType == DisputeType.QueryDispute) return uint256(qrySlashingPercentage); + if (_disputeType == DisputeType.IndexingDispute) return uint256(idxSlashingPercentage); + return 0; + } + /** * @dev Recover the signer address of the `_attestation`. * @param _attestation The attestation struct From b61a9b27eb18eab5ec6bb612244d58c33f2321f2 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Sat, 10 Apr 2021 18:39:44 -0300 Subject: [PATCH 4/9] disputes: add more tests --- contracts/disputes/DisputeManagerStorage.sol | 2 + test/disputes/common.ts | 42 ++++++++++++++ test/disputes/poi.test.ts | 56 ++++++++++++++++++- test/disputes/query.test.ts | 58 ++++---------------- 4 files changed, 110 insertions(+), 48 deletions(-) create mode 100644 test/disputes/common.ts diff --git a/contracts/disputes/DisputeManagerStorage.sol b/contracts/disputes/DisputeManagerStorage.sol index 66c182d26..f9f328c19 100644 --- a/contracts/disputes/DisputeManagerStorage.sol +++ b/contracts/disputes/DisputeManagerStorage.sol @@ -17,6 +17,7 @@ contract DisputeManagerV1Storage is Managed { // Minimum deposit required to create a Dispute uint256 public minimumDeposit; + // -- Slot 0xf // Percentage of indexer slashed funds to assign as a reward to fisherman in successful dispute // Parts per million. (Allows for 4 decimal points, 999,999 = 99.9999%) uint32 public fishermanRewardPercentage; @@ -26,6 +27,7 @@ contract DisputeManagerV1Storage is Managed { uint32 public qrySlashingPercentage; uint32 public idxSlashingPercentage; + // -- Slot 0x10 // Disputes created : disputeID => Dispute // disputeID - check creation functions to see how disputeID is built mapping(bytes32 => IDisputeManager.Dispute) public disputes; diff --git a/test/disputes/common.ts b/test/disputes/common.ts new file mode 100644 index 000000000..37d77fd5a --- /dev/null +++ b/test/disputes/common.ts @@ -0,0 +1,42 @@ +import { utils } from 'ethers' +import { Attestation, Receipt } from '@graphprotocol/common-ts' + +export const MAX_PPM = 1000000 + +const { defaultAbiCoder: abi, arrayify, concat, hexlify, solidityKeccak256, joinSignature } = utils + +export interface Dispute { + id: string + attestation: Attestation + encodedAttestation: string + indexerAddress: string + receipt: Receipt +} + +export function createQueryDisputeID( + attestation: Attestation, + indexerAddress: string, + submitterAddress: string, +): string { + return solidityKeccak256( + ['bytes32', 'bytes32', 'bytes32', 'address', 'address'], + [ + attestation.requestCID, + attestation.responseCID, + attestation.subgraphDeploymentID, + indexerAddress, + submitterAddress, + ], + ) +} + +export function encodeAttestation(attestation: Attestation): string { + const data = arrayify( + abi.encode( + ['bytes32', 'bytes32', 'bytes32'], + [attestation.requestCID, attestation.responseCID, attestation.subgraphDeploymentID], + ), + ) + const sig = joinSignature(attestation) + return hexlify(concat([data, sig])) +} diff --git a/test/disputes/poi.test.ts b/test/disputes/poi.test.ts index 3b79d2bae..6a3adce6d 100644 --- a/test/disputes/poi.test.ts +++ b/test/disputes/poi.test.ts @@ -18,9 +18,11 @@ import { Account, } from '../lib/testHelpers' +import { MAX_PPM } from './common' + const { keccak256 } = utils -describe('DisputeManager:POI', async () => { +describe.only('DisputeManager:POI', async () => { let other: Account let governor: Account let arbitrator: Account @@ -48,6 +50,16 @@ describe('DisputeManager:POI', async () => { const metadata = randomHexBytes(32) const poi = randomHexBytes(32) // proof of indexing + async function calculateSlashConditions(indexerAddress: string) { + const idxSlashingPercentage = await disputeManager.idxSlashingPercentage() + const fishermanRewardPercentage = await disputeManager.fishermanRewardPercentage() + const stakeAmount = await staking.getIndexerStakedTokens(indexerAddress) + const slashAmount = stakeAmount.mul(idxSlashingPercentage).div(toBN(MAX_PPM)) + const rewardsAmount = slashAmount.mul(fishermanRewardPercentage).div(toBN(MAX_PPM)) + + return { slashAmount, rewardsAmount } + } + async function setupIndexers() { // Dispute manager is allowed to slash await staking.connect(governor.signer).setSlasher(disputeManager.address, true) @@ -180,6 +192,8 @@ describe('DisputeManager:POI', async () => { }) context('> when dispute is created', function () { + // NOTE: other dispute resolution paths are tested in query.test.ts + beforeEach(async function () { // Create dispute await disputeManager @@ -193,6 +207,46 @@ describe('DisputeManager:POI', async () => { .createIndexingDispute(allocationID, fishermanDeposit) await expect(tx).revertedWith('Dispute already created') }) + + describe('accept a dispute', function () { + it('should resolve dispute, slash indexer and reward the fisherman', async function () { + const disputeID = keccak256(allocationID) + + // Before state + const beforeIndexerStake = await staking.getIndexerStakedTokens(indexer.address) + const beforeFishermanBalance = await grt.balanceOf(fisherman.address) + const beforeTotalSupply = await grt.totalSupply() + + // Calculations + const { slashAmount, rewardsAmount } = await calculateSlashConditions(indexer.address) + + // Perform transaction (accept) + const tx = disputeManager.connect(arbitrator.signer).acceptDispute(disputeID) + await expect(tx) + .emit(disputeManager, 'DisputeAccepted') + .withArgs( + disputeID, + indexer.address, + fisherman.address, + fishermanDeposit.add(rewardsAmount), + ) + + // After state + const afterFishermanBalance = await grt.balanceOf(fisherman.address) + const afterIndexerStake = await staking.getIndexerStakedTokens(indexer.address) + const afterTotalSupply = await grt.totalSupply() + + // Fisherman reward properly assigned + deposit returned + expect(afterFishermanBalance).eq( + beforeFishermanBalance.add(fishermanDeposit).add(rewardsAmount), + ) + // Indexer slashed + expect(afterIndexerStake).eq(beforeIndexerStake.sub(slashAmount)) + // Slashed funds burned + const tokensToBurn = slashAmount.sub(rewardsAmount) + expect(afterTotalSupply).eq(beforeTotalSupply.sub(tokensToBurn)) + }) + }) }) }) }) diff --git a/test/disputes/query.test.ts b/test/disputes/query.test.ts index 792869559..27b0c5545 100644 --- a/test/disputes/query.test.ts +++ b/test/disputes/query.test.ts @@ -1,6 +1,6 @@ import { expect } from 'chai' -import { constants, utils } from 'ethers' -import { createAttestation, Attestation, Receipt } from '@graphprotocol/common-ts' +import { constants } from 'ethers' +import { createAttestation, Receipt } from '@graphprotocol/common-ts' import { DisputeManager } from '../../build/typechain/contracts/DisputeManager' import { EpochManager } from '../../build/typechain/contracts/EpochManager' @@ -20,49 +20,13 @@ import { Account, } from '../lib/testHelpers' +import { Dispute, createQueryDisputeID, encodeAttestation, MAX_PPM } from './common' + const { AddressZero, HashZero } = constants -const { defaultAbiCoder: abi, arrayify, concat, hexlify, solidityKeccak256, joinSignature } = utils -const MAX_PPM = 1000000 const NON_EXISTING_DISPUTE_ID = randomHexBytes() -interface Dispute { - id: string - attestation: Attestation - encodedAttestation: string - indexerAddress: string - receipt: Receipt -} - -function createDisputeID( - attestation: Attestation, - indexerAddress: string, - submitterAddress: string, -) { - return solidityKeccak256( - ['bytes32', 'bytes32', 'bytes32', 'address', 'address'], - [ - attestation.requestCID, - attestation.responseCID, - attestation.subgraphDeploymentID, - indexerAddress, - submitterAddress, - ], - ) -} - -function encodeAttestation(attestation: Attestation): string { - const data = arrayify( - abi.encode( - ['bytes32', 'bytes32', 'bytes32'], - [attestation.requestCID, attestation.responseCID, attestation.subgraphDeploymentID], - ), - ) - const sig = joinSignature(attestation) - return hexlify(concat([data, sig])) -} - -describe('DisputeManager:Query', async () => { +describe.only('DisputeManager:Query', async () => { let me: Account let other: Account let governor: Account @@ -193,7 +157,7 @@ describe('DisputeManager:Query', async () => { // Create dispute data dispute = { - id: createDisputeID(attestation, indexer.address, fisherman.address), + id: createQueryDisputeID(attestation, indexer.address, fisherman.address), attestation, encodedAttestation: encodeAttestation(attestation), indexerAddress: indexer.address, @@ -336,7 +300,7 @@ describe('DisputeManager:Query', async () => { // Create dispute (same receipt but different indexer) const attestation = await buildAttestation(receipt, indexer2ChannelKey.privKey) const newDispute: Dispute = { - id: createDisputeID(attestation, indexer2.address, fisherman.address), + id: createQueryDisputeID(attestation, indexer2.address, fisherman.address), attestation, encodedAttestation: encodeAttestation(attestation), indexerAddress: indexer2.address, @@ -520,8 +484,8 @@ describe('DisputeManager:Query', async () => { it('should create dispute', async function () { const [attestation1, attestation2] = await getConflictingAttestations() - const dID1 = createDisputeID(attestation1, indexer.address, fisherman.address) - const dID2 = createDisputeID(attestation2, indexer2.address, fisherman.address) + const dID1 = createQueryDisputeID(attestation1, indexer.address, fisherman.address) + const dID2 = createQueryDisputeID(attestation2, indexer2.address, fisherman.address) const tx = disputeManager .connect(fisherman.signer) .createQueryDisputeConflict( @@ -539,8 +503,8 @@ describe('DisputeManager:Query', async () => { async function setupConflictingDisputes() { const [attestation1, attestation2] = await getConflictingAttestations() - const dID1 = createDisputeID(attestation1, indexer.address, fisherman.address) - const dID2 = createDisputeID(attestation2, indexer2.address, fisherman.address) + const dID1 = createQueryDisputeID(attestation1, indexer.address, fisherman.address) + const dID2 = createQueryDisputeID(attestation2, indexer2.address, fisherman.address) const tx = disputeManager .connect(fisherman.signer) .createQueryDisputeConflict( From 9b286b90ea0e8aeba429f4d111609daa853a854a Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Fri, 30 Apr 2021 01:18:35 -0300 Subject: [PATCH 5/9] disputes: wrap function in only arbitrator modifier --- contracts/disputes/DisputeManager.sol | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 868a73cfe..2f74e7197 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -136,14 +136,22 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa */ event DisputeLinked(bytes32 indexed disputeID1, bytes32 indexed disputeID2); + // -- Modifiers -- + + function _onlyArbitrator() internal view { + require(msg.sender == arbitrator, "Caller is not the Arbitrator"); + } + /** * @dev Check if the caller is the arbitrator. */ modifier onlyArbitrator { - require(msg.sender == arbitrator, "Caller is not the Arbitrator"); + _onlyArbitrator(); _; } + // -- Functions -- + /** * @dev Initialize this contract. * @param _arbitrator Arbitrator role From 59157c8705c7cda991b6609ede463c6e542d2201 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Fri, 30 Apr 2021 01:20:37 -0300 Subject: [PATCH 6/9] [N01] add a Null value to the dispute type enum to avoid using 0 --- contracts/disputes/IDisputeManager.sol | 2 +- test/disputes/poi.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/disputes/IDisputeManager.sol b/contracts/disputes/IDisputeManager.sol index e8d940e97..99a8b8d0d 100644 --- a/contracts/disputes/IDisputeManager.sol +++ b/contracts/disputes/IDisputeManager.sol @@ -6,7 +6,7 @@ pragma experimental ABIEncoderV2; interface IDisputeManager { // -- Dispute -- - enum DisputeType { IndexingDispute, QueryDispute } + enum DisputeType { Null, IndexingDispute, QueryDispute } // Disputes contain info necessary for the Arbitrator to verify and resolve struct Dispute { diff --git a/test/disputes/poi.test.ts b/test/disputes/poi.test.ts index 6a3adce6d..420e34955 100644 --- a/test/disputes/poi.test.ts +++ b/test/disputes/poi.test.ts @@ -22,7 +22,7 @@ import { MAX_PPM } from './common' const { keccak256 } = utils -describe.only('DisputeManager:POI', async () => { +describe('DisputeManager:POI', async () => { let other: Account let governor: Account let arbitrator: Account From d5487d80f9dd099f2970ea98e34ff30813c7693e Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Fri, 30 Apr 2021 01:27:13 -0300 Subject: [PATCH 7/9] [L02] better define the attestation size --- contracts/disputes/DisputeManager.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 2f74e7197..4357a3635 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -53,14 +53,17 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa // -- Constants -- - uint256 private constant ATTESTATION_SIZE_BYTES = 161; + // Attestation size is the sum of the receipt (96) + signature (65) + uint256 private constant ATTESTATION_SIZE_BYTES = RECEIPT_SIZE_BYTES + SIG_SIZE_BYTES; uint256 private constant RECEIPT_SIZE_BYTES = 96; uint256 private constant SIG_R_LENGTH = 32; uint256 private constant SIG_S_LENGTH = 32; + uint256 private constant SIG_V_LENGTH = 1; uint256 private constant SIG_R_OFFSET = RECEIPT_SIZE_BYTES; uint256 private constant SIG_S_OFFSET = RECEIPT_SIZE_BYTES + SIG_R_LENGTH; uint256 private constant SIG_V_OFFSET = RECEIPT_SIZE_BYTES + SIG_R_LENGTH + SIG_S_LENGTH; + uint256 private constant SIG_SIZE_BYTES = SIG_R_LENGTH + SIG_S_LENGTH + SIG_V_LENGTH; uint256 private constant UINT8_BYTE_LENGTH = 1; uint256 private constant BYTES32_BYTE_LENGTH = 32; From 7709b944fdbef185a06d48937a099f0b4b19339b Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Fri, 30 Apr 2021 13:03:30 -0300 Subject: [PATCH 8/9] [L03] use a different event when update the two variables for slashing percentages --- contracts/disputes/DisputeManager.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 4357a3635..8c179a783 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -279,7 +279,8 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa ); qrySlashingPercentage = _qryPercentage; idxSlashingPercentage = _idxPercentage; - emit ParameterUpdated("slashingPercentage"); + emit ParameterUpdated("qrySlashingPercentage"); + emit ParameterUpdated("idxSlashingPercentage"); } /** From 3b38f9e6d82b7cf7169cad111d9c9cf4cdea3326 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Fri, 30 Apr 2021 13:06:57 -0300 Subject: [PATCH 9/9] [L04] add a note in the documentation to clarify that accept dispute would fail under certain conditions and that best course of action is to resolve using draw or reject --- contracts/disputes/DisputeManager.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/disputes/DisputeManager.sol b/contracts/disputes/DisputeManager.sol index 8c179a783..076cfe3c8 100644 --- a/contracts/disputes/DisputeManager.sol +++ b/contracts/disputes/DisputeManager.sol @@ -548,6 +548,9 @@ contract DisputeManager is DisputeManagerV1Storage, GraphUpgradeable, IDisputeMa /** * @dev The arbitrator accepts a dispute as being valid. + * This function will revert if the indexer is not slashable, whether because it does not have + * any stake available or the slashing percentage is configured to be zero. In those cases + * a dispute must be resolved using drawDispute or rejectDispute. * @notice Accept a dispute with ID `_disputeID` * @param _disputeID ID of the dispute to be accepted */