Optimized Attestation view calls and removal of the reveal TX #1578

nambrot · 2019-11-04T23:53:16Z

Description

This PR tries to reduce the number of view calls a client has to make to receive attestations. It does so by introducing two view calls:

Attestations#getCompletableAttestationStates
It directly returns the attestation issuers for which a user can complete attestations for (instead of having to iterate over all selected issuers themselves)

Accounts#batchGetMetadataURL
Fetches the metdata URLs for a set of accounts (instead of having to do N view calls)

This PR also removes any mention of the reveal transaction

Tested

on namoffchainreveal
unit tests

Related issues

…ions

codecov · 2019-11-06T03:01:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2664e2b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1578   +/-   ##
=========================================
  Coverage          ?   73.78%           
=========================================
  Files             ?      279           
  Lines             ?     7606           
  Branches          ?      657           
=========================================
  Hits              ?     5612           
  Misses            ?     1881           
  Partials          ?      113

Flag	Coverage Δ
#mobile	`73.78% <ø> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2664e2b...eb9d87b. Read the comment docs.

jmrossy · 2019-11-06T10:16:53Z

packages/utils/src/collections.ts

@@ -18,6 +18,16 @@ export function zip3<A, B, C>(as: A[], bs: B[], cs: C[]) {
  return res
 }

