Add selectIssuers Transaction #1327

nambrot · 2019-10-14T16:47:51Z

Description

This PR is the first part for #1314. It adds the commit/reveal transactions, in Attestations speak request/selectIssuers. It does not currently enforce the wait, will be done in a separate pr once #1197 gets merged

record block times
contractkit mods
Store attetation fee token on each attestation

Tested

Unit tests

Other changes

Added SafeCast from Safe Casting Library from uint256 to uintXX OpenZeppelin/openzeppelin-contracts#1926. We should use upstream whenever it gets merged

Backwards compatibility

Once merged, it is not backwards compatible

codecov · 2019-10-14T17:32:50Z

Codecov Report

Merging #1327 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
+ Coverage   73.61%   73.69%   +0.08%     
==========================================
  Files         277      277              
  Lines        7443     7432      -11     
  Branches      956      660     -296     
==========================================
- Hits         5479     5477       -2     
+ Misses       1852     1843       -9     
  Partials      112      112

Flag	Coverage Δ
#mobile	`73.69% <ø> (+0.08%)`	⬆️

Impacted Files	Coverage Δ
packages/mobile/src/navigator/NavigatorWrapper.tsx	`77.77% <0%> (+22.6%)`	⬆️

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 6981eb2...7a3fbcb. Read the comment docs.

packages/protocol/contracts/identity/Attestations.sol

asaj · 2019-10-16T18:11:50Z

packages/protocol/contracts/identity/Attestations.sol

      state.issuers.push(issuer);
    }
  }

