poanetwork / poa-popa

DApp for proof of physical address (PoPA) attestation for validators of POA Network
https://popa.poa.network
GNU General Public License v3.0
24 stars 18 forks source link

(Freature) Add ERC-735 claim generation + PoPA ERC-725 Contract #212

Closed unjapones closed 5 years ago

unjapones commented 5 years ago

Integration branch that includes the code from PRs #208 , #209 + some UI improvements on the ERC-735 claim generation page.

@phahulin one thing that it is still pending:

this PR adds a contract called ProofOfPhysicalAddressKeyHolder.sol which is a different contract than the main ProofOfPhysicalAddress.sol.

The reason:

making ProofOfPhysicalAddress.sol inherit from KeyHolder.sol makes the contract deployment fail on testrpc (generic out of gas error, most probably due to a large contract), breaking any automated testing.

The not so good thing about this is that the address of the "PoPA identity contract" (the address that appears on the issuer field of the ERC-735 claims, and that I highlight in the demo below) holds the address of ProofOfPhysicalAddressKeyHolder.sol and not the main ProofOfPhysicalAddress.sol. I'm not really sure if this is abad thing but we end up with 2 contracts... [1]

@phahulin do you have an opinion on the above? May be we could try a refactor using Solidity's delegatecall, or some other pattern, so we end up with a unique contract that has everything: previous features + new identity/key holder features.

Short demo: popa_erc725_integration

NOTES:

unjapones commented 5 years ago

Just in case, I have just performed the following:

pablofullana commented 5 years ago

@phahulin friendly ping 😬

phahulin commented 5 years ago

I think it's best if we stay with two contracts without the need to upgrade existing PoPA contract. I'm gonna merge this into a new branch (not master), so that we can safely upgrade existing setup, because this PR introduces some breaking changes.