PR fixes (comments, docstrings, remove FA from UsingRegistry) (#9660)

* PR fixes: comments, docstrings, remove FA from UsingRegistry * Use defaultTrustedIssuers directly instead of getter * Remove duplicate revocation test case * Remove stale TODOs * Add comments and spacing
celo-org · Jun 23, 2022 · 3bbdfb3 · 3bbdfb3
1 parent 6f749ec
commit 3bbdfb3
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 39 deletions.
diff --git a/packages/protocol/contracts/common/UsingRegistry.sol b/packages/protocol/contracts/common/UsingRegistry.sol
@@ -15,7 +15,6 @@ import "../governance/interfaces/IValidators.sol";
 
 import "../identity/interfaces/IRandom.sol";
 import "../identity/interfaces/IAttestations.sol";
-import "../identity/interfaces/IFederatedAttestations.sol";
 
 import "../stability/interfaces/IExchange.sol";
 import "../stability/interfaces/IReserve.sol";
@@ -37,9 +36,6 @@ contract UsingRegistry is Ownable {
   bytes32 constant FEE_CURRENCY_WHITELIST_REGISTRY_ID = keccak256(
     abi.encodePacked("FeeCurrencyWhitelist")
   );
-  bytes32 constant FEDERATED_ATTESTATIONS_REGISTRY_ID = keccak256(
-    abi.encodePacked("FederatedAttestations")
-  );
   bytes32 constant FREEZER_REGISTRY_ID = keccak256(abi.encodePacked("Freezer"));
   bytes32 constant GOLD_TOKEN_REGISTRY_ID = keccak256(abi.encodePacked("GoldToken"));
   bytes32 constant GOVERNANCE_REGISTRY_ID = keccak256(abi.encodePacked("Governance"));
@@ -96,10 +92,6 @@ contract UsingRegistry is Ownable {
     return IFeeCurrencyWhitelist(registry.getAddressForOrDie(FEE_CURRENCY_WHITELIST_REGISTRY_ID));
   }
 
-  function getFederatedAttestations() internal view returns (IFederatedAttestations) {
-    return IFederatedAttestations(registry.getAddressForOrDie(FEDERATED_ATTESTATIONS_REGISTRY_ID));
-  }
-
   function getFreezer() internal view returns (IFreezer) {
     return IFreezer(registry.getAddressForOrDie(FREEZER_REGISTRY_ID));
   }

diff --git a/packages/protocol/contracts/identity/Escrow.sol b/packages/protocol/contracts/identity/Escrow.sol
@@ -148,6 +148,8 @@ contract Escrow is
   /**
   * @notice Transfer tokens to a specific user. Supports both identity with privacy (an empty
   *         identifier and 0 minAttestations) and without (with identifier and minAttestations).
+  *         Sets trustedIssuers to the issuers listed in `defaultTrustedIssuers`.
+  *         (To override this and set custom trusted issuers, use `transferWithTrustedIssuers`.)
   * @param identifier The hashed identifier of a user to transfer to.
   * @param token The token to be transferred.
   * @param value The amount to be transferred.
@@ -160,8 +162,6 @@ contract Escrow is
   * @dev Throws if identifier is null and minAttestations > 0.
   * @dev If minAttestations is 0, trustedIssuers will be set to empty list.
   * @dev msg.sender needs to have already approved this contract to transfer
-
-
   */
   // solhint-disable-next-line no-simple-event-func-name
   function transfer(
@@ -175,7 +175,7 @@ contract Escrow is
     address[] memory trustedIssuers;
     // If minAttestations == 0, trustedIssuers should remain empty
     if (minAttestations > 0) {
-      trustedIssuers = getDefaultTrustedIssuers();
+      trustedIssuers = defaultTrustedIssuers;
     }
     return
       _transfer(
@@ -307,7 +307,6 @@ contract Escrow is
     EscrowedPayment memory payment = escrowedPayments[paymentId];
     require(payment.sender == msg.sender, "Only sender of payment can attempt to revoke payment.");
     require(
-      // solhint-disable-next-line not-rely-on-time
       now >= (payment.timestamp.add(payment.expirySeconds)),
       "Transaction not redeemable for sender yet."
     );
@@ -350,7 +349,7 @@ contract Escrow is
 
   /**
   * @notice Gets array of all trusted issuers set per paymentId.
-  * @param paymentId The ID of the payment to be deleted.
+  * @param paymentId The ID of the payment to get.
   * @return An array of addresses of trusted issuers set for an escrowed payment.
   */
   function getTrustedIssuersPerPayment(address paymentId) external view returns (address[] memory) {
@@ -501,8 +500,7 @@ contract Escrow is
     newPayment.value = value;
     newPayment.sentIndex = sentIndex;
     newPayment.receivedIndex = receivedIndex;
-    // solhint-disable-next-line not-rely-on-time
-    newPayment.timestamp = now;
+    newPayment.timestamp = block.timestamp;
     newPayment.expirySeconds = expirySeconds;
     newPayment.minAttestations = minAttestations;
 

diff --git a/packages/protocol/contracts/identity/FederatedAttestations.sol b/packages/protocol/contracts/identity/FederatedAttestations.sol
@@ -34,12 +34,16 @@ contract FederatedAttestations is
     // using uint64 to allow for extra space to add parameters
   }
 
-  // TODO ASv2 revisit linting issues & all solhint-disable-next-line max-line-length
+  // Mappings from identifier <-> attestation are separated by issuer,
+  // *requiring* users to specify issuers when retrieving attestations.
+  // Maintaining bidirectional mappings (vs. in Attestations.sol) makes it possible
+  // to perform lookups by identifier or account without indexing event data.
 
   // identifier -> issuer -> attestations
   mapping(bytes32 => mapping(address => OwnershipAttestation[])) public identifierToAttestations;
   // account -> issuer -> identifiers
   mapping(address => mapping(address => bytes32[])) public addressToIdentifiers;
+
   // unique attestation hash -> isRevoked
   mapping(bytes32 => bool) public revokedAttestations;
 
@@ -174,8 +178,6 @@ contract FederatedAttestations is
     bytes32[] calldata identifiers,
     address[] calldata accounts
   ) external {
-    // TODO ASv2 Reviewers: we are planning to provide sensible limits in the SDK
-    // to prevent out of gas errors -- is that sufficient or should we limit here as well?
     require(identifiers.length == accounts.length, "Unequal number of identifiers and accounts");
     require(
       issuer == msg.sender || getAccounts().attestationSignerToAccount(msg.sender) == issuer,
@@ -201,8 +203,6 @@ contract FederatedAttestations is
    * @dev Adds attestation info to the arrays in order of provided trustedIssuers
    * @dev Expectation that only one attestation exists per (identifier, issuer, account)
    */
-  // TODO reviewers: is it preferable to return an array of `trustedIssuer` indices
-  // (indicating issuer per attestation) instead of counts per attestation?
   function lookupAttestations(bytes32 identifier, address[] calldata trustedIssuers)
     external
     view

diff --git a/packages/protocol/test/identity/federatedattestations.ts b/packages/protocol/test/identity/federatedattestations.ts
@@ -694,25 +694,6 @@ contract('FederatedAttestations', (accounts: string[]) => {
       assert.isAtLeast(publishedOn.toNumber(), publishedOnLowerBound)
     })
 
-    it('should revert if the attestation is revoked', async () => {
-      await signAndRegisterAttestation(identifier1, issuer1, account1, nowUnixTime, signer1)
-      await federatedAttestations.revokeAttestation(identifier1, issuer1, account1, {
-        from: issuer1,
-      })
-      await assertRevert(
-        federatedAttestations.registerAttestation(
-          identifier1,
-          issuer1,
-          account1,
-          signer1,
-          nowUnixTime,
-          sig.v,
-          sig.r,
-          sig.s
-        )
-      )
-    })
-
     it('should succeed if issuer == signer', async () => {
       await signAndRegisterAttestation(identifier1, issuer1, account1, nowUnixTime, issuer1)
       await assertAttestationInStorage(identifier1, issuer1, 0, account1, nowUnixTime, issuer1, 0)