-  function isAttestationTimeValid(uint128 attestationTime) internal view returns (bool) {
+  function isAttestationRequestBlockTimeValid(


I think this function name could be more readable, maybe isAttestationRequestExpired?

Named it to isAttestationExpired following the below conversation on what really constitutes a request

packages/protocol/contracts/identity/Attestations.sol

asaj · 2019-10-16T18:15:28Z

packages/protocol/contracts/identity/Attestations.sol

-    require(isAttestationTimeValid(attestation.time), "Attestation request timed out");
+    require(
+      isAttestationRequestBlockTimeValid(attestation.blockNumber),
+      "Attestation request timed out"


Is it the request that timed out here?

Hmm, yeah I just realized that we have two different notions of "request".

First is the request where you commit the attestation fee and one has to call selectIssuers for to set the attestation.

Second is the request for that attestation to be completed to the issuer, either by calling the reveal tx or making a request to the attestation service.

Both represent the notion of a user requesting attestations, albeit at different abstractions.

I'll change it so that only the former is named request, the latter being just attestation, but lmk if that is also confusing

asaj · 2019-10-16T18:16:17Z

packages/protocol/contracts/identity/Attestations.sol

+   * @param account Address of the account
+   * @return [Block numbr at which was requested, Number of requests]
+   */
+  function getActiveAttestationRequest(


What does active mean in this context? Is an attestation request still active if it has been attestationExpiryBlocks since the request?

Active in this context means that it is a request for attestation for which issuers were not selected yet. Maybe a better word would be getAttestationRequest or getUnselectedAttestationRequest? I'll change it to the former for now.

asaj · 2019-10-16T18:17:07Z

packages/protocol/contracts/identity/Attestations.sol

    // solhint-disable-next-line not-rely-on-time
-    return now < attestationTime.add(attestationExpirySeconds);
+    return block.number < uint256(attestationRequestBlockTime).add(attestationExpiryBlocks);


Why are we using time and block numbers? Seems like we should pick one

It's blocks all throughout, somehow I thought using blockTime would indicate this denotes a time in block units, vs. a duration in block units, but see how this is more confusing. Changed it to attestationRequestBlock

asaj · 2019-10-18T16:52:11Z

packages/protocol/contracts/common/MockSafeCast.sol

+
+import "./SafeCast.sol";
+
+contract MockSafeCast {


I wouldn't call this a mock. SafeCastTest would be consistent with the rest of our naming

asaj · 2019-10-18T17:03:56Z

packages/protocol/contracts/identity/Attestations.sol

-    // The timestamp of the most recent attestation request
-    uint96 mostRecentAttestationRequest;
+    // The block number of the most recent attestation request
+    uint64 mostRecentAttestationRequest;


nit: mostRecentAttestationRequestBlockNumber

packages/protocol/contracts/identity/Attestations.sol

asaj · 2019-10-18T17:05:57Z

packages/protocol/contracts/identity/Attestations.sol

@@ -203,19 +221,31 @@ contract Attestations is

    if (accounts[msg.sender].attestationRequestFeeToken != address(0x0)) {
      require(
-        !isAttestationTimeValid(accounts[msg.sender].mostRecentAttestationRequest) ||
+        isAttestationExpired(accounts[msg.sender].mostRecentAttestationRequest) ||
          accounts[msg.sender].attestationRequestFeeToken == attestationRequestFeeToken,
        "A different fee token was previously specified for this account"


Consider There exists an unexpired attestation request with a different fee token

asaj · 2019-10-18T17:07:33Z

packages/protocol/contracts/identity/Attestations.sol

@@ -203,19 +221,31 @@ contract Attestations is

    if (accounts[msg.sender].attestationRequestFeeToken != address(0x0)) {
      require(
-        !isAttestationTimeValid(accounts[msg.sender].mostRecentAttestationRequest) ||
+        isAttestationExpired(accounts[msg.sender].mostRecentAttestationRequest) ||


Do we have a bug here? Imagine a low attestationExpiryBlocks that would allow us to pass this check and specify a different fee token, but then we increase it so the attestation request we originally thought was expired is now not expired?

I'm not sure what the technical definition of bug is but I would say in general we have some possibly unintended edge cases when changing parameters by governance. It's hard to anticipate and protect against that, in general and in this specific case I think?

packages/protocol/test/identity/attestations.ts

packages/contractkit/src/wrappers/Attestations.ts

packages/protocol/test/identity/attestations.ts

* Enforce wait for selectIssuers * Fix lint * PR comments

* master: (62 commits) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) [Wallet] Improve error handling around account creation and keystore ops (#1497) Add CI test for checking licenses and misc package.json cleanup (#1550) [Wallet] Implement SMS invite on iOS (#1541) CI: brings back to master (#1554) Validators: uses Ethereum address for proof of possession (#1494) Validate Attestation Requests (#1468) Rename hosted node references to forno (#1511) Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508) Added txpool family to geth apis. Sorted geth cmd options (#1462) [Wallet] Fix yarn dev command for running android (#1534) [Wallet] iOS info plist changes and version bump (#1539) ... # Conflicts: # yarn.lock

* master: (73 commits) Fix Ethstats Image reference (#1577) EU Cookies Behavior Change (#1447) [verifier] Upgrade to RN 61 (#1572) [Wallet] Update link styles and Implement VerificationEducationScreen (#1565) [wallet] Added native phone picker (#1310) [Wallet] Set up new verification screen skeletons (#1563) Bump e2e test migrate numbers where needed (#1567) [Wallet] Create new carousel component (#1555) [Wallet] Protect Backup Key and Safeguards with PIN (#1556) Increase ganache gas limit (#1569) Re-work locked gold requirements for validators and groups (#1474) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) ...

Add revealIssuers tx

7c9859d

nambrot requested review from asaj and m-chrzan as code owners October 14, 2019 16:47

nambrot added 2 commits October 14, 2019 10:13

Indention

301e776

Record request/completion time separately

932bd67

nambrot added 4 commits October 14, 2019 10:39

Contractkit changes

236fd8a

Merge branch 'master' into nambrot/attestation-commit

9982d56

Change reference to ActionalbeAttestations

f99bc97

Lint

bb87717

nambrot assigned asaj and m-chrzan Oct 14, 2019

asaj reviewed Oct 14, 2019

View reviewed changes

asaj assigned nambrot and unassigned asaj Oct 14, 2019

nambrot added 4 commits October 15, 2019 10:11

Change to blocks instead of Unix seconds

4b05a4b

Add SafeCast library

9d1b99d

PR comments

2573113

Move events below storage variables

c42d317

nambrot assigned asaj and unassigned nambrot Oct 15, 2019

nambrot added 2 commits October 15, 2019 11:42

Adjust CK

66729fd

lint

723f45c

asaj reviewed Oct 16, 2019

View reviewed changes

asaj assigned nambrot and unassigned asaj Oct 16, 2019

PR comments

110c0b2

nambrot assigned asaj and unassigned nambrot Oct 16, 2019

asaj reviewed Oct 18, 2019

View reviewed changes

asaj assigned nambrot and unassigned asaj Oct 29, 2019

nambrot mentioned this pull request Oct 30, 2019

Refactor to Accounts.sol #1392

Merged

6 tasks

nambrot added 4 commits October 31, 2019 09:55

[Add-on PR] Enforce wait for selectIssuers (#1503)

c9c5ef3

* Enforce wait for selectIssuers * Fix lint * PR comments

Fix election random access

2d99e4c

Merge branch 'master' into nambrot/attestation-commit

07ac718

Adjust e2e test and celotool cmd

ab3d0f8

nambrot added the automerge Have PR merge automatically when checks pass label Oct 31, 2019

nambrot added 3 commits October 31, 2019 14:27

Adjust CK

3ac9474

Merge branch 'master' into nambrot/attestation-commit

fa03266

Merge branch 'master' into nambrot/attestation-commit

7a3fbcb

celo-ci-bot-user merged commit 3c86fef into master Oct 31, 2019

celo-ci-bot-user deleted the nambrot/attestation-commit branch October 31, 2019 23:07

leojoy95 mentioned this pull request May 13, 2022

[Snyk] Fix for 5 vulnerabilities leojoy95/celo-monorepo#419

Open

snyk-bot mentioned this pull request May 30, 2022

[Snyk] Security upgrade yargs from 12.0.2 to 13.2.4 denissnykio/celo-monorepo#828

Open

snyk-bot mentioned this pull request Jul 27, 2022

[Snyk] Fix for 24 vulnerabilities denissnykio/celo-monorepo#836

Open

snyk-bot mentioned this pull request Sep 27, 2022

[Snyk] Fix for 27 vulnerabilities denissnykio/celo-monorepo#859

Open

This was referenced May 17, 2023

[Snyk] Fix for 13 vulnerabilities lili2311/lerna-celo-monorepo#9

Open

[Snyk] Fix for 13 vulnerabilities lili2311/lerna-celo-monorepo#10

Open

snyk-bot mentioned this pull request Jun 10, 2023

[Snyk] Fix for 13 vulnerabilities lili2311/lerna-celo-monorepo#39

Open

lili2311 mentioned this pull request Jul 20, 2023

[Snyk-dev] Fix for 14 vulnerabilities lili2311/lerna-celo-monorepo#71

Open

This was referenced Nov 26, 2023

[Snyk-dev] Fix for 21 vulnerabilities lili2311/lerna-celo-monorepo#156

Open

[Snyk-dev] Fix for 22 vulnerabilities lili2311/lerna-celo-monorepo#194

Open

lili2311 mentioned this pull request Dec 23, 2023

[Snyk-dev] Fix for 22 vulnerabilities lili2311/lerna-celo-monorepo#246

Open

lili2311 mentioned this pull request Feb 3, 2024

[Snyk-dev] Fix for 22 vulnerabilities lili2311/lerna-celo-monorepo#279

Open

lili2311 mentioned this pull request Mar 25, 2024

[Snyk-dev] Fix for 23 vulnerabilities lili2311/lerna-celo-monorepo#334

Open

lili2311 mentioned this pull request Apr 15, 2024

[Snyk-dev] Fix for 24 vulnerabilities lili2311/lerna-celo-monorepo#368

Open

lili2311 mentioned this pull request May 20, 2024

[Snyk-dev] Fix for 25 vulnerabilities lili2311/lerna-celo-monorepo#385

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add selectIssuers Transaction #1327

Add selectIssuers Transaction #1327

nambrot commented Oct 14, 2019 •

edited

Loading

codecov bot commented Oct 14, 2019 •

edited

Loading

asaj Oct 16, 2019

nambrot Oct 16, 2019

asaj Oct 16, 2019

nambrot Oct 16, 2019

asaj Oct 16, 2019

nambrot Oct 16, 2019

asaj Oct 16, 2019

nambrot Oct 16, 2019

asaj Oct 18, 2019

asaj Oct 18, 2019

asaj Oct 18, 2019

asaj Oct 18, 2019

nambrot Oct 18, 2019

Add selectIssuers Transaction #1327

Add selectIssuers Transaction #1327

Conversation

nambrot commented Oct 14, 2019 • edited Loading

Description

Tested

Other changes

Backwards compatibility

codecov bot commented Oct 14, 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

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 Oct 14, 2019 •

edited

Loading

codecov bot commented Oct 14, 2019 •

edited

Loading