[feat] add multiple arbitrable transaction #163
Conversation
Pull Request Test Coverage Report for Build 830
💛 - Coveralls |
n1c01a5
changed the title
There was a problem hiding this comment.
Did you looks at src/contracts/abstractions/Arbitrable.js? That should be able to be abstracted on top of both ArbitrableTransaction and MultipleArbitrableTransaction. The interfaces interact with the abstract contract and if methods don't exist it delegates to the underlying implementation (what you created). Looks like deploy might need to be cleaned up in the arbitrable abstraction.
src/constants/contract.js
Outdated
| WAITING_PARTY_A: 1, | ||
| WAITING_PARTY_B: 2, | ||
| WAITING_BUYER: 2, | ||
| WAITING_SELLER: 1, |
| { | ||
| from: account, | ||
| value: this._Web3Wrapper.toWei(value, 'ether'), | ||
| gas: 800000 // FIXME gas hardcoded maybe use estimateGas before |
There was a problem hiding this comment.
It would be good to add estimate gas to the Web3Wrapper class
| * @param {number} arbitrationCost - Arbitration cost. | ||
| * @returns {object} - The result transaction object. | ||
| */ | ||
| payArbitrationFeeByBuyer = async ( |
There was a problem hiding this comment.
Buyer/Seller seems a little use case specific. What about something like Sender/Recipient?
There was a problem hiding this comment.
buyer/seller sounds generic for me and it's this wording buyer/seller is used in the smart contract
There was a problem hiding this comment.
Ah ok. To me it sounds like it has to be for a transaction of goods. But if its in the smart contract it is fine
| * @param {number} transactionId - The index of the transaction. | ||
| * @returns {object} - The result transaction object. | ||
| */ | ||
| payArbitrationFeeBySeller = async ( |
| @@ -1,4 +1,4 @@ | |||
| import arbitrableTransactionArtifact from 'kleros-interaction/build/contracts/ArbitrableTransaction' | |||
| import arbitrableTransactionArtifact from 'kleros-interaction/build/contracts/MultipleArbitrableTransaction' | |||
There was a problem hiding this comment.
why change this one? We should leave single arbitrable is the old artifact
|
And If it isn't possible to have an abstracted deploy it should be removed from the abstract contract @n1c01a5 |
| * @param {number} transactionId - The index of the transaction. | ||
| * @returns {object} Object Data of the contract. | ||
| */ | ||
| getData = async transactionId => { |
There was a problem hiding this comment.
This should be called contractId. transactionId is a commonly used term for the tx hash. Also you are referencing the contract at an index not a transaction unless I am misunderstanding what this is for
There was a problem hiding this comment.
I agree but basically I prefer used the wording from the smart contract. And the method is createTransaction so the keyword is transaction no contract (and contract can do reference to a smart contract :p)
There was a problem hiding this comment.
Can we still do something different than transactionId since that is used for tx hashes? Perhaps transactionIndex
There was a problem hiding this comment.
Or arbitrableTransactionId would work too and is also more descriptive
to avoid confusing between the txHash and a arbitrable transaction
the store must also updated to match with this api
| _party: this._Web3Wrapper.getAccount(0), | ||
| _transactionId: this.arbitrableTransactionId | ||
| _evidence: evidenceJsonLink |
There was a problem hiding this comment.
I don't think this is going to work. _party and _evidence are not indexed. Also the only reason that we are looking up the event is to fetch the evidenceJsonLink. You shouldn't be able to pass it to getEvidence or else it isn't coming from the blockchain
| return metaEvidenceResponse.body || metaEvidenceResponse | ||
| } | ||
|
|
||
| /** | ||
| * Get the evidence submitted in a dispute. | ||
| */ | ||
| getEvidence = async () => { | ||
| getEvidence = async evidenceJsonLink => { |
There was a problem hiding this comment.
How can we pass this? We need to get it form the chain
| @@ -45,7 +25,7 @@ class MultipleArbitrable extends ContractImplementation { | |||
| * one meta-evidence that is submitted on contract creation. Look up meta-evidence event | |||
| * and make an http request to the resource. | |||
| */ | |||
| getMetaEvidence = async () => { | |||
| getMetaEvidence = async (arbitrableTransactionId, metaEvidenceJsonLink) => { | |||
There was a problem hiding this comment.
I don't understand why we need these
| _transactionId: this.arbitrableTransactionId, | ||
| _evidence: this.metaEvidenceJsonLink | ||
| _transactionId: arbitrableTransactionId, | ||
| _evidence: metaEvidenceJsonLink |
There was a problem hiding this comment.
Also it looks like MetaEvidence is not following the standard in MultupleArbitrableTransaction (it calls _metaEvidenceID -> _transactionId in the event). This makes it so it is incompatible with the api. Can you change it to follow the standard? It needs to change in the Dispute event as well
| @@ -1,6 +1,5 @@ | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
This file shouldn't exist. The contract should be made standard compliant and then just use Arbitrable.js to fetch evidence and MetaEvidence
| @@ -28,12 +29,17 @@ class Arbitrable extends ContractImplementation { | |||
| if (this.metaEvidenceCache[this.contractAddress]) | |||
| return this.metaEvidenceCache[this.contractAddress] | |||
|
|
|||
| if (this.arbitrableTransactionId === null) | |||
There was a problem hiding this comment.
Can you name this metaEvidenceID (not all arbitrable contracts use transactionId as the metaEvidenceID)?
There was a problem hiding this comment.
Also pass it as a param instead of using this.arbitrableTransactionID and have it default to 0
| * @returns {object} truffle-contract Object | err The deployed contract or an error | ||
| */ | ||
| createArbitrableTransaction = async ( | ||
| account = this._Web3Wrapper.getAccount(0), |
There was a problem hiding this comment.
I have been taking all these defaults for account out. I think its better if the caller passes the correct account. Especially if its the first param they will have to do it anyways since there are no kwargs
There was a problem hiding this comment.
I don't understand what you say. Can you write the parameters that you want?
tests/integration/contracts.test.js
Outdated
| partyB, | ||
| arbitrationCost.minus(partyBFeeContractInstance) | ||
| // seller pays fee | ||
| const raiseDisputeBySellerTxObj = await ArbitrableTransactionInstanceInstance.payArbitrationFeeBySeller( |
There was a problem hiding this comment.
this test fail. I have no idea why
(WARNING)
createTransactiontransaction has a hardcodedgas limit