+export function compact<A>(as: Array<A | null>): A[] {


Seems like this could be done with .filter?

Great point, originally filter didn't properly get rid of the type, but i can use the custom defined type guard below

jmrossy · 2019-11-06T10:19:59Z

packages/contractkit/src/wrappers/Attestations.ts

+          }
+        } catch (error) {
+          console.error(error)
+          return null


Consider allowing the error to propagate

I was thinking about it, but it seemed to me that a validator not or incorrectly registering an attestation service url should not result in an error for a client who just wants to complete attestations. Thus, I favored returning the attestations that are completable, by intent of the function.

jmrossy · 2019-11-06T10:21:05Z

packages/contractkit/src/wrappers/Attestations.ts

-      this.contract.methods.reveal(
-        PhoneNumberUtils.getPhoneHash(phoneNumber),
-        encryptedPhone,
+  async requestAttestationFromIssuer(


Maybe reveal is a better name here? or something else. I find this is confusing given that requesting an attestation is already a step in the verification process

I'm always open to naming advice, I opted for requestFromIssuer because that's what it represents imo (vs. requesting from the smart contract). reveal made more sense when it was on-chain as we encrypted the number to the validator, but now we are explicitly requesting from a validator's service

+1 to two "request"s being confusing. In some sense, we are revealing our phone number here to the issuer. Alternatively, maybe the original request is more of a commitment to request attestations?

I like commit a lot, but many related structs and events do make more sense as request for that transaction. Here are the steps currently:

request - "commits" to a number of attestations, we have that commitment as an "unselected request" right now

selectIssuers - "selects" on an "unselected request", and persists the actual attestations with their corresponding issuers

requestFromIssuer - advances an attestation by "revealing" the phone number to an issuer, prompting an issuer to send the SMS

complete - completes the attestation by posting the signed message from the SMS.

I still don't quite like reveal but given my lack of alternative suggestions, I'll move it to reveal

asaj · 2019-11-06T20:20:36Z

Do you have any information on whether or not this actually makes things faster?

asaj · 2019-11-06T20:20:53Z

packages/protocol/contracts/identity/Attestations.sol

+    AttestedAddress storage state = identifiers[identifier].attestations[account];
+    address[] storage issuers = state.selectedIssuers;
+
+    uint num = 0;


uint256 everywhere

asaj · 2019-11-06T20:20:58Z

packages/protocol/contracts/identity/Attestations.sol

+    * @param account Address of the account.
+    * @return ( blockNumbers[] - Block number of request/completion the attestation,
+    *           issuers[] - Address of the issuer,
+    *


asaj · 2019-11-06T20:21:25Z

packages/protocol/contracts/identity/Attestations.sol

@@ -238,46 +230,6 @@ contract Attestations is
    delete state.unselectedRequests[msg.sender];
  }

-  /**


Document this in the PR desrcriptinn. If anything, maybe this should be the title of the PR

asaj · 2019-11-06T20:21:47Z

packages/protocol/contracts/common/Accounts.sol

+    view
+    returns (uint[] memory, bytes memory)
+  {
+    uint totalSize = 0;


asaj · 2019-11-06T20:22:30Z

Do you have any information on whether or not this actually makes things faster?

Also please be sure to follow the style guide.

nambrot · 2019-11-06T21:07:13Z

We have no concrete information on whether this makes things actually faster or not. However theoretically, we move from 2*N + 1 view calls to 1 view call (where N = 3). It might be a negligent number once we improve light client performance, but in the meantime this seems like an easy win.

My bad on uint vs uint256, it's been a while since I've been allowed to use all 256 bits :)

asaj · 2019-11-06T22:39:00Z

packages/attestation-service/config/config.json

@@ -0,0 +1,21 @@
+{


Didn't review this

asaj · 2019-11-06T22:40:12Z

packages/contractkit/src/wrappers/Attestations.ts

@@ -179,50 +176,50 @@ export class AttestationsWrapper extends BaseWrapper<Attestations> {
  }

  /**
-   * Returns an array of attestations that can be completed, along with the issuers public key
+   * Returns an array of attestations that can be completed, along with the issuers attestation service url


nit: issuer's

asaj · 2019-11-06T22:41:07Z

packages/contractkit/src/wrappers/Attestations.ts

-          .then(parseAttestationInfo)
-      )
+
+    const result = await this.contract.methods.getCompletableAttestations(phoneHash, account).call()


Please define a helper method that parses this into a more readable object

asaj · 2019-11-06T22:43:14Z

packages/contractkit/src/wrappers/Attestations.ts

-      this.contract.methods.reveal(
-        PhoneNumberUtils.getPhoneHash(phoneNumber),
-        encryptedPhone,
+  async requestAttestationFromIssuer(


+1 to two "request"s being confusing. In some sense, we are revealing our phone number here to the issuer. Alternatively, maybe the original request is more of a commitment to request attestations?

asaj · 2019-11-06T22:46:59Z

packages/protocol/contracts/identity/Attestations.sol

-   * @param issuer The issuer of the attestation.
-   * @param sendSms Whether or not to send an SMS. For testing purposes.
-   */
-  function reveal(


asaj · 2019-11-06T22:56:13Z

packages/protocol/contracts/common/Accounts.sol

+    uint256[] memory sizes = new uint256[](accountsToQuery.length);
+    for (uint256 i = 0; i < accountsToQuery.length; i++) {
+      sizes[i] = bytes(accounts[accountsToQuery[i]].metadataURL).length;
+      totalSize += sizes[i];


safemath everywhere

packages/protocol/contracts/identity/Attestations.sol

packages/protocol/contracts/common/Accounts.sol

packages/protocol/contracts/common/interfaces/IAccounts.sol

asaj · 2019-11-07T23:09:22Z

packages/protocol/contracts/common/Accounts.sol

@@ -1,29 +1,26 @@
 pragma solidity ^0.5.3;

-import "openzeppelin-solidity/contracts/utils/ReentrancyGuard.sol";
-import "openzeppelin-solidity/contracts/math/SafeMath.sol";
+import 'openzeppelin-solidity/contracts/utils/ReentrancyGuard.sol';


I thought we went with double quotes?

Fixed with an upstream merge

* master: (56 commits) Adjust e2e transfer and governance tests to match new fee distribution and eliminate ProposerFraction (#1585) [Wallet] Add more local currencies (#1698) Switch to correct cluster when fauceting (#1687) [Wallet] Use the country of the phone number for determining the default local currency (#1684) [Wallet] Limit QR code scanner to 1 code per second (#1676) Update Dark backgrounds text color (#1677) Remove integration sync test Minor attestation service fixes (#1680) [wallet] Fixed Native phone picker Use native API instead (#1669) Fix token addresses for notification service (#1674) Add golang to setup docs [wallet] Hide invite education copy after invite was redeemed (#1670) [Wallet] Add spinner, timer, and tip text to Verification input screen (#1656) [Wallet] Fix app deprecation check mechanism (#1358) Point end-to-end governance test back to master (#1665) Add EpochRewards smart contract to calculate epoch rewards and payments (#1558) Optimized Attestation view calls and removal of the reveal TX (#1578) Support claim signatures and support Keybase claims (#1575) [Wallet] Add timestamp to top banner messages (#1657) Export geth metrics on VM testnet (#1351) ... # Conflicts: # yarn.lock

…#1578)" This reverts commit 7e3d507.

Support more efficient fetching of information in regards to attestat…

c6030f5

…ions

nambrot requested review from asaj and m-chrzan as code owners November 4, 2019 23:53

nambrot added 3 commits November 4, 2019 18:02

Address lint

63d0bcb

Fix lint

31f9628

Support off-chain reveal in Contractkit/Celotool

16ffc20

nambrot requested a review from jmrossy as a code owner November 6, 2019 03:01

nambrot added 3 commits November 5, 2019 19:23

Include numeral in utils

74f329b

Remove reveal tx

975a8eb

Fix lint

75ffa26

nambrot mentioned this pull request Nov 6, 2019

Deployment fixes for attestation-service #1607

Merged

1 task

Rename to getCompletableAttestations

e155778

nambrot assigned jmrossy and asaj Nov 6, 2019

jmrossy reviewed Nov 6, 2019

View reviewed changes

jmrossy assigned nambrot and unassigned jmrossy Nov 6, 2019

asaj reviewed Nov 6, 2019

View reviewed changes

nambrot changed the title ~~Support more efficient fetching of information in regards to attestations~~ Optimized Attestation view calls and removal of the reveal TX Nov 6, 2019

PR comments

5aa4c55

nambrot mentioned this pull request Nov 6, 2019

Remove all identity-related code from celo-blockchain celo-org/celo-blockchain#572

Merged

1 task

Fix e2e test

f56acd7

asaj reviewed Nov 6, 2019

View reviewed changes

asaj removed their assignment Nov 6, 2019

nambrot added 2 commits November 6, 2019 15:31

PR comments

9268525

Fix lint

47cc6db

nambrot added 2 commits November 6, 2019 16:39

Fix lint

0b5d736

Rename to reveal

03991da

nambrot assigned asaj and unassigned nambrot Nov 7, 2019

Merge branch 'master' into nambrot/actionable-attestations

e576e79

nambrot assigned jmrossy Nov 7, 2019

asaj approved these changes Nov 7, 2019

View reviewed changes

asaj assigned nambrot and unassigned asaj Nov 7, 2019

Merge branch 'master' into nambrot/actionable-attestations

9ef5d97

nambrot removed their assignment Nov 7, 2019

Fix lint from prettify

4cd276d

jmrossy approved these changes Nov 11, 2019

View reviewed changes

nambrot added the automerge Have PR merge automatically when checks pass label Nov 11, 2019

nambrot and others added 3 commits November 11, 2019 10:59

Merge branch 'master' into nambrot/actionable-attestations

bc3a01f

Merge refs/heads/master into nambrot/actionable-attestations

6b624c9

Merge refs/heads/master into nambrot/actionable-attestations

eb9d87b

celo-ci-bot-user merged commit 7e3d507 into master Nov 11, 2019

celo-ci-bot-user deleted the nambrot/actionable-attestations branch November 11, 2019 20:20

cmcewen added a commit that referenced this pull request Nov 15, 2019

Revert "Optimized Attestation view calls and removal of the reveal TX (…

c9086b4

…#1578)" This reverts commit 7e3d507.

This was referenced Jul 31, 2022

[Snyk] Fix for 7 vulnerabilities leojoy95/celo-monorepo#497

Open

[Snyk] Fix for 2 vulnerabilities leojoy95/celo-monorepo#501

Open

snyk-bot mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.2.10 leojoy95/celo-monorepo#502

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimized Attestation view calls and removal of the reveal TX #1578

Optimized Attestation view calls and removal of the reveal TX #1578

nambrot commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 6, 2019 •

edited

Loading

jmrossy Nov 6, 2019

nambrot Nov 6, 2019

jmrossy Nov 6, 2019

nambrot Nov 6, 2019

jmrossy Nov 6, 2019

nambrot Nov 6, 2019

asaj Nov 6, 2019

nambrot Nov 7, 2019 •

edited

Loading

asaj commented Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj commented Nov 6, 2019

nambrot commented Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 6, 2019

asaj Nov 7, 2019

nambrot Nov 7, 2019

Optimized Attestation view calls and removal of the reveal TX #1578

Optimized Attestation view calls and removal of the reveal TX #1578

Conversation

nambrot commented Nov 4, 2019 • edited Loading

Description

Tested

Related issues

codecov bot commented Nov 6, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nambrot Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

asaj commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asaj commented Nov 6, 2019

nambrot commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nambrot commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 6, 2019 •

edited

Loading

nambrot Nov 7, 2019 •

edited

Loading