diff --git a/packages/celotool/src/cmds/account/verify.ts b/packages/celotool/src/cmds/account/verify.ts index 126a7742f02..1f94340b198 100644 --- a/packages/celotool/src/cmds/account/verify.ts +++ b/packages/celotool/src/cmds/account/verify.ts @@ -1,8 +1,11 @@ import { AccountArgv } from '@celo/celotool/src/cmds/account' import { portForwardAnd } from '@celo/celotool/src/lib/port_forward' import { newKit } from '@celo/contractkit' -import { AttestationsWrapper } from '@celo/contractkit/lib/wrappers/Attestations' -import { ActionableAttestation, decodeAttestationCode } from '@celo/walletkit' +import { + ActionableAttestation, + AttestationsWrapper, +} from '@celo/contractkit/lib/wrappers/Attestations' +import { base64ToHex } from '@celo/utils/lib/attestations' import prompts from 'prompts' import { switchToClusterFromEnv } from 'src/lib/cluster' import * as yargs from 'yargs' @@ -59,7 +62,8 @@ async function verifyCmd(argv: VerifyArgv) { await requestMoreAttestations( attestations, argv.phone, - argv.num - attestationsToComplete.length + argv.num - attestationsToComplete.length, + account ) } @@ -97,7 +101,8 @@ export async function printCurrentCompletedAttestations( async function requestMoreAttestations( attestations: AttestationsWrapper, phoneNumber: string, - attestationsRequested: number + attestationsRequested: number, + account: string ) { await attestations .approveAttestationFee(attestationsRequested) @@ -105,6 +110,8 @@ async function requestMoreAttestations( await attestations .request(phoneNumber, attestationsRequested) .then((txo) => txo.sendAndWaitForReceipt()) + await attestations.waitForSelectingIssuers(phoneNumber, account) + await attestations.selectIssuers(phoneNumber).then((txo) => txo.sendAndWaitForReceipt()) } async function revealAttestations( @@ -128,7 +135,7 @@ async function verifyCode( account: string, attestationsToComplete: ActionableAttestation[] ) { - const code = decodeAttestationCode(base64Code) + const code = base64ToHex(base64Code) const matchingIssuer = attestations.findMatchingIssuer( phoneNumber, account, diff --git a/packages/celotool/src/e2e-tests/attestations_tests.ts b/packages/celotool/src/e2e-tests/attestations_tests.ts index f9bcf5f07d9..b557f2e83cc 100644 --- a/packages/celotool/src/e2e-tests/attestations_tests.ts +++ b/packages/celotool/src/e2e-tests/attestations_tests.ts @@ -49,11 +49,16 @@ describe('governance tests', () => { it('requests an attestation', async function(this: any) { this.timeout(10000) + const approve = await Attestations.approveAttestationFee(2) await approve.sendAndWaitForReceipt() const request = await Attestations.request(phoneNumber, 2) await request.sendAndWaitForReceipt() + await Attestations.waitForSelectingIssuers(phoneNumber, validatorAddress) + const selectIssuers = await Attestations.selectIssuers(phoneNumber) + await selectIssuers.sendAndWaitForReceipt() + const stats = await Attestations.getAttestationStat(phoneNumber, validatorAddress) assert.equal(stats.total, 2) const actionable = await Attestations.getActionableAttestations(phoneNumber, validatorAddress) diff --git a/packages/contractkit/src/wrappers/Attestations.ts b/packages/contractkit/src/wrappers/Attestations.ts index 863a4892e0a..753c08f4590 100644 --- a/packages/contractkit/src/wrappers/Attestations.ts +++ b/packages/contractkit/src/wrappers/Attestations.ts @@ -1,4 +1,5 @@ import { ECIES, PhoneNumberUtils, SignatureUtils } from '@celo/utils' +import { sleep } from '@celo/utils/lib/async' import { zip3 } from '@celo/utils/lib/collections' import BigNumber from 'bignumber.js' import * as Web3Utils from 'web3-utils' @@ -30,7 +31,7 @@ export interface AttestationsToken { } export interface AttestationsConfig { - attestationExpirySeconds: number + attestationExpiryBlocks: number attestationRequestFees: AttestationsToken[] } @@ -46,13 +47,13 @@ export enum AttestationState { export interface ActionableAttestation { issuer: Address attestationState: AttestationState - time: number + blockNumber: number publicKey: string } const parseAttestationInfo = (rawState: { 0: string; 1: string }) => ({ attestationState: parseInt(rawState[0], 10), - time: parseInt(rawState[1], 10), + blockNumber: parseInt(rawState[1], 10), }) function attestationMessageToSign(phoneHash: string, account: Address) { @@ -68,8 +69,8 @@ export class AttestationsWrapper extends BaseWrapper { /** * Returns the time an attestation can be completable before it is considered expired */ - attestationExpirySeconds = proxyCall( - this.contract.methods.attestationExpirySeconds, + attestationExpiryBlocks = proxyCall( + this.contract.methods.attestationExpiryBlocks, undefined, toNumber ) @@ -85,6 +86,50 @@ export class AttestationsWrapper extends BaseWrapper { toBigNumber ) + selectIssuersWaitBlocks = proxyCall( + this.contract.methods.selectIssuersWaitBlocks, + undefined, + toNumber + ) + + /** + * @notice Returns the unselected attestation request for an identifier/account pair, if any. + * @param identifier Hash of the identifier. + * @param account Address of the account. + */ + getUnselectedRequest = proxyCall( + this.contract.methods.getUnselectedRequest, + tupleParser(PhoneNumberUtils.getPhoneHash, (x: string) => x), + (res) => ({ + blockNumber: toNumber(res[0]), + attestationsRequested: toNumber(res[1]), + attestationRequestFeeToken: res[2], + }) + ) + + waitForSelectingIssuers = async ( + phoneNumber: string, + account: Address, + timeoutSeconds = 120, + pollDurationSeconds = 1 + ) => { + const startTime = Date.now() + const unselectedRequest = await this.getUnselectedRequest(phoneNumber, account) + const waitBlocks = await this.selectIssuersWaitBlocks() + + if (unselectedRequest.blockNumber === 0) { + throw new Error('No unselectedRequest to wait for') + } + // Technically should use subscriptions here but not all providers support it. + // TODO: Use subscription if provider supports + while (Date.now() - startTime < timeoutSeconds * 1000) { + const blockNumber = await this.kit.web3.eth.getBlockNumber() + if (blockNumber >= unselectedRequest.blockNumber + waitBlocks) { + return + } + await sleep(pollDurationSeconds * 1000) + } + } /** * Returns the attestation stats of a phone number/account pair * @param phoneNumber Phone Number @@ -95,7 +140,7 @@ export class AttestationsWrapper extends BaseWrapper { account: Address ) => Promise = proxyCall( this.contract.methods.getAttestationStats, - tupleParser(PhoneNumberUtils.getPhoneHash, (x: string) => x), + tupleParser(PhoneNumberUtils.getPhoneHash, stringIdentity), (stat) => ({ completed: toNumber(stat[0]), total: toNumber(stat[1]) }) ) @@ -177,8 +222,8 @@ export class AttestationsWrapper extends BaseWrapper { account: Address ): Promise { const phoneHash = PhoneNumberUtils.getPhoneHash(phoneNumber) - const expirySeconds = await this.attestationExpirySeconds() - const nowInUnixSeconds = Math.floor(new Date().getTime() / 1000) + const expiryBlocks = await this.attestationExpiryBlocks() + const currentBlockNumber = await this.kit.web3.eth.getBlockNumber() const issuers = await this.contract.methods.getAttestationIssuers(phoneHash, account).call() const issuerState = Promise.all( @@ -196,14 +241,14 @@ export class AttestationsWrapper extends BaseWrapper { ) const isIncomplete = (status: AttestationState) => status === AttestationState.Incomplete - const hasNotExpired = (time: number) => nowInUnixSeconds < time + expirySeconds + const hasNotExpired = (blockNumber: number) => currentBlockNumber < blockNumber + expiryBlocks const isValidKey = (key: string) => key !== null && key !== '0x0' return zip3(issuers, await issuerState, await publicKeys) .filter( ([_issuer, attestation, publicKey]) => isIncomplete(attestation.attestationState) && - hasNotExpired(attestation.time) && + hasNotExpired(attestation.blockNumber) && isValidKey(publicKey) ) .map(([issuer, attestation, publicKey]) => ({ @@ -266,7 +311,7 @@ export class AttestationsWrapper extends BaseWrapper { }) ) return { - attestationExpirySeconds: await this.attestationExpirySeconds(), + attestationExpiryBlocks: await this.attestationExpiryBlocks(), attestationRequestFees: fees, } } @@ -328,6 +373,16 @@ export class AttestationsWrapper extends BaseWrapper { ) } + /** + * Selects the issuers for previously requested attestations for a phone number + * @param phoneNumber The phone number for which to request attestations for + * @param token The token with which to pay for the attestation fee + */ + async selectIssuers(phoneNumber: string) { + const phoneHash = PhoneNumberUtils.getPhoneHash(phoneNumber) + return toTransactionObject(this.kit, this.contract.methods.selectIssuers(phoneHash)) + } + /** * Reveals the phone number to the issuer of the attestation on-chain * @param phoneNumber The phone number which requested attestation diff --git a/packages/protocol/contracts/common/SafeCast.sol b/packages/protocol/contracts/common/SafeCast.sol new file mode 100644 index 00000000000..498c77b68f6 --- /dev/null +++ b/packages/protocol/contracts/common/SafeCast.sol @@ -0,0 +1,88 @@ +pragma solidity ^0.5.0; +// TODO: Remove this and use upstream when +// https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1926/files gets merged + +/** + * @dev Wrappers over Solidity's uintXX casting operators with added overflow + * checks. + * + * Downcasting from uint256 in Solidity does not revert by default on overflow. + * This can easily result in undesired exploitation or bugs, since developers + * usually assume that overflows raise errors. `SafeCast` restores this intuition + * by reverting the transaction when such an operation overflows. + * + * Using this library instead of the unchecked operations eliminates an entire + * class of bugs, so it's recommended to use it always. + */ +library SafeCast { + + /** + * @dev Returns the downcasted uint128 from uint256, reverting on + * overflow (when the input is greater than largest uint128). + * + * Counterpart to Solidity's `uint128` operator. + * + * Requirements: + * - input must fit into 128 bits + */ + function toUint128(uint256 value) internal pure returns (uint128) { + require(value < 2**128, "SafeCast: value doesn\'t fit in 128 bits"); + return uint128(value); + } + + /** + * @dev Returns the downcasted uint64 from uint256, reverting on + * overflow (when the input is greater than largest uint64). + * + * Counterpart to Solidity's `uint64` operator. + * + * Requirements: + * - input must fit into 64 bits + */ + function toUint64(uint256 value) internal pure returns (uint64) { + require(value < 2**64, "SafeCast: value doesn\'t fit in 64 bits"); + return uint64(value); + } + + /** + * @dev Returns the downcasted uint32 from uint256, reverting on + * overflow (when the input is greater than largest uint32). + * + * Counterpart to Solidity's `uint32` operator. + * + * Requirements: + * - input must fit into 32 bits + */ + function toUint32(uint256 value) internal pure returns (uint32) { + require(value < 2**32, "SafeCast: value doesn\'t fit in 32 bits"); + return uint32(value); + } + + /** + * @dev Returns the downcasted uint16 from uint256, reverting on + * overflow (when the input is greater than largest uint16). + * + * Counterpart to Solidity's `uint16` operator. + * + * Requirements: + * - input must fit into 16 bits + */ + function toUint16(uint256 value) internal pure returns (uint16) { + require(value < 2**16, "SafeCast: value doesn\'t fit in 16 bits"); + return uint16(value); + } + + /** + * @dev Returns the downcasted uint8 from uint256, reverting on + * overflow (when the input is greater than largest uint8). + * + * Counterpart to Solidity's `uint8` operator. + * + * Requirements: + * - input must fit into 8 bits. + */ + function toUint8(uint256 value) internal pure returns (uint8) { + require(value < 2**8, "SafeCast: value doesn\'t fit in 8 bits"); + return uint8(value); + } +} \ No newline at end of file diff --git a/packages/protocol/contracts/common/SafeCastTest.sol b/packages/protocol/contracts/common/SafeCastTest.sol new file mode 100644 index 00000000000..af7261c2e7a --- /dev/null +++ b/packages/protocol/contracts/common/SafeCastTest.sol @@ -0,0 +1,30 @@ + +pragma solidity ^0.5.0; +// TODO: Remove this and use upstream when +// https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1926/files gets merged + +import "./SafeCast.sol"; + +contract SafeCastTest { + using SafeCast for uint; + + function toUint128(uint a) public pure returns (uint128) { + return a.toUint128(); + } + + function toUint64(uint a) public pure returns (uint64) { + return a.toUint64(); + } + + function toUint32(uint a) public pure returns (uint32) { + return a.toUint32(); + } + + function toUint16(uint a) public pure returns (uint16) { + return a.toUint16(); + } + + function toUint8(uint a) public pure returns (uint8) { + return a.toUint8(); + } +} \ No newline at end of file diff --git a/packages/protocol/contracts/governance/Election.sol b/packages/protocol/contracts/governance/Election.sol index e810dd07329..9d036065110 100644 --- a/packages/protocol/contracts/governance/Election.sol +++ b/packages/protocol/contracts/governance/Election.sol @@ -909,7 +909,7 @@ contract Election is * @return The permuted array. */ function shuffleArray(address[] memory array) private view returns (address[] memory) { - bytes32 r = getRandom().random(); + bytes32 r = getRandom().getBlockRandomness(block.number); for (uint256 i = array.length - 1; i > 0; i = i.sub(1)) { uint256 j = uint256(r) % (i + 1); (array[i], array[j]) = (array[j], array[i]); diff --git a/packages/protocol/contracts/identity/Attestations.sol b/packages/protocol/contracts/identity/Attestations.sol index f7654e3744b..4b25a774821 100644 --- a/packages/protocol/contracts/identity/Attestations.sol +++ b/packages/protocol/contracts/identity/Attestations.sol @@ -12,6 +12,7 @@ import "../governance/interfaces/IValidators.sol"; import "../common/Initializable.sol"; import "../common/UsingRegistry.sol"; import "../common/Signatures.sol"; +import "../common/SafeCast.sol"; import "../common/UsingPrecompiles.sol"; @@ -28,56 +29,7 @@ contract Attestations is { using SafeMath for uint256; - using SafeMath for uint128; - using SafeMath for uint96; - - event AttestationsRequested( - bytes32 indexed identifier, - address indexed account, - uint256 attestationsRequested, - address attestationRequestFeeToken - ); - - event AttestationCompleted( - bytes32 indexed identifier, - address indexed account, - address indexed issuer - ); - - event Withdrawal( - address indexed account, - address indexed token, - uint256 amount - ); - - event AttestationExpirySecondsSet( - uint256 value - ); - - event AttestationRequestFeeSet( - address indexed token, - uint256 value - ); - - event AttestorAuthorized( - address indexed account, - address attestor - ); - - event AccountDataEncryptionKeySet( - address indexed account, - bytes dataEncryptionKey - ); - - event AccountMetadataURLSet( - address indexed account, - string metadataURL - ); - - event AccountWalletAddressSet( - address indexed account, - address walletAddress - ); + using SafeCast for uint256; enum AttestationStatus { None, @@ -88,45 +40,60 @@ contract Attestations is struct Attestation { AttestationStatus status; - // For outstanding attestations, this is the timestamp of the request - // For completed attestations, this is the timestamp of the attestation completion - uint128 time; + // For outstanding attestations, this is the block number of the request. + // For completed attestations, this is the block number of the attestation completion. + uint32 blockNumber; + + // The token with which attestation request fees were paid. + address attestationRequestFeeToken; } struct Account { - // The timestamp of the most recent attestation request - uint96 mostRecentAttestationRequest; - - // The address at which the account expects to receive transfers + // The address at which the account expects to receive transfers. address walletAddress; - // The token with which attestation request fees are paid - address attestationRequestFeeToken; - - // The address of the key with which this account wants to sign attestations + // The address of the key with which this account wants to sign attestations. address authorizedAttestor; - // The ECDSA public key used to encrypt and decrypt data for this account + // The ECDSA public key used to encrypt and decrypt data for this account. bytes dataEncryptionKey; - // The URL under which an account adds metadata and claims + // The URL under which an account adds metadata and claims. string metadataURL; } // Stores attestations state for a single (identifier, account address) pair. - struct AttestationsMapping { - // Number of completed attestations - uint64 completed; - // List of issuers responsible for attestations - address[] issuers; - // State of each attestation keyed by issuer + struct AttestedAddress { + // Total number of requested attestations. + uint32 requested; + // Total number of completed attestations. + uint32 completed; + // List of selected issuers responsible for attestations. The length of this list + // might be smaller than `requested` (which represents the total number of requested + // attestations) if users are not calling `selectIssuers` on unselected requests. + address[] selectedIssuers; + // State of each attestation keyed by issuer. mapping(address => Attestation) issuedAttestations; } + struct UnselectedRequest { + // The block at which the attestations were requested. + uint32 blockNumber; + + // The number of attestations that were requested. + uint32 attestationsRequested; + + // The token with which attestation request fees were paid in this request. + address attestationRequestFeeToken; + } + struct IdentifierState { - // All account addresses associated with this identifier + // All account addresses associated with this identifier. address[] accounts; - mapping(address => AttestationsMapping) attestations; + // Keeps the state of attestations for account addresses for this identifier. + mapping(address => AttestedAddress) attestations; + // Temporarily stores attestation requests for which issuers should be selected by the account. + mapping(address => UnselectedRequest) unselectedRequests; } mapping(bytes32 => IdentifierState) identifiers; @@ -139,8 +106,12 @@ contract Attestations is // solhint-disable-next-line state-visibility address constant REQUEST_ATTESTATION = address(0xff); - // The duration in seconds in which an attestation can be completed - uint256 public attestationExpirySeconds; + // The duration in blocks in which an attestation can be completed from the block in which the + // attestation was requested. + uint256 public attestationExpiryBlocks; + + // The duration to wait until selectIssuers can be called for an attestation request. + uint256 public selectIssuersWaitBlocks; // The fees that are associated with attestations for a particular token. mapping(address => uint256) public attestationRequestFees; @@ -148,9 +119,39 @@ contract Attestations is // Maps a token and attestation issuer to the amount that they're owed. mapping(address => mapping(address => uint256)) public pendingWithdrawals; + event AttestationsRequested( + bytes32 indexed identifier, + address indexed account, + uint256 attestationsRequested, + address attestationRequestFeeToken + ); + + event AttestationIssuersSelected( + bytes32 indexed identifier, + address indexed account, + uint256 attestationsRequested, + address attestationRequestFeeToken + ); + + event AttestationCompleted( + bytes32 indexed identifier, + address indexed account, + address indexed issuer + ); + + event Withdrawal( address indexed account, address indexed token, uint256 amount); + event AttestationExpiryBlocksSet(uint256 value); + event AttestationRequestFeeSet(address indexed token,uint256 value); + event AttestorAuthorized(address indexed account, address attestor); + event AccountDataEncryptionKeySet(address indexed account, bytes dataEncryptionKey); + event AccountMetadataURLSet(address indexed account, string metadataURL); + event AccountWalletAddressSet(address indexed account, address walletAddress); + event SelectIssuersWaitBlocksSet(uint256 value); + function initialize( address registryAddress, - uint256 _attestationExpirySeconds, + uint256 _attestationExpiryBlocks, + uint256 _selectIssuersWaitBlocks, address[] calldata attestationRequestFeeTokens, uint256[] calldata attestationRequestFeeValues ) @@ -159,7 +160,9 @@ contract Attestations is { _transferOwnership(msg.sender); setRegistry(registryAddress); - setAttestationExpirySeconds(_attestationExpirySeconds); + setAttestationExpiryBlocks(_attestationExpiryBlocks); + setSelectIssuersWaitBlocks(_selectIssuersWaitBlocks); + require( attestationRequestFeeTokens.length > 0 && attestationRequestFeeTokens.length == attestationRequestFeeValues.length, @@ -173,7 +176,7 @@ contract Attestations is /** * @notice Commit to the attestation request of a hashed identifier. * @param identifier The hash of the identifier to be attested. - * @param attestationsRequested The number of requested attestations for this request + * @param attestationsRequested The number of requested attestations for this request. * @param attestationRequestFeeToken The address of the token with which the attestation fee will * be paid. */ @@ -200,21 +203,21 @@ contract Attestations is require(attestationsRequested > 0, "You have to request at least 1 attestation"); - if (accounts[msg.sender].attestationRequestFeeToken != address(0x0)) { - require( - !isAttestationTimeValid(accounts[msg.sender].mostRecentAttestationRequest) || - accounts[msg.sender].attestationRequestFeeToken == attestationRequestFeeToken, - "A different fee token was previously specified for this account" - ); - } + IdentifierState storage state = identifiers[identifier]; - // solhint-disable-next-line not-rely-on-time - accounts[msg.sender].mostRecentAttestationRequest = uint96(now); - accounts[msg.sender].attestationRequestFeeToken = attestationRequestFeeToken; + require( + state.unselectedRequests[msg.sender].blockNumber == 0 || + isAttestationExpired(state.unselectedRequests[msg.sender].blockNumber), + "There exists an unexpired, unselected attestation request" + ); - IdentifierState storage state = identifiers[identifier]; + state.unselectedRequests[msg.sender].blockNumber = block.number.toUint32(); + state.unselectedRequests[msg.sender].attestationsRequested = attestationsRequested.toUint32(); + state.unselectedRequests[msg.sender].attestationRequestFeeToken = attestationRequestFeeToken; - addIncompleteAttestations(attestationsRequested, state.attestations[msg.sender]); + state.attestations[msg.sender].requested = uint256( + state.attestations[msg.sender].requested + ).add(attestationsRequested).toUint32(); emit AttestationsRequested( identifier, @@ -224,11 +227,39 @@ contract Attestations is ); } + /** + * @notice Selects the issuers for the most recent attestation request. + * @param identifier The hash of the identifier to be attested. + */ + function selectIssuers(bytes32 identifier) external { + IdentifierState storage state = identifiers[identifier]; + + require( + state.unselectedRequests[msg.sender].blockNumber > 0, + "No unselected attestation request to select issuers for" + ); + + require( + !isAttestationExpired(state.unselectedRequests[msg.sender].blockNumber), + "The attestation request has expired" + ); + + addIncompleteAttestations(identifier); + emit AttestationIssuersSelected( + identifier, + msg.sender, + state.unselectedRequests[msg.sender].attestationsRequested, + state.unselectedRequests[msg.sender].attestationRequestFeeToken + ); + + delete state.unselectedRequests[msg.sender]; + } + /** * @notice Reveal the encrypted phone number to the issuer. * @param identifier The hash of the identifier to be attested. * @param encryptedPhone The number ECIES encrypted with the issuer's public key. - * @param issuer The issuer of the attestation + * @param issuer The issuer of the attestation. * @param sendSms Whether or not to send an SMS. For testing purposes. */ function reveal( @@ -242,13 +273,10 @@ contract Attestations is Attestation storage attestation = identifiers[identifier].attestations[msg.sender].issuedAttestations[issuer]; - require( - attestation.status == AttestationStatus.Incomplete, - "Attestation is not incomplete" - ); + require(attestation.status == AttestationStatus.Incomplete, "Attestation is not incomplete"); // solhint-disable-next-line not-rely-on-time - require(isAttestationTimeValid(attestation.time), "Attestation request timed out"); + require(!isAttestationExpired(attestation.blockNumber), "Attestation timed out"); // Generate the yet-to-be-signed attestation code that will be signed and sent to the // encrypted phone number via SMS via the 'RequestAttestation' precompiled contract. @@ -282,13 +310,14 @@ contract Attestations is Attestation storage attestation = identifiers[identifier].attestations[msg.sender].issuedAttestations[issuer]; + address token = attestation.attestationRequestFeeToken; + // solhint-disable-next-line not-rely-on-time - attestation.time = uint128(now); + attestation.blockNumber = block.number.toUint32(); attestation.status = AttestationStatus.Complete; + delete attestation.attestationRequestFeeToken; identifiers[identifier].attestations[msg.sender].completed++; - address token = accounts[msg.sender].attestationRequestFeeToken; - pendingWithdrawals[token][issuer] = pendingWithdrawals[token][issuer].add( attestationRequestFees[token] ); @@ -302,9 +331,9 @@ contract Attestations is } /** - * @notice Revokes an account for an identifier - * @param identifier The identifier for which to revoke - * @param index The index of the account in the accounts array + * @notice Revokes an account for an identifier. + * @param identifier The identifier for which to revoke. + * @param index The index of the account in the accounts array. */ function revoke(bytes32 identifier, uint256 index) external { uint256 numAccounts = identifiers[identifier].accounts.length; @@ -336,9 +365,9 @@ contract Attestations is } /** - * @notice Setter for the dataEncryptionKey and wallet address for an account + * @notice Setter for the dataEncryptionKey and wallet address for an account. * @param dataEncryptionKey secp256k1 public key for data encryption. Preferably compressed. - * @param walletAddress The wallet address to set for the account + * @param walletAddress The wallet address to set for the account. */ function setAccount( bytes calldata dataEncryptionKey, @@ -351,10 +380,35 @@ contract Attestations is } /** - * @notice Returns attestation issuers for a identifier/account pair + * @notice Returns the unselected attestation request for an identifier/account pair, if any. + * @param identifier Hash of the identifier. + * @param account Address of the account. + * @return [ + * Block number at which was requested, + * Number of unselected requests, + * Address of the token with which this attestation request was paid for + * ] + */ + function getUnselectedRequest( + bytes32 identifier, + address account + ) + external + view + returns (uint32, uint32, address) + { + return ( + identifiers[identifier].unselectedRequests[account].blockNumber, + identifiers[identifier].unselectedRequests[account].attestationsRequested, + identifiers[identifier].unselectedRequests[account].attestationRequestFeeToken + ); + } + + /** + * @notice Returns selected attestation issuers for a identifier/account pair. * @param identifier Hash of the identifier. - * @param account Address of the account - * @return Addresses of the attestation issuers + * @param account Address of the account. + * @return Addresses of the selected attestation issuers. */ function getAttestationIssuers( bytes32 identifier, @@ -364,13 +418,13 @@ contract Attestations is view returns (address[] memory) { - return identifiers[identifier].attestations[account].issuers; + return identifiers[identifier].attestations[account].selectedIssuers; } /** - * @notice Returns attestation stats for a identifier/account pair + * @notice Returns attestation stats for a identifier/account pair. * @param identifier Hash of the identifier. - * @param account Address of the account + * @param account Address of the account. * @return [Number of completed attestations, Number of total requested attestations] */ function getAttestationStats( @@ -379,22 +433,22 @@ contract Attestations is ) external view - returns (uint64, uint64) + returns (uint32, uint32) { return ( identifiers[identifier].attestations[account].completed, - uint64(identifiers[identifier].attestations[account].issuers.length) + identifiers[identifier].attestations[account].requested ); } /** - * @notice Batch lookup function to determine attestation stats for a list of identifiers - * @param identifiersToLookup Array of n identifiers - * @return [0] Array of number of matching accounts per identifier - * @return [1] Array of sum([0]) matching walletAddresses - * @return [2] Array of sum([0]) numbers indicating the completions for each account + * @notice Batch lookup function to determine attestation stats for a list of identifiers. + * @param identifiersToLookup Array of n identifiers. + * @return [0] Array of number of matching accounts per identifier. + * @return [1] Array of sum([0]) matching walletAddresses. + * @return [2] Array of sum([0]) numbers indicating the completions for each account. * @return [3] Array of sum([0]) numbers indicating the total number of requested - attestations for each account + attestations for each account. */ function batchGetAttestationStats( bytes32[] calldata identifiersToLookup @@ -422,9 +476,9 @@ contract Attestations is addresses[currentIndex] = accounts[addrs[matchIndex]].walletAddress; completed[currentIndex] = identifiers[identifiersToLookup[i]].attestations[addrs[matchIndex]].completed; - total[currentIndex] = uint64( - identifiers[identifiersToLookup[i]].attestations[addrs[matchIndex]].issuers.length - ); + total[currentIndex] = + identifiers[identifiersToLookup[i]].attestations[addrs[matchIndex]].requested; + currentIndex++; } } @@ -433,11 +487,15 @@ contract Attestations is } /** - * @notice Returns the state of a specific attestation + * @notice Returns the state of a specific attestation. * @param identifier Hash of the identifier. - * @param account Address of the account - * @param issuer Address of the issuer - * @return [Status of the attestation, time of the attestation] + * @param account Address of the account. + * @param issuer Address of the issuer. + * @return [ + * Status of the attestation, + * Block number of request/completion the attestation, + * Address of the token with which this attestation request was paid for + * ] */ function getAttestationState( bytes32 identifier, @@ -446,37 +504,18 @@ contract Attestations is ) external view - returns (uint8, uint128) + returns (uint8, uint32, address) { + Attestation storage attestation = + identifiers[identifier].attestations[account].issuedAttestations[issuer]; return ( - uint8(identifiers[identifier].attestations[account].issuedAttestations[issuer].status), - identifiers[identifier].attestations[account].issuedAttestations[issuer].time + uint8(attestation.status), + attestation.blockNumber, + attestation.attestationRequestFeeToken ); } - /** - * @notice Returns address of the token in which the account chose to pay attestation fees - * @param account Address of the account - * @return Address of the token contract - */ - function getAttestationRequestFeeToken(address account) external view returns (address) { - return accounts[account].attestationRequestFeeToken; - } - - /** - * @notice Returns timestamp of the most recent attestation request - * @param account Address of the account - * @return Timestamp of the most recent attestation request - */ - function getMostRecentAttestationRequest(address account) - external - view - returns (uint256) - { - return accounts[account].mostRecentAttestationRequest; - } - /** * @notice Returns the fee set for a particular token. * @param token Address of the attestationRequestFeeToken. @@ -498,14 +537,25 @@ contract Attestations is } /** - * @notice Updates 'attestationExpirySeconds'. - * @param _attestationExpirySeconds The new limit on blocks allowed to come between requesting + * @notice Updates 'attestationExpiryBlocks'. + * @param _attestationExpiryBlocks The new limit on blocks allowed to come between requesting * an attestation and completing it. */ - function setAttestationExpirySeconds(uint256 _attestationExpirySeconds) public onlyOwner { - require(_attestationExpirySeconds > 0, "attestationExpirySeconds has to be greater than 0"); - attestationExpirySeconds = _attestationExpirySeconds; - emit AttestationExpirySecondsSet(_attestationExpirySeconds); + function setAttestationExpiryBlocks(uint256 _attestationExpiryBlocks) public onlyOwner { + require(_attestationExpiryBlocks > 0, "attestationExpiryBlocks has to be greater than 0"); + attestationExpiryBlocks = _attestationExpiryBlocks; + emit AttestationExpiryBlocksSet(_attestationExpiryBlocks); + } + + /** + * @notice Updates 'selectIssuersWaitBlocks'. + * @param _selectIssuersWaitBlocks The wait period in blocks to call selectIssuers on attestation + * requests. + */ + function setSelectIssuersWaitBlocks(uint256 _selectIssuersWaitBlocks) public onlyOwner { + require(_selectIssuersWaitBlocks > 0, "selectIssuersWaitBlocks has to be greater than 0"); + selectIssuersWaitBlocks = _selectIssuersWaitBlocks; + emit SelectIssuersWaitBlocksSet(_selectIssuersWaitBlocks); } /** @@ -538,7 +588,7 @@ contract Attestations is /** * @notice Getter for the data encryption key and version. - * @param account The address of the account to get the key for + * @param account The address of the account to get the key for. * @return dataEncryptionKey secp256k1 public key for data encryption. Preferably compressed. */ function getDataEncryptionKey(address account) external view returns (bytes memory) { @@ -592,8 +642,8 @@ contract Attestations is } /** - * @notice Authorizes an address to attest on behalf - * @param attestor The address of the attestor to set for the account + * @notice Authorizes an address to attest on behalf. + * @param attestor The address of the attestor to set for the account. * @param v The recovery id of the incoming ECDSA signature. * @param r Output value r of the ECDSA signature. * @param s Output value s of the ECDSA signature. @@ -640,8 +690,8 @@ contract Attestations is } /** - * @notice Setter for the wallet address for an account - * @param walletAddress The wallet address to set for the account + * @notice Setter for the wallet address for an account. + * @param walletAddress The wallet address to set for the account. */ function setWalletAddress(address walletAddress) public { accounts[msg.sender].walletAddress = walletAddress; @@ -649,9 +699,9 @@ contract Attestations is } /** - * @notice Getter for the wallet address for an account - * @param account The address of the account to get the wallet address for - * @return Wallet address + * @notice Getter for the wallet address for an account. + * @param account The address of the account to get the wallet address for. + * @return Wallet address. */ function getWalletAddress(address account) external view returns (address) { return accounts[account].walletAddress; @@ -689,8 +739,7 @@ contract Attestations is attestation.status == AttestationStatus.Incomplete, "Attestation code does not match any outstanding attestation" ); - // solhint-disable-next-line not-rely-on-time - require(isAttestationTimeValid(attestation.time), "Attestation request timed out"); + require(!isAttestationExpired(attestation.blockNumber), "Attestation timed out"); return issuer; } @@ -707,10 +756,10 @@ contract Attestations is /** * @notice Helper function for batchGetAttestationStats to calculate the - total number of addresses that have >0 complete attestations for the identifiers - * @param identifiersToLookup Array of n identifiers + total number of addresses that have >0 complete attestations for the identifiers. + * @param identifiersToLookup Array of n identifiers. * @return Array of n numbers that indicate the number of matching addresses per identifier - * and array of addresses preallocated for total number of matches + * and array of addresses preallocated for total number of matches. */ function batchlookupAccountsForIdentifier( bytes32[] memory identifiersToLookup @@ -734,48 +783,52 @@ contract Attestations is } /** - * @notice Adds additional attestations given the current randomness - * @param n Number of attestations to add - * @param state The accountState of the address to add attestations for + * @notice Adds additional attestations given the current randomness. + * @param identifier The hash of the identifier to be attested. */ function addIncompleteAttestations( - uint256 n, - AttestationsMapping storage state + bytes32 identifier ) internal { - IRandom random = IRandom(registry.getAddressForOrDie(RANDOM_REGISTRY_ID)); + AttestedAddress storage state = identifiers[identifier].attestations[msg.sender]; + UnselectedRequest storage unselectedRequest = + identifiers[identifier].unselectedRequests[msg.sender]; - bytes32 seed = random.random(); + bytes32 seed = getRandom().getBlockRandomness( + unselectedRequest.blockNumber + selectIssuersWaitBlocks + ); uint256 numberValidators = numberValidatorsInCurrentSet(); uint256 currentIndex = 0; address validator; address issuer; - while (currentIndex < n) { + while (currentIndex < unselectedRequest.attestationsRequested) { seed = keccak256(abi.encodePacked(seed)); validator = validatorAddressFromCurrentSet(uint256(seed) % numberValidators); issuer = getLockedGold().getAccountFromValidator(validator); - Attestation storage attestations = - state.issuedAttestations[issuer]; + Attestation storage attestation = state.issuedAttestations[issuer]; - // Attestation issuers can only be added if they haven't already - if (attestations.status != AttestationStatus.None) { + // Attestation issuers can only be added if they haven't been already. + if (attestation.status != AttestationStatus.None) { continue; } currentIndex++; - attestations.status = AttestationStatus.Incomplete; - // solhint-disable-next-line not-rely-on-time - attestations.time = uint128(now); - state.issuers.push(issuer); + attestation.status = AttestationStatus.Incomplete; + attestation.blockNumber = unselectedRequest.blockNumber; + attestation.attestationRequestFeeToken = unselectedRequest.attestationRequestFeeToken; + state.selectedIssuers.push(issuer); } } - function isAttestationTimeValid(uint128 attestationTime) internal view returns (bool) { - // solhint-disable-next-line not-rely-on-time - return now < attestationTime.add(attestationExpirySeconds); + function isAttestationExpired(uint128 attestationRequestBlock) + internal + view + returns (bool) + { + return block.number >= uint256(attestationRequestBlock).add(attestationExpiryBlocks); } } diff --git a/packages/protocol/contracts/identity/interfaces/IAttestations.sol b/packages/protocol/contracts/identity/interfaces/IAttestations.sol index c0d104e57c0..eddf2da808f 100644 --- a/packages/protocol/contracts/identity/interfaces/IAttestations.sol +++ b/packages/protocol/contracts/identity/interfaces/IAttestations.sol @@ -3,16 +3,17 @@ pragma solidity ^0.5.3; interface IAttestations { - function initialize(address, uint256, address[] calldata, uint256[] calldata) external; + function initialize(address, uint256, uint256, address[] calldata, uint256[] calldata) external; function setAttestationRequestFee(address, uint256) external; function request(bytes32, uint256, address) external; + function selectIssuers(bytes32) external; function reveal(bytes32, bytes calldata, address, bool) external; function complete(bytes32, uint8, bytes32, bytes32) external; function revoke(bytes32, uint256) external; function withdraw(address) external; - function setAttestationExpirySeconds(uint256) external; + function setAttestationExpiryBlocks(uint256) external; function setAccountDataEncryptionKey(bytes calldata) external; function authorizeAttestor(address, uint8, bytes32, bytes32) external; @@ -27,6 +28,7 @@ interface IAttestations { function getWalletAddress(address) external view returns (address); function getMetadataURL(address) external view returns (string memory); + function getUnselectedRequest(bytes32, address) external view returns (uint32, uint32, address); function getAttestationRequestFee(address) external view returns (uint256); function lookupAccountsForIdentifier(bytes32) external view returns (address[] memory); @@ -37,7 +39,7 @@ interface IAttestations { ) external view - returns (uint64, uint64); + returns (uint32, uint32); function getAttestationState( bytes32, @@ -46,8 +48,6 @@ interface IAttestations { ) external view - returns (uint8, uint128); + returns (uint8, uint32, address); - function getAttestationRequestFeeToken(address) external view returns (address); - function getMostRecentAttestationRequest(address) external view returns (uint256); } diff --git a/packages/protocol/contracts/identity/interfaces/IRandom.sol b/packages/protocol/contracts/identity/interfaces/IRandom.sol index 3d494fca1be..e63450d313f 100644 --- a/packages/protocol/contracts/identity/interfaces/IRandom.sol +++ b/packages/protocol/contracts/identity/interfaces/IRandom.sol @@ -5,4 +5,5 @@ interface IRandom { function revealAndCommit(bytes32, bytes32, address) external; function random() external view returns (bytes32); + function getBlockRandomness(uint256) external view returns (bytes32); } diff --git a/packages/protocol/contracts/identity/test/MockRandom.sol b/packages/protocol/contracts/identity/test/MockRandom.sol index 6953fdb56bf..7130f4e4525 100644 --- a/packages/protocol/contracts/identity/test/MockRandom.sol +++ b/packages/protocol/contracts/identity/test/MockRandom.sol @@ -1,13 +1,15 @@ pragma solidity ^0.5.3; -import "openzeppelin-solidity/contracts/math/SafeMath.sol"; +import "../Random.sol"; +contract MockRandom is Random { + mapping (uint256 => bytes32) private history; -contract MockRandom { - - bytes32 public random; - - function setRandom(bytes32 value) external { - random = value; + function addTestRandomness(uint256 blockNumber, bytes32 randomness) external { + history[blockNumber] = randomness; + } + function getBlockRandomness(uint256 blockNumber) external view returns (bytes32) { + require(history[blockNumber] != 0x0, "No randomness found"); + return history[blockNumber]; } -} +} \ No newline at end of file diff --git a/packages/protocol/migrations/14_attestations.ts b/packages/protocol/migrations/14_attestations.ts index a41b51070f6..715c98e3939 100644 --- a/packages/protocol/migrations/14_attestations.ts +++ b/packages/protocol/migrations/14_attestations.ts @@ -10,7 +10,7 @@ import { import { config } from '@celo/protocol/migrationsConfig' import { AttestationsInstance, StableTokenInstance } from 'types' import { TransactionObject } from 'web3/eth/types' -const initializeArgs = async (): Promise<[string, string, string[], string[]]> => { +const initializeArgs = async (): Promise<[string, string, string, string[], string[]]> => { const stableToken: StableTokenInstance = await getDeployedProxiedContract( 'StableToken', artifacts @@ -22,7 +22,8 @@ const initializeArgs = async (): Promise<[string, string, string[], string[]]> = ) return [ config.registry.predeployedProxyAddress, - config.attestations.attestationExpirySeconds.toString(), + config.attestations.attestationExpiryBlocks.toString(), + config.attestations.selectIssuersWaitBlocks.toString(), [stableToken.address], [attestationFee.toString()], ] diff --git a/packages/protocol/migrationsConfig.js b/packages/protocol/migrationsConfig.js index c5747fa40b3..8dbfa49d536 100644 --- a/packages/protocol/migrationsConfig.js +++ b/packages/protocol/migrationsConfig.js @@ -8,8 +8,9 @@ BigNumber.config({ EXPONENTIAL_AT: 1e9 }) const DefaultConfig = { attestations: { - attestationExpirySeconds: 60 * 60, // 1 hour, + attestationExpiryBlocks: (60 * 60) / 5, // 1 hour, attestationRequestFeeInDollars: 0.05, + selectIssuersWaitBlocks: 4, }, blockchainParameters: { minimumClientVersion: { diff --git a/packages/protocol/test/common/safecast.ts b/packages/protocol/test/common/safecast.ts new file mode 100644 index 00000000000..b327b67c32f --- /dev/null +++ b/packages/protocol/test/common/safecast.ts @@ -0,0 +1,48 @@ +// TODO: Remove this and use upstream when https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1926/files gets merged + +import BN = require('bn.js') +import { expect } from 'chai' +import { assertRevert } from '../../lib/test-utils' +const SafeCastTest = artifacts.require('SafeCastTest') + +contract('SafeCast', async () => { + beforeEach(async function() { + this.safeCast = await SafeCastTest.new() + }) + + function testToUint(bits) { + describe(`toUint${bits}`, () => { + const maxValue = new BN('2').pow(new BN(bits)).sub(new BN('1')) + + it('downcasts 0', async function() { + expect((await this.safeCast[`toUint${bits}`](0)).toString()).to.equal('0') + }) + + it('downcasts 1', async function() { + expect((await this.safeCast[`toUint${bits}`](1)).toString()).to.equal('1') + }) + + it(`downcasts 2^${bits} - 1 (${maxValue})`, async function() { + expect((await this.safeCast[`toUint${bits}`](maxValue)).toString()).to.equal( + maxValue.toString() + ) + }) + + it(`reverts when downcasting 2^${bits} (${maxValue.add(new BN('1'))})`, async function() { + await assertRevert( + this.safeCast[`toUint${bits}`](maxValue.add(new BN('1'))), + `SafeCast: value doesn't fit in ${bits} bits` + ) + }) + + it(`reverts when downcasting 2^${bits} + 1 (${maxValue.add(new BN('2'))})`, async function() { + await assertRevert( + this.safeCast[`toUint${bits}`](maxValue.add(new BN('2'))), + `SafeCast: value doesn't fit in ${bits} bits` + ) + }) + }) + } + + ;[8, 16, 32, 64, 128].forEach((bits) => testToUint(bits)) +}) diff --git a/packages/protocol/test/governance/election.ts b/packages/protocol/test/governance/election.ts index 4fc979117aa..d550709f635 100644 --- a/packages/protocol/test/governance/election.ts +++ b/packages/protocol/test/governance/election.ts @@ -3,22 +3,22 @@ import { assertContainSubset, assertEqualBN, assertRevert, - NULL_ADDRESS, mineBlocks, + NULL_ADDRESS, } from '@celo/protocol/lib/test-utils' import { toFixed } from '@celo/utils/lib/fixidity' import BigNumber from 'bignumber.js' import { + ElectionTestContract, + ElectionTestInstance, MockLockedGoldContract, MockLockedGoldInstance, - MockValidatorsContract, - MockValidatorsInstance, MockRandomContract, MockRandomInstance, + MockValidatorsContract, + MockValidatorsInstance, RegistryContract, RegistryInstance, - ElectionTestContract, - ElectionTestInstance, } from 'types' const ElectionTest: ElectionTestContract = artifacts.require('ElectionTest') @@ -769,6 +769,9 @@ contract('Election', (accounts: string[]) => { assert.sameMembers(actual.map((x) => x.toLowerCase()), expected.map((x) => x.toLowerCase())) } + const setRandomness = async (hash: string) => + random.addTestRandomness((await web3.eth.getBlockNumber()) + 1, hash) + beforeEach(async () => { await mockValidators.setMembers(group1, [validator1, validator2, validator3, validator4]) await mockValidators.setMembers(group2, [validator5, validator6]) @@ -788,7 +791,6 @@ contract('Election', (accounts: string[]) => { random = await MockRandom.new() await registry.setAddressFor(CeloContractName.Random, random.address) - await random.setRandom(hash1) }) describe('when a single group has >= minElectableValidators as members and received votes', () => { @@ -797,6 +799,7 @@ contract('Election', (accounts: string[]) => { }) it("should return that group's member list", async () => { + await setRandomness(hash1) assertSameAddresses(await election.electValidators(), [ validator1, validator2, @@ -814,6 +817,7 @@ contract('Election', (accounts: string[]) => { }) it('should return maxElectableValidators elected validators', async () => { + await setRandomness(hash1) assertSameAddresses(await election.electValidators(), [ validator1, validator2, @@ -833,9 +837,9 @@ contract('Election', (accounts: string[]) => { }) it('should return different results', async () => { - await random.setRandom(hash1) + await setRandomness(hash1) const valsWithHash1 = (await election.electValidators()).map((x) => x.toLowerCase()) - await random.setRandom(hash2) + await setRandomness(hash2) const valsWithHash2 = (await election.electValidators()).map((x) => x.toLowerCase()) assert.sameMembers(valsWithHash1, valsWithHash2) assert.notDeepEqual(valsWithHash1, valsWithHash2) @@ -855,6 +859,7 @@ contract('Election', (accounts: string[]) => { }) it('should elect only n members from that group', async () => { + await setRandomness(hash1) assertSameAddresses(await election.electValidators(), [ validator7, validator1, @@ -876,6 +881,7 @@ contract('Election', (accounts: string[]) => { }) it('should not elect any members from that group', async () => { + await setRandomness(hash1) assertSameAddresses(await election.electValidators(), [ validator1, validator2, @@ -894,6 +900,7 @@ contract('Election', (accounts: string[]) => { }) it('should revert', async () => { + await setRandomness(hash1) await assertRevert(election.electValidators()) }) }) diff --git a/packages/protocol/test/identity/attestations.ts b/packages/protocol/test/identity/attestations.ts index f74b0d06a7e..6a4402a96ad 100644 --- a/packages/protocol/test/identity/attestations.ts +++ b/packages/protocol/test/identity/attestations.ts @@ -2,10 +2,12 @@ import Web3 = require('web3') import { CeloContractName } from '@celo/protocol/lib/registry-utils' import { + advanceBlockNum, + assertEqualBN, assertLogMatches2, assertRevert, + assertSameAddress, NULL_ADDRESS, - timeTravel, } from '@celo/protocol/lib/test-utils' import { attestToIdentifier } from '@celo/utils' import { privateKeyToAddress } from '@celo/utils/lib/address' @@ -17,14 +19,14 @@ import { MockElectionInstance, MockLockedGoldContract, MockLockedGoldInstance, + MockRandomContract, + MockRandomInstance, MockStableTokenContract, MockStableTokenInstance, RegistryContract, RegistryInstance, TestAttestationsContract, TestAttestationsInstance, - TestRandomContract, - TestRandomInstance, } from 'types' import { getParsedSignatureOfAddress } from '../../lib/signing-utils' @@ -37,7 +39,7 @@ const Attestations: TestAttestationsContract = artifacts.require('TestAttestatio const MockStableToken: MockStableTokenContract = artifacts.require('MockStableToken') const MockElection: MockElectionContract = artifacts.require('MockElection') const MockLockedGold: MockLockedGoldContract = artifacts.require('MockLockedGold') -const Random: TestRandomContract = artifacts.require('TestRandom') +const Random: MockRandomContract = artifacts.require('MockRandom') const Registry: RegistryContract = artifacts.require('Registry') const dataEncryptionKey = '0x02f2f48ee19680706196e2e339e5da3491186e0c4c5030670656b0e01611111111' @@ -49,7 +51,7 @@ contract('Attestations', (accounts: string[]) => { let attestations: TestAttestationsInstance let mockStableToken: MockStableTokenInstance let otherMockStableToken: MockStableTokenInstance - let random: TestRandomInstance + let random: MockRandomInstance let mockElection: MockElectionInstance let mockLockedGold: MockLockedGoldInstance let registry: RegistryInstance @@ -75,7 +77,8 @@ contract('Attestations', (accounts: string[]) => { const phoneHash: string = getPhoneHash(phoneNumber) const attestationsRequested = 3 - const attestationExpirySeconds = 60 + const attestationExpiryBlocks = 60 + const selectIssuersWaitBlocks = 4 const attestationFee = new BigNumber(web3.utils.toWei('.05', 'ether').toString()) async function getVerificationCodeSignature( @@ -159,7 +162,8 @@ contract('Attestations', (accounts: string[]) => { await registry.setAddressFor(CeloContractName.LockedGold, mockLockedGold.address) await attestations.initialize( registry.address, - attestationExpirySeconds, + attestationExpiryBlocks, + selectIssuersWaitBlocks, [mockStableToken.address, otherMockStableToken.address], [attestationFee, attestationFee] ) @@ -167,11 +171,11 @@ contract('Attestations', (accounts: string[]) => { }) describe('#initialize()', () => { - it('should have set attestationExpirySeconds', async () => { - const actualAttestationExpirySeconds: number = await attestations.attestationExpirySeconds.call( + it('should have set attestationExpiryBlocks', async () => { + const actualAttestationExpiryBlocks: number = await attestations.attestationExpiryBlocks.call( this ) - assert.equal(actualAttestationExpirySeconds, attestationExpirySeconds) + assert.equal(actualAttestationExpiryBlocks, attestationExpiryBlocks) }) it('should have set the fee', async () => { @@ -183,7 +187,8 @@ contract('Attestations', (accounts: string[]) => { await assertRevert( attestations.initialize( registry.address, - attestationExpirySeconds, + attestationExpiryBlocks, + selectIssuersWaitBlocks, [mockStableToken.address], [attestationFee] ) @@ -320,28 +325,28 @@ contract('Attestations', (accounts: string[]) => { }) }) - describe('#setAttestationExpirySeconds()', () => { - const newMaxNumBlocksPerAttestation = attestationExpirySeconds + 1 + describe('#setAttestationExpiryBlocks()', () => { + const newMaxNumBlocksPerAttestation = attestationExpiryBlocks + 1 - it('should set attestationExpirySeconds', async () => { - await attestations.setAttestationExpirySeconds(newMaxNumBlocksPerAttestation) - const actualAttestationExpirySeconds = await attestations.attestationExpirySeconds.call(this) - assert.equal(actualAttestationExpirySeconds, newMaxNumBlocksPerAttestation) + it('should set attestationExpiryBlocks', async () => { + await attestations.setAttestationExpiryBlocks(newMaxNumBlocksPerAttestation) + const actualAttestationExpiryBlocks = await attestations.attestationExpiryBlocks.call(this) + assert.equal(actualAttestationExpiryBlocks, newMaxNumBlocksPerAttestation) }) - it('should emit the AttestationExpirySecondsSet event', async () => { - const response = await attestations.setAttestationExpirySeconds(newMaxNumBlocksPerAttestation) + it('should emit the AttestationExpiryBlocksSet event', async () => { + const response = await attestations.setAttestationExpiryBlocks(newMaxNumBlocksPerAttestation) assert.lengthOf(response.logs, 1) const event = response.logs[0] assertLogMatches2(event, { - event: 'AttestationExpirySecondsSet', + event: 'AttestationExpiryBlocksSet', args: { value: new BigNumber(newMaxNumBlocksPerAttestation) }, }) }) it('should revert when set by a non-owner', async () => { await assertRevert( - attestations.setAttestationExpirySeconds(newMaxNumBlocksPerAttestation, { + attestations.setAttestationExpiryBlocks(newMaxNumBlocksPerAttestation, { from: accounts[1], }) ) @@ -386,57 +391,56 @@ contract('Attestations', (accounts: string[]) => { }) }) - describe('#request()', () => { - it('should increment the number of attestations requested', async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + describe('#setSelectIssuersWaitBlocks()', () => { + const newSelectIssuersWaitBlocks = selectIssuersWaitBlocks + 1 - const [completed, total] = await attestations.getAttestationStats(phoneHash, caller) - assert.equal(completed.toNumber(), 0) - assert.equal(total.toNumber(), attestationsRequested) + it('should set selectIssuersWaitBlocks', async () => { + await attestations.setSelectIssuersWaitBlocks(newSelectIssuersWaitBlocks) + const actualAttestationExpiryBlocks = await attestations.selectIssuersWaitBlocks.call(this) + assert.equal(actualAttestationExpiryBlocks, newSelectIssuersWaitBlocks) }) - it('should set the mostRecentAttestationRequested timestamp', async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) - - const requestBlock = await web3.eth.getBlock('latest') - const mostRecentAttestationRequested = await attestations.getMostRecentAttestationRequest( - caller - ) - - assert.equal(requestBlock.timestamp.toString(), mostRecentAttestationRequested.toString()) + it('should emit the SelectIssuersWaitBlocksSet event', async () => { + const response = await attestations.setSelectIssuersWaitBlocks(newSelectIssuersWaitBlocks) + assert.lengthOf(response.logs, 1) + const event = response.logs[0] + assertLogMatches2(event, { + event: 'SelectIssuersWaitBlocksSet', + args: { value: new BigNumber(newSelectIssuersWaitBlocks) }, + }) }) - it('should add the correct number attestation issuers', async () => { - assert.isEmpty(await attestations.getAttestationIssuers(phoneHash, caller)) - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) - - const attestationIssuers = await attestations.getAttestationIssuers(phoneHash, caller) - assert.lengthOf(attestationIssuers, attestationsRequested) - assert.lengthOf(uniq(attestationIssuers), attestationsRequested) + it('should revert when set by a non-owner', async () => { + await assertRevert( + attestations.setSelectIssuersWaitBlocks(newSelectIssuersWaitBlocks, { + from: accounts[1], + }) + ) }) + }) - it('should set the block of request', async () => { + describe('#request()', () => { + it('should indicate an unselected attestation request', async () => { await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + const requestBlock = await web3.eth.getBlock('latest') - const expectedBlock = await web3.eth.getBlock('latest') - - const attestationIssuers = await attestations.getAttestationIssuers(phoneHash, caller) - - await Promise.all( - attestationIssuers.map(async (issuer) => { - const [status, time] = await attestations.getAttestationState(phoneHash, caller, issuer) + const [ + blockNumber, + actualAttestationsRequested, + actualAttestationRequestFeeToken, + ] = await attestations.getUnselectedRequest(phoneHash, caller) - assert.equal(status.toNumber(), 1) - assert.equal(time.toNumber(), expectedBlock.timestamp) - }) - ) + assertEqualBN(blockNumber, requestBlock.number) + assertEqualBN(attestationsRequested, actualAttestationsRequested) + assertSameAddress(actualAttestationRequestFeeToken, mockStableToken.address) }) - it('should set the attestationRequestFeeToken', async () => { + it('should increment the number of attestations requested', async () => { await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) - const token = await attestations.getAttestationRequestFeeToken(caller) - assert.equal(token, mockStableToken.address) + const [completed, total] = await attestations.getAttestationStats(phoneHash, caller) + assertEqualBN(completed, 0) + assertEqualBN(total, attestationsRequested) }) it('should revert if 0 attestations are requested', async () => { @@ -468,32 +472,117 @@ contract('Attestations', (accounts: string[]) => { await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) }) - it('should allow to request more attestations', async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + describe('when the issuers have not yet been revealed', () => { + it('should revert requesting more attestations', async () => { + await assertRevert(attestations.request(phoneHash, 1, mockStableToken.address)) + }) - const attestationIssuers = await attestations.getAttestationIssuers(phoneHash, caller) + describe('when the original request has expired', () => { + it('should allow to request more attestations', async () => { + await advanceBlockNum(attestationExpiryBlocks, web3) + await attestations.request(phoneHash, 1, mockStableToken.address) + }) + }) + }) + + describe('when the issuers have been revealed', async () => { + beforeEach(async () => { + const requestBlockNumber = await web3.eth.getBlockNumber() + await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks, '0x1') + await attestations.selectIssuers(phoneHash) + }) - assert.lengthOf(attestationIssuers, attestationsRequested * 2) - assert.lengthOf(uniq(attestationIssuers), attestationsRequested * 2) + it('should allow to request more attestations', async () => { + await attestations.request(phoneHash, 1, mockStableToken.address) + const [completed, total] = await attestations.getAttestationStats(phoneHash, caller) + assert.equal(completed.toNumber(), 0) + assert.equal(total.toNumber(), attestationsRequested + 1) + }) }) + }) + }) - it('should revert if a different fee token is provided', async () => { - await assertRevert( - attestations.request(phoneHash, attestationsRequested, otherMockStableToken.address) - ) + describe('#selectIssuers()', () => { + let expectedRequestBlockNumber: number + describe('when attestations were requested', () => { + beforeEach(async () => { + await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + expectedRequestBlockNumber = await web3.eth.getBlockNumber() }) - describe('if attestationExpirySeconds has passed', async () => { + // These tests/functionality implicitly relies on randomness to only be available + // historically. The attestation contract itself will not test that the current block + // number is sufficiently in the future after the request block + describe('when the randomness of the right block has been set', async () => { beforeEach(async () => { - await timeTravel(attestationExpirySeconds + 1, web3) + const requestBlockNumber = await web3.eth.getBlockNumber() + await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks, '0x1') }) - it('should allow using a different attestationRequestFeeToken', async () => { - await attestations.request(phoneHash, attestationsRequested, otherMockStableToken.address) + it('should add the correct number attestation issuers', async () => { + assert.isEmpty(await attestations.getAttestationIssuers(phoneHash, caller)) + await attestations.selectIssuers(phoneHash) - const newFeeToken = await attestations.getAttestationRequestFeeToken(caller) - assert.equal(newFeeToken, otherMockStableToken.address) + const attestationIssuers = await attestations.getAttestationIssuers(phoneHash, caller) + assert.lengthOf(attestationIssuers, attestationsRequested) + assert.lengthOf(uniq(attestationIssuers), attestationsRequested) }) + + it('should set the block of request in the attestation', async () => { + await attestations.selectIssuers(phoneHash) + const attestationIssuers = await attestations.getAttestationIssuers(phoneHash, caller) + + await Promise.all( + attestationIssuers.map(async (issuer) => { + const [status, requestBlock] = await attestations.getAttestationState( + phoneHash, + caller, + issuer + ) + + assert.equal(status.toNumber(), 1) + assert.equal(requestBlock.toNumber(), expectedRequestBlockNumber) + }) + ) + }) + + it('should delete the unselected request', async () => { + await attestations.selectIssuers(phoneHash) + const [ + blockNumber, + actualAttestationsRequested, + ] = await attestations.getUnselectedRequest(phoneHash, caller) + assertEqualBN(blockNumber, 0) + assertEqualBN(actualAttestationsRequested, 0) + }) + + it('should emit the AttestationIssuersSelected event', async () => { + const response = await attestations.selectIssuers(phoneHash) + + assert.lengthOf(response.logs, 1) + const event = response.logs[0] + assertLogMatches2(event, { + event: 'AttestationIssuersSelected', + args: { + identifier: phoneHash, + account: caller, + attestationsRequested: new BigNumber(attestationsRequested), + attestationRequestFeeToken: mockStableToken.address, + }, + }) + }) + }) + + it('should revert when selecting too soon', async () => { + const requestBlockNumber = await web3.eth.getBlockNumber() + await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks - 1, '0x1') + await assertRevert(attestations.selectIssuers(phoneHash)) + }) + }) + + describe('without requesting attestations before', () => { + it('should revert when revealing issuers', async () => { + await assertRevert(attestations.selectIssuers(phoneHash)) }) }) }) @@ -502,7 +591,7 @@ contract('Attestations', (accounts: string[]) => { let issuer: string beforeEach(async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + await requestAttestations() issuer = (await attestations.getAttestationIssuers(phoneHash, caller))[0] }) @@ -525,7 +614,7 @@ contract('Attestations', (accounts: string[]) => { }) it('should revert if the request as expired', async () => { - await timeTravel(attestationExpirySeconds, web3) + await advanceBlockNum(attestationExpiryBlocks, web3) // @ts-ignore await assertRevert(attestations.reveal(phoneHash, phoneHash, issuer, false)) }) @@ -537,7 +626,7 @@ contract('Attestations', (accounts: string[]) => { let r: string, s: string beforeEach(async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + await requestAttestations() issuer = (await attestations.getAttestationIssuers(phoneHash, caller))[0] ;[v, r, s] = await getVerificationCodeSignature(caller, issuer) }) @@ -573,15 +662,21 @@ contract('Attestations', (accounts: string[]) => { assert.equal(numTotal.toNumber(), attestationsRequested) }) - it('should set the block number of the successful completion', async () => { + it('should set the time of the successful completion', async () => { + await advanceBlockNum(1, web3) await attestations.complete(phoneHash, v, r, s) const expectedBlock = await web3.eth.getBlock('latest') - const [status, time] = await attestations.getAttestationState(phoneHash, caller, issuer) + const [ + status, + completionBlock, + actualAttestationRequestFeeToken, + ] = await attestations.getAttestationState(phoneHash, caller, issuer) assert.equal(status.toNumber(), 2) - assert.equal(time.toNumber(), expectedBlock.timestamp) + assert.equal(completionBlock.toNumber(), expectedBlock.number) + assertSameAddress(actualAttestationRequestFeeToken, NULL_ADDRESS) }) it('should increment pendingWithdrawals for the rewards recipient', async () => { @@ -652,7 +747,7 @@ contract('Attestations', (accounts: string[]) => { }) it('does not let you verify beyond the window', async () => { - await timeTravel(attestationExpirySeconds, web3) + await advanceBlockNum(attestationExpiryBlocks, web3) await assertRevert(attestations.complete(phoneHash, v, r, s)) }) }) @@ -660,7 +755,7 @@ contract('Attestations', (accounts: string[]) => { describe('#withdraw()', () => { let issuer: string beforeEach(async () => { - await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + await requestAttestations() issuer = (await attestations.getAttestationIssuers(phoneHash, caller))[0] const [v, r, s] = await getVerificationCodeSignature(caller, issuer) await attestations.complete(phoneHash, v, r, s) @@ -702,6 +797,9 @@ contract('Attestations', (accounts: string[]) => { const requestAttestations = async () => { await attestations.request(phoneHash, attestationsRequested, mockStableToken.address) + const requestBlockNumber = await web3.eth.getBlockNumber() + await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks, '0x1') + await attestations.selectIssuers(phoneHash) } const requestAndCompleteAttestations = async () => { await requestAttestations() @@ -807,6 +905,10 @@ contract('Attestations', (accounts: string[]) => { await attestations.request(phoneHash, attestationsRequested, mockStableToken.address, { from: other, }) + const requestBlockNumber = await web3.eth.getBlockNumber() + await random.addTestRandomness(requestBlockNumber + selectIssuersWaitBlocks, '0x1') + await attestations.selectIssuers(phoneHash, { from: other }) + const issuer = (await attestations.getAttestationIssuers(phoneHash, other))[0] const [v, r, s] = await getVerificationCodeSignature(other, issuer) await attestations.complete(phoneHash, v, r, s, { from: other }) diff --git a/packages/utils/src/attestations.ts b/packages/utils/src/attestations.ts index 87eed41a84d..cc78ff59e97 100644 --- a/packages/utils/src/attestations.ts +++ b/packages/utils/src/attestations.ts @@ -23,6 +23,10 @@ export function attestationMessageToSign(identifier: string, account: string) { return messageHash } +export function base64ToHex(base64String: string) { + return '0x' + Buffer.from(base64String, 'base64').toString('hex') +} + export function attestToIdentifier( identifier: string, account: string,