Skip to content

Conversation

abarmat
Copy link
Contributor

@abarmat abarmat commented Mar 11, 2020

This PR implements Disputes on a different contract that interacts with the Staking contract.

Spec:
https://www.notion.so/thegraph/Disputes-51960724e8464c119ce4b249c7c1f425

Tasks:

  • add disregard dispute
  • remove bool return types for some methods
  • add all require error texts
  • set the staking contract relationship from this contract
  • reorganize function and vars according to solidity style guide
  • add a function to slash on the staking contract
  • have the possiblity to have many slashers for the staking contract
  • have the domain hash eip-712 calculated in the constructor
  • replace indexing node with indexNode
  • move parse of attestation out of the createDispute function
  • add zeppelin math lib
  • review how the attestation is built and the signatures
  • add lint for JS tests
  • rewrite tests
    x dispute deposit below what is required
    x two exactly equal disputes should be rejected
    x reject dispute
    x ignore dispute
    x accept dispute
    x dispute manager not allowed to slash
  • update specs with latest interface
  • reject token transfers to this contract without proper _data format
  • review all natspec
  • can we use chain.id?
  • support for conflicting attestations

@davekay100 davekay100 self-requested a review March 17, 2020 16:57
Copy link
Contributor

@davekay100 davekay100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just small changes, but I was unable to get the tests running. I was getting this error:

Error: web3@1.2.1 detected, incompatible with requirement of web3@1.0.0-beta.37

Any ideas how to get past it?

@abarmat
Copy link
Contributor Author

abarmat commented Mar 18, 2020

I'm pushing some fixes based on the review. I fixed web3 problems by bumping version of openzeppelin/test-helpers library and others.

abarmat added 3 commits March 18, 2020 02:21
- remove unnecessary cast
- check empty arbitrator + test
- improve error messages
- use "Dispute memory dispute" when possible
- add comments
@abarmat abarmat mentioned this pull request Mar 18, 2020
Copy link
Contributor

@davekay100 davekay100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates look good, I approve, but I see you still have a few unchecked boxes. Up to you if you want to include these in this PR, or we just merge this and go forward

@abarmat abarmat merged commit 848a96a into master Mar 19, 2020
@abarmat abarmat deleted the feat/disputes branch March 19, 2020 14:12
)
}

function createAttestationHash(attestation) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? This doesn't quite match up with how I interpret the spec here: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-hashstruct

hashStruct(s : 𝕊) = keccak256(typeHash ‖ encodeData(s)) where typeHash = keccak256(encodeType(typeOf(s)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function that is performing that is this one:

function createAttestationHash(attestation) {
  const attestationTypeHash = web3.utils.sha3(
    'Attestation(IpfsHash requestCID,IpfsHash responseCID,uint256 gasUsed,uint256 responseNumBytes)IpfsHash(bytes32 hash,uint16 hashFunction)',
  )

  // ABI encoded
  return web3.utils.sha3(
    web3.eth.abi.encodeParameters(
      ['bytes32', 'bytes'],
      [attestationTypeHash, attestation],
    ),
  )
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarifying 👍

@abarmat abarmat changed the title [WIP] feat: Disputes feat: Disputes Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants