search

poanetwork / poa-dapps-validators

DApp for a list of validators with metadata for POA Network (Core/Sokol). Validators can update metadata using DApp.
https://validators.poa.network
GNU General Public License v3.0
13 stars 41 forks source link

(Feature, Proof Of Concept) Retrieve validators' physical addresses from PoPA DApp #75

Closed unjapones closed 6 years ago

unjapones commented 6 years ago

~Depends on https://github.com/poanetwork/poa-chain-spec/pull/79~ (merged).

Description

Implements:

Add the following:

Important considerations

Screenshots, demo

The following screen shows an example scenario:

78_multiple

varasev commented 6 years ago

Proof of Physical Address contract ABI/JSON file is copied (from poa-popa) & added directly to the code base via this PR, and the address of the corresponding contract is hardcoded for core network.

I'd suggest to add the ABI and the address of PoPA contract to https://github.com/poanetwork/poa-chain-spec repo and read ABI/address from there as it is done for other consensus contracts.

modify AllValidators.js UI container to use the file above and augment each validator's data with the confirmed piece of information.

I think it's better to move initialization of PoPA contract from AllValidators.js to index.js in getWeb3() function as it is done for ValidatorMetadata and KeysManager contracts. E.g.:

this.getPoPAContract = this.getPoPAContract.bind(this)

the JS file/module /src/contracts/ProofOfPhysicalAddress.contract.js to query the confirmed physical addresses of the validators.

There are many requests made by ProofOfPhysicalAddress.contract.js. Is it possible to make only one request per validator? Maybe PoPA smart contract provides some batch function to retrieve validator's addresses count, addresses array, and confirmation statuses in a single request?

unjapones commented 6 years ago

@varasev thank you for the feedback. I'll work on the following things:

  1. Initialize PoPA contract the same way the other contracts are being initialized. For this, I may fork poa-chain-spec, create the corresponding branch(es) and adapt the code in this PR.
  2. Try to optimize the requests/queries being made to the PoPA contract as you mentioned (look for batch info gathering).

For now, I'll assume the design/UI appearance of this is ok, and that we may ask the designer for his/her opinion later.

unjapones commented 6 years ago

@varasev implemented the item "1" from my previous comment. I had to fork poa-chain-spec and commit to the core branch in my fork in order to test this (plus, I changed a constant that I'll revert back to its original value). The description of the PR here mentions this dependency.

Regarding item 2, it looks like we can't make the optimization: PoPA's contract uses a mapping involving the wallet/addresses that registered or confirmed a physical address, meaning we can't get all these wallet/addresses (the keys of the mapping) unless they are stored in an array on the contract. Without this array, we are not able to know in advance the individual validators that have a physical address in PoPA; forcing us to query each validator data one by one.

varasev commented 6 years ago

@unjapones

What if we use LogAddressConfirmed event? https://github.com/poanetwork/poa-popa/blob/master/blockchain/contracts/ProofOfPhysicalAddress.sol#L392

We could read that event with web3's getPastEvents method (https://web3js.readthedocs.io/en/1.0/web3-eth-contract.html#getpastevents) to know if the specified validator has confirmed address and to know its keccakIdentifier to find confirmed address with userAddressByKeccakIdentifier function: https://github.com/poanetwork/poa-popa/blob/master/blockchain/contracts/ProofOfPhysicalAddress.sol#L178

Maybe this would help to reduce the number of requests to PoPA smart contract for each validator?

Also, if PoPA is only for Core network, Validators DApp shouldn't try to find a confirmed address for Sokol version.

unjapones commented 6 years ago

@varasev done with the latest set of changes requested.

varasev commented 6 years ago

@unjapones Thanks for optimization. Now I think we could implement the logic described in https://github.com/poanetwork/poa-dapps-validators/issues/78#issuecomment-417268863

unjapones commented 6 years ago

@varasev :cool: I'll start working on what you described on #78.

unjapones commented 6 years ago

@varasev incorporated here what we agreed about multiple addresses and their confirmed/unconfirmed status. Also updated the description of the PR accordingly.

varasev commented 6 years ago

@unjapones

incorporated here what we agreed about multiple addresses and their confirmed/unconfirmed status. Also updated the description of the PR accordingly.

1) At this moment DApp scans only confirmed addresses. We need to display both confirmed and unconfirmed as it's described in that comment.

2) We shouldn't display confirmation icons for Sokol version.

3) Let's display Confirmed Address tooltip on hover above the green icon and Unconfirmed Address on hover above the gray icon. I think it's enough to add title HTML attribute for that.

varasev commented 6 years ago

@unjapones Please also check that address is displayed correctly for different width of browser's client area.

unjapones commented 6 years ago

@varasev applied the changes from your last comment in this PR:

  1. Retrieving the logs from the "registered" physical addresses from PoPA to get all addresses. Also, the scenario where a user deregistered an address (the DApp should not show it) should be automatically covered.

  2. Did a little refactor on the corresponding UI components to render the confirmed icon only if isConfirmed is present on the data structure used to represent a physical address. When using the address from Metadata (i.e. only on core network) the isConfirmed flag is not set.

  3. Changed the breakpoint used to trigger the rendering of the fields in column mode, here are two GIFs with the before & after the change:

Before dapp_validators_before_new_breakpoint

After

Note: the first validator has 2 addresses because the unconfirmed one  was hardcoded to display the _multiple addresses from PoPA scenario_.

dapp_validators_new_breakpoint

unjapones commented 6 years ago

@varasev the PR https://github.com/poanetwork/poa-chain-spec/pull/79 has been merged. Did you have a chance to review the PR here?

varasev commented 6 years ago

@unjapones Yes, I'm planning to review your changes in the next 1-2 days.

varasev commented 6 years ago

I've tried to start and view updated DApp - it seems to be looking good. However, there is a little question about unregistered addresses:

Also, the scenario where a user deregistered an address (the DApp should not show it) should be automatically covered.

As far as I understand, ProofOfPhysicalAddress.userAddressByKeccakIdentifier will return not found for the address that was unregistered, am I right?

unjapones commented 6 years ago

As far as I understand, ProofOfPhysicalAddress.userAddressByKeccakIdentifier will return not found for the address that was unregistered, am I right?

Yes, you are right.

I'll improve the doc-block of ProofOfPhysicalAddress.getPhysicalAddressesByWalletAddressAndKeccakIdentifierArray since the method is returning a promise that resolves to the addresses that are confirmed or registered/unconfirmed.

The addresses that were unregistered will not appear in the result since they are not found by ProofOfPhysicalAddress contract. If a validator unregistered each of his/her addresses, then this check in AllValidators UI component will excecute the else part of the code and will fallback to the validator's Metadata address info.

varasev commented 6 years ago

Thanks for the fixes. I think we should also add some short text on the page Set Metadata, something like this:

The entered address will be displayed as Unconfirmed and will
be used if you don't have Registered Address(es) in PoPA DApp. 
You have to use PoPA to register and confirm your address(es).
unjapones commented 6 years ago

@pashagonchar I need to add the message described in this comment to the "Set Metadata" form. Do you have an idea or guideline about where or how should it look like?

@varasev I will also try to improve the form, since I found out that the order of the fields can be improved for narrower resolutions: the Last name fields ends up after the License expiration input, plus the focus switch using the TAB key is not very intuitive (check the GIF below):

setmetadataform

varasev commented 6 years ago

@unjapones Yes, good idea.

pashagonchar commented 6 years ago

@unjapones Zeplin: https://zpl.io/aNoXA7Q poa_validators_2_1

unjapones commented 6 years ago

@varasev applied the changes following @pashagonchar design. Here is a little GIF demo:

dapp-validators_set-metadata-form-updates

Should we make the portion of the text PoPA DApp a link to the actual PoPA DApp? :sunglasses:

varasev commented 6 years ago

Should we make the portion of the text PoPA DApp a link to the actual PoPA DApp?

Yes, let's make the link.

unjapones commented 6 years ago

:+1: Done with the last set of changes.

varasev commented 6 years ago

Done with the last set of changes.

Ok, thanks, I'll review those soon.

pablofullana commented 6 years ago

hey @varasev, friendly ping

varasev commented 6 years ago

Hi @pablofullana @unjapones I had a few high priority tasks during the last few days (we were making a hard fork in Sokol network), so I still had no chance to review the changes, but that's in my TODO and I think I'll try to do it on Friday or at the beginning of the next week. Thanks for your patience :)

varasev commented 6 years ago

The code of the last changes looks good, but I'll also check the DApp with e2e tests in poa-test-setup before merging.