Added MultipleArbitrableTransaction.sol and fixed issue #27 #32
Conversation
…trableTransaction. Implement withdraw function, remove pay function according to kleros#27
|
|
||
| Transaction[] transactions; | ||
|
|
||
| mapping (uint => uint) disputeTxMap; |
There was a problem hiding this comment.
Dispute IDs can be duplicated across arbitrators so this won't work.
There was a problem hiding this comment.
hmm, noted. Let me see if I can find a workaround.
Changed key to a bytes32 type in order to store a hash of (arbitrator, disputeID) to address the fact that a disputeID can be the same for different arbitrators. Added a test for that scenario.
As the comment stated, parties can pay more. This is required to cope with the case where the arbitrator decreases the fee.
| * @param _party The address of the party submiting the evidence. Note that 0 is kept for evidences not submitted by any party. | ||
| * @param _evidence A link to evidence or if it is short the evidence itself. Can be web link ("http://X"), IPFS ("ipfs:/X") or another storing service (using the URI, see https://en.wikipedia.org/wiki/Uniform_Resource_Identifier ). One usecase of short evidence is to include the hash of the plain English contract. | ||
| */ | ||
| event Evidence(uint indexed _transactionId, Arbitrator indexed _arbitrator, uint indexed _disputeID, address _party, string _evidence); |
There was a problem hiding this comment.
Evidence is defined in Arbitrable https://github.com/kleros/kleros-interaction/blob/master/contracts/standard/arbitration/Arbitrable.sol#L45
To keep consistency, it's better to use the same evidence event (without the _transactionID which can be recovered form _party and _disputeID).
There was a problem hiding this comment.
@clesaege _party and _disputeID does not make it unique in this contract because we can have 2 transactions with the same party and different arbitrators. That's why I brought Evidence event to this contract instead of using the Arbitrable one. Please let me know what to do regarding this.
There was a problem hiding this comment.
No, but _disputeID and _arbitrator makes it unique.
ethereum/EIPs#792 : It must track the disputes by their (arbitrator,disputeID) unique key
There was a problem hiding this comment.
You are absolutely right @clesaege . I have made the requested changes now.
|
|
||
| uint arbitrationCost = transaction.arbitrator.arbitrationCost(transaction.arbitratorExtraData); | ||
| transaction.sellerFee += msg.value; | ||
| require(transaction.sellerFee == arbitrationCost); // Require that the total pay at least the arbitration cost. |
There was a problem hiding this comment.
It should be >= so it's at least. Otherwise it would block when the arbitrator lowers the fees. Note that it was a bug in the ArbitrableTransaction which is no fixed.
|
|
||
| uint arbitrationCost = transaction.arbitrator.arbitrationCost(transaction.arbitratorExtraData); | ||
| transaction.buyerFee += msg.value; | ||
| require(transaction.buyerFee == arbitrationCost); // Require that the total pay at least the arbitration cost. |
There was a problem hiding this comment.
Same as for the seller one. == -> >=
| require(msg.sender == transaction.buyer || msg.sender == transaction.seller); | ||
|
|
||
| require(transaction.status>=Status.DisputeCreated); | ||
| Evidence(_transactionId, transaction.arbitrator,transaction.disputeId,msg.sender,_evidence); |
There was a problem hiding this comment.
We can remove the _transactionId which will make it consistent with other Arbitrable contracts.
| /** @dev | ||
| * returns the last transaction id whose buyer is the sender | ||
| */ | ||
| function lastTransactionId () constant public returns (uint index) { |
There was a problem hiding this comment.
This has a O(n) complexity and thus should only be used by the UI (we need to warn about that).
I also don't think this function is required.
There was a problem hiding this comment.
@clesaege I only use this in tests. Can I keep it? :)
There was a problem hiding this comment.
Do you have a way to do without it? If so that would be better in order to lower code complexity (code ,complexity in tests is better than code complexity in smart contracts).
There was a problem hiding this comment.
Yes, I'll figure it out
| Status status; | ||
| } | ||
|
|
||
| Transaction[] transactions; |
|
|
||
| Transaction[] transactions; | ||
|
|
||
| mapping (bytes32 => uint) disputeTxMap; |
| } | ||
|
|
||
| /** @dev Transfer the transaction's amount to the seller if the timeout has passed | ||
| * @param _transactionId The index of the transaction. |
| Transaction storage transaction = transactions[_transactionId]; | ||
| require(msg.sender == transaction.seller); | ||
| require(now>=transaction.lastInteraction+transaction.timeout); | ||
|
|
There was a problem hiding this comment.
We also need to make sure it's not disputed, otherwise people could withdraw even if a dispute is raised.
There was a problem hiding this comment.
require(transaction.status==Status.NoDispute);
| uint256 amount; | ||
| uint256 timeout; // Time in seconds a party can take before being considered unresponding and lose the dispute. | ||
| uint disputeId; | ||
| bool disputed; |
There was a problem hiding this comment.
disputed seems redundant with status, I think we can remove disputed
MultipleArbitrableTransaction mimics the functionality of Arbitrable transaction with the difference that it's deployed once and functions are invoked indicating the transaction ID.
A new set of tests were added. Those tests includes the ones already being tested against ArbitrableTransaction.sol and some others that tests the fact that multiple transactions can be possible.
Implemented withdraw function and removed pay function according to #27
Please let me know your feedback @clesaege @epiqueras @n1c01a5