Skip to content
This repository was archived by the owner on Jan 14, 2020. It is now read-only.

Conversation

@tyleryasaka
Copy link
Contributor

@tyleryasaka tyleryasaka commented May 3, 2018

Checklist:

  • Code contains relevant tests for the problem you are solving
  • Ensure all new and existing tests pass
  • Update any relevant READMEs and docs
  • Submit to the develop branch instead of master

Description:

This adds 2 new resources, Attestations and Users. They are extensively documented in my docs PR.

Some other things going on:

  • ERC725 contract restructuring
  • Fixes some migration issues that were making my tests flaky (not sure if anyone else has been experiencing this)
  • Extensive test coverage for the attestations and users resources
  • Adds a couple of conveniences to the contract service for the new web3 setup

Example usage in demo-dapp

Can use attestationServerUrl: "http://bridge-server-test.herokuapp.com/api/attestations" in config. That's a heroku app that's up and running right now.

Just to make this easier to review and understand.

Can stick this in src/services/origin.js (in demo-dapp):

import Origin from 'origin'

let config = {}

config.ipfsDomain = process.env.IPFS_DOMAIN ? process.env.IPFS_DOMAIN : null
config.ipfsApiPort = process.env.IPFS_API_PORT ? process.env.IPFS_API_PORT : null
config.ipfsGatewayPort = process.env.IPFS_GATEWAY_PORT ? process.env.IPFS_GATEWAY_PORT : null
config.ipfsGatewayProtocol = process.env.IPFS_GATEWAY_PROTOCOL ? process.env.IPFS_GATEWAY_PROTOCOL : null
config.attestationServerUrl = "http://bridge-server-test.herokuapp.com/api/attestations"

const origin = new Origin(config)
window.web3 = origin.contractService.web3

let sendCode = () => {
  origin.attestations.phoneGenerateCode({
    phone: "555-555-5555"
  })
}

let createUser = async () => {
  let phoneAttestation = await origin.attestations.phoneVerify({
    phone: "555-555-5555",
    code: "332308"
  })
  let myNewUser = {
    profile: { claims: { name: "Wonder Woman" } },
    attestations: [ phoneAttestation ]
  }
  let user = await origin.users.set(myNewUser)
  console.log('user created', user)
}

// get your verification code, then remove me
sendCode()

// uncomment me once you have your code
// createUser()

export default origin

tyleryasaka added 30 commits May 2, 2018 20:53
This will allow it to be accessed from both ClaimHolder and ClaimHolderPresigned
The name `create` conflicts with a Truffle method (it conflicts with something anyway - I am guessing it's Truffle)
This one is different than the other resources. Takes in different config options.
origin-js should validate all attestations, and only return the ones that are valid.
This is required for now so that issuer will be passed in. We can refactor this later.
Now the user directly calls the identity contracts, and the identity contracts add themselves to the user registry.

This seems cleaner and simpler, but the real motivation to refactor was to allow identity contract addresses to be reliably predicted before they are actually deployed (for creating presigned claims). If the identity contract is deployed directly by the user, then its address can be predicted safely. If it is deployed by a shared user registry contract though, that opens the door to race conditions where other people might be deploying identities at the same time.
We can and should just accept wallet as input - users of origin-js should not have to worry about the identity contract itself (for now). We're managing that behind the scenes.
ClaimHolder -> ClaimHolderRegistered -> ClaimHolderPresigned

This way we can use ClaimHolder on its own without a constructor that tries to add itself to a user registry.
Instead of hard-coding a single key, we have a single "hard-coded" identity contract that can have multiple keys - so we can use multiple keys to sign attestations if we want to or need to. This complies with the design of ERC725/ERC735.
We shouldn't be loading json files and using truffle-contract in the tests. Depending on how the tests are run, the files may not have the correct network addresses. The tests should be able to be run in an isolated environment, regardless of the state of the json files.
We're using a master origin identity contract to keep up with issuers. No need for the config option now. (Eventually might allow people to pass in the address of one or more trusted attestation identities - for now we'll just manage that automatically.)
Once data leaves the attestation resource, we'll assume that it's been hashed
This results in double hashing if your data is already hashed. That was a bad idea in the first place.
This will allow consumers of this library to simply pass in the state that they want (both the profile and attestations). In other words, this efficiently replaces the existing user with whatever is passed in. (The only exception to this is removing attestations. We won't worry about that for now.)
Because we're just fetching from the logs, we want to make sure to get the most recent so that profile updates work. Sooner rather than later we should move away from using the logs like this.
tyleryasaka added 16 commits May 2, 2018 22:48
Of course there's no real distinction between private and public, but it's easier to read if we keep the "public" methods at the top
This is a really nice library that lets us mock the fetch object for http requests. Makes it a breeze to write tests that make external http requests.
This carefully stubs out the server response and asserts that each request uses the correct method, url, and params.

As long as we keep the test expectations in line with the actual bridge server, we get decent confidence that this code will work with the bridge server.
We can do this since we're on 1.0 now. :D
It doesn't add any new functionality, strictly speaking. The same thing could be accomplished by creating a contract that does the same thing. I basically just created this as a convenience for clearing users between tests. We can refactor later if we think of a cleaner solution.
@tyleryasaka tyleryasaka force-pushed the tyleryasaka/attestations-and-users branch from b05f3cc to d18dddb Compare May 4, 2018 00:27
Copy link
Contributor

@nick nick left a comment

Choose a reason for hiding this comment

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

Really awesome work!

"babel-polyfill": "^6.26.0",
"bs58": "^4.0.1",
"cross-fetch": "^2.1.1",
"ethereumjs-util": "^5.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nick No I don't think it's needed. It's currently being used for attestation signature validation.

async isValidAttestation({ claimType, data, signature }, identityAddress) {

We should be able to do all this with web3. Created an issue for it so I don't forget. #153

arguments: args
})
.send(options)
.on("receipt", receipt => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we listen for the "confirmation" event instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nick I'm actually not that familiar with this API. I just looked at what we're doing in ResourceBase.contractFn with waitTransactionFinished and did the same thing here.

https://github.com/OriginProtocol/origin-js/blob/master/src/ResourceBase.js#L24

@joshfraser joshfraser merged commit e47be8c into develop May 4, 2018
@tyleryasaka tyleryasaka deleted the tyleryasaka/attestations-and-users branch May 4, 2018 09:22
@wanderingstan
Copy link
Contributor

@tyleryasaka one nit-pick: when running npm test its spitting out multiple screenfuls of hex. I wonder if some debugging code got left in?
image

@tyleryasaka
Copy link
Contributor Author

tyleryasaka commented May 5, 2018

@wanderingstan That output is coming from the new web3 version 1.0, rather than anything I changed in this PR. The various console.log statements scattered through this codebase are spitting out more verbose items now.

I think we should remove console.log statements from this package entirely. I don't think it's good practice in general.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants