diff --git a/packages/protocol/contracts/common/UsingRegistry.sol b/packages/protocol/contracts/common/UsingRegistry.sol index 15f00a61c82..c2b5109e6cf 100644 --- 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 index a35a77f5b94..e314c944021 100644 --- 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 index 8c42411d8d6..ef08b975afc 100644 --- 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 index 50d733499af..b9a3b605eb2 100644 --- 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)