search

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

Add contract method to remove a registered address #100

Closed fvictorio closed 6 years ago

fvictorio commented 6 years ago

Depends on #98.

phahulin commented 6 years ago

Also, maybe move blochchain/build into .gitignore?

fvictorio commented 6 years ago

Also, maybe move blochchain/build into .gitignore?

It was already in .gitignore, but it was also added for some reason. I removed it.

fvictorio commented 6 years ago

This is ready for review now.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 282

Totals Coverage Status
Change from base Build 261: 1.9%
Covered Lines: 828
Relevant Lines: 1060
💛 - Coveralls
fvictorio commented 6 years ago

@phahulin @matiasgaratortiz Can we merge this?

phahulin commented 6 years ago

@igorbarinov philosophical question: in PoPA contract we have three variables for statistics: totalUsers, totalAddresses, totalConfirmed, incremented respectively when new user is created, when new address is registered and when address is confirmed.

Since it is possible for a user to remove his own claim directly from ERC780 registry, those 3 variables might not represent actual numbers at a given moment.

Now with this PR we also have an ability to remove addresses from PoPA. My question is what should we do about those variables:

  1. we can remove them from contract. Adv: no misleading numbers, Dis: no knowledge on how often the service is used, since mappings cannot be iterated
  2. keep them and don't decrement on removal. In this case totalUsers will measure how many users ever used PoPA, totalAddresses - how many addresses ever in total were registered, etc. Adv: having some statistics. Dis: need to remember that these variables might not represent actual numbers at the present moment, but an overall sum. For example, if a user registers an address and them removes it, we'll still have totalUser==1, totalAddresses == 1, ...
  3. keep them and decrement on removal. Adv: having some statistics, more accurate than in 2. Dis: Still might be not 100% accurate if user manually removes addresses from ERC780 registry

IMO, we should go with 2. @matiasgaratortiz @fvictorio what do you think?

fvictorio commented 6 years ago

I agree that 2 seems the better option. I don't think 3 is a good idea, since, as you mentioned, we have no way to have those numbers "synced" with whatever happens in the ERC780 regsitry.

Choosing 2 would also mean that we don't have to make any more changes here, right?

phahulin commented 6 years ago

Ok, I think we can merge this now, so that we can continue from here, also, usage of statistical variables is independent of other parts of code. @igorbarinov if you disagree with choosing (2) option above, please write it here and we can address it in a separate PR.

igorbarinov commented 6 years ago

@phahulin I think 2 is fine