From ea1b8b5a6a193c3dc7844abd4dd542ebafd6c369 Mon Sep 17 00:00:00 2001 From: Calvin Wang Date: Tue, 13 Jun 2023 21:19:04 +1000 Subject: [PATCH 1/5] add more aggregator cairo tests --- contracts/src/tests/test_aggregator.cairo | 225 +++++++++++++++++++--- 1 file changed, 202 insertions(+), 23 deletions(-) diff --git a/contracts/src/tests/test_aggregator.cairo b/contracts/src/tests/test_aggregator.cairo index 0c43c4195..886d849a6 100644 --- a/contracts/src/tests/test_aggregator.cairo +++ b/contracts/src/tests/test_aggregator.cairo @@ -7,6 +7,7 @@ use starknet::class_hash::Felt252TryIntoClassHash; use starknet::syscalls::deploy_syscall; use array::ArrayTrait; +use clone::Clone; use traits::Into; use traits::TryInto; use option::OptionTrait; @@ -15,12 +16,12 @@ use core::result::ResultTrait; use chainlink::ocr2::aggregator::pow; use chainlink::ocr2::aggregator::Aggregator; use chainlink::ocr2::aggregator::Aggregator::Billing; +use chainlink::ocr2::aggregator::Aggregator::PayeeConfig; use chainlink::access_control::access_controller::AccessController; +use chainlink::token::link_token::LinkToken; use chainlink::tests::test_ownable::should_implement_ownable; use chainlink::tests::test_access_controller::should_implement_access_control; -// TODO: aggregator tests - #[test] #[available_gas(10000000)] fn test_pow_2_0() { @@ -67,7 +68,18 @@ trait IAccessController { // importing from access_controller.cairo doesnt work fn disable_access_check(); } -fn setup() -> (ContractAddress, ContractAddress, ContractAddress, IAccessControllerDispatcher) { +#[abi] +trait ILinkToken { + fn has_access(user: ContractAddress, data: Array) -> bool; + fn add_access(user: ContractAddress); + fn remove_access(user: ContractAddress); + fn enable_access_check(); + fn disable_access_check(); +} + +fn setup() -> ( + ContractAddress, ContractAddress, IAccessControllerDispatcher, ILinkTokenDispatcher +) { let acc1: ContractAddress = contract_address_const::<777>(); let acc2: ContractAddress = contract_address_const::<888>(); // set acc1 as default caller @@ -78,14 +90,22 @@ fn setup() -> (ContractAddress, ContractAddress, ContractAddress, IAccessControl calldata.append(acc1.into()); // owner = acc1; let (billingAccessControllerAddr, _) = deploy_syscall( AccessController::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ) - .unwrap(); + ).unwrap(); let billingAccessController = IAccessControllerDispatcher { contract_address: billingAccessControllerAddr }; - // return accounts and billing access controller - (acc1, acc2, billingAccessControllerAddr, billingAccessController) + // deploy link token contract + let mut calldata = ArrayTrait::new(); + calldata.append(acc1.into()); // minter = acc1; + calldata.append(acc1.into()); // owner = acc1; + let (linkTokenAddr, _) = deploy_syscall( + LinkToken::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false + ).unwrap(); + let linkToken = ILinkTokenDispatcher { contract_address: linkTokenAddr }; + + // return accounts, billing access controller, link token + (acc1, acc2, billingAccessController, linkToken) } #[test] @@ -103,8 +123,7 @@ fn test_ownable() { calldata.append(123); // description let (aggregatorAddr, _) = deploy_syscall( Aggregator::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ) - .unwrap(); + ).unwrap(); should_implement_ownable(aggregatorAddr, account); } @@ -124,8 +143,7 @@ fn test_access_control() { calldata.append(123); // description let (aggregatorAddr, _) = deploy_syscall( Aggregator::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ) - .unwrap(); + ).unwrap(); should_implement_access_control(aggregatorAddr, account); } @@ -145,23 +163,27 @@ fn test_upgrade_non_owner() { #[available_gas(2000000)] #[should_panic(expected: ('Ownable: caller is not owner', ))] fn test_set_billing_access_controller_not_owner() { - let (owner, acc2, billingAccessControllerAddr, _) = setup(); - // Aggregator initialization + let (owner, acc2, billingAccessController, _) = setup(); Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); // set billing access controller should revert if caller is not owner set_caller_address(acc2); - Aggregator::set_billing_access_controller(billingAccessControllerAddr); + Aggregator::set_billing_access_controller(billingAccessController.contract_address); } #[test] #[available_gas(2000000)] #[should_panic(expected: ('caller does not have access', ))] fn test_set_billing_config_no_access() { - let (owner, acc2, billingAccessControllerAddr, _) = setup(); - // Aggregator initialization + let (owner, acc2, billingAccessController, _) = setup(); Aggregator::constructor( - owner, contract_address_const::<777>(), 0, 100, billingAccessControllerAddr, 8, 123 + owner, + contract_address_const::<777>(), + 0, + 100, + billingAccessController.contract_address, + 8, + 123 ); // set billing config as acc2 with no access @@ -178,10 +200,15 @@ fn test_set_billing_config_no_access() { #[test] #[available_gas(2000000)] fn test_set_billing_config_as_owner() { - let (owner, _, billingAccessControllerAddr, _) = setup(); - // Aggregator initialization + let (owner, _, billingAccessController, _) = setup(); Aggregator::constructor( - owner, contract_address_const::<777>(), 0, 100, billingAccessControllerAddr, 8, 123 + owner, + contract_address_const::<777>(), + 0, + 100, + billingAccessController.contract_address, + 8, + 123 ); // set billing config as owner @@ -204,14 +231,19 @@ fn test_set_billing_config_as_owner() { #[test] #[available_gas(2000000)] fn test_set_billing_config_as_acc_with_access() { - let (owner, acc2, billingAccessControllerAddr, billingAccessController) = setup(); + let (owner, acc2, billingAccessController, _) = setup(); // grant acc2 access on access controller set_contract_address(owner); billingAccessController.add_access(acc2); - // Aggregator initialization Aggregator::constructor( - owner, contract_address_const::<777>(), 0, 100, billingAccessControllerAddr, 8, 123 + owner, + contract_address_const::<777>(), + 0, + 100, + billingAccessController.contract_address, + 8, + 123 ); // set billing config as acc2 with access @@ -231,3 +263,150 @@ fn test_set_billing_config_as_acc_with_access() { assert(billing.gas_base == 1, 'should be 1'); assert(billing.gas_per_signature == 1, 'should be 1'); } + +// --- Payee Management Tests --- + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('Ownable: caller is not owner', ))] +fn test_set_payees_caller_not_owner() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: acc2, payee: acc2, }); + + // set payee should revert if caller is not owner + set_caller_address(acc2); + Aggregator::set_payees(payees); +} + +#[test] +#[available_gas(2000000)] +fn test_set_single_payee() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: acc2, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); +} + +#[test] +#[available_gas(2000000)] +fn test_set_multiple_payees() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: acc2, payee: acc2, }); + payees.append(PayeeConfig { transmitter: owner, payee: owner, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('only current payee can update', ))] +fn test_transfer_payeeship_caller_not_payee() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let transmitter = contract_address_const::<123>(); + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: transmitter, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); + Aggregator::transfer_payeeship(transmitter, owner); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('cannot transfer to self', ))] +fn test_transfer_payeeship_to_self() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let transmitter = contract_address_const::<123>(); + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: transmitter, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); + set_caller_address(acc2); + Aggregator::transfer_payeeship(transmitter, acc2); +} + +#[test] +#[available_gas(2000000)] +#[should_panic(expected: ('only proposed payee can accept', ))] +fn test_accept_payeeship_caller_not_proposed_payee() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let transmitter = contract_address_const::<123>(); + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: transmitter, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); + set_caller_address(acc2); + Aggregator::transfer_payeeship(transmitter, owner); + Aggregator::accept_payeeship(transmitter); +} + +#[test] +#[available_gas(2000000)] +fn test_transfer_and_accept_payeeship() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let transmitter = contract_address_const::<123>(); + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: transmitter, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); + set_caller_address(acc2); + Aggregator::transfer_payeeship(transmitter, owner); + set_caller_address(owner); + Aggregator::accept_payeeship(transmitter); +} +// --- Payments and Withdrawals Tests --- +// +// NOTE: this test suite largely incomplete as we cannot generate or mock +// off-chain signatures in cairo-test, and thus cannot generate aggregator rounds. +// We could explore testing against a mock aggregator contract with the signature +// verification logic removed in the future. + +#[test] +#[available_gas(2000000)] +fn test_owed_payment_no_rounds() { + let (owner, acc2, _, _) = setup(); + Aggregator::constructor(owner, contract_address_const::<777>(), 0, 100, acc2, 8, 123); + + let transmitter = contract_address_const::<123>(); + let mut payees = ArrayTrait::new(); + payees.append(PayeeConfig { transmitter: transmitter, payee: acc2, }); + + set_caller_address(owner); + Aggregator::set_payees(payees); + + let owed = Aggregator::owed_payment(transmitter); + assert(owed == 0, 'owed payment should be 0'); +} + +#[test] +#[available_gas(2000000)] +fn test_link_available_for_payment_no_rounds_or_funds() { + let (owner, acc2, _, linkToken) = setup(); + Aggregator::constructor(owner, linkToken.contract_address, 0, 100, acc2, 8, 123); + + let (is_negative, diff) = Aggregator::link_available_for_payment(); + assert(is_negative == true, 'is_negative should be true'); + assert(diff == 0, 'absolute_diff should be 0'); +} From 0c1191773e5e4511d8b6d745555131bf47e51d5d Mon Sep 17 00:00:00 2001 From: Calvin Wang Date: Tue, 13 Jun 2023 21:42:28 +1000 Subject: [PATCH 2/5] fix linting --- contracts/src/tests/test_aggregator.cairo | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/src/tests/test_aggregator.cairo b/contracts/src/tests/test_aggregator.cairo index 886d849a6..99682b97c 100644 --- a/contracts/src/tests/test_aggregator.cairo +++ b/contracts/src/tests/test_aggregator.cairo @@ -90,7 +90,8 @@ fn setup() -> ( calldata.append(acc1.into()); // owner = acc1; let (billingAccessControllerAddr, _) = deploy_syscall( AccessController::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ).unwrap(); + ) + .unwrap(); let billingAccessController = IAccessControllerDispatcher { contract_address: billingAccessControllerAddr }; @@ -101,7 +102,8 @@ fn setup() -> ( calldata.append(acc1.into()); // owner = acc1; let (linkTokenAddr, _) = deploy_syscall( LinkToken::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ).unwrap(); + ) + .unwrap(); let linkToken = ILinkTokenDispatcher { contract_address: linkTokenAddr }; // return accounts, billing access controller, link token @@ -123,7 +125,8 @@ fn test_ownable() { calldata.append(123); // description let (aggregatorAddr, _) = deploy_syscall( Aggregator::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ).unwrap(); + ) + .unwrap(); should_implement_ownable(aggregatorAddr, account); } @@ -143,7 +146,8 @@ fn test_access_control() { calldata.append(123); // description let (aggregatorAddr, _) = deploy_syscall( Aggregator::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false - ).unwrap(); + ) + .unwrap(); should_implement_access_control(aggregatorAddr, account); } From c9900105f2dd03192e53851a5fed0283c567c340 Mon Sep 17 00:00:00 2001 From: Calvin Wang Date: Wed, 14 Jun 2023 13:45:07 +1000 Subject: [PATCH 3/5] remove hardhat tests covered by cairo tests --- contracts/test/ocr2/aggregator.test.ts | 59 +----------- contracts/test/ocr2/aggregator_proxy.test.ts | 98 -------------------- 2 files changed, 1 insertion(+), 156 deletions(-) delete mode 100644 contracts/test/ocr2/aggregator_proxy.test.ts diff --git a/contracts/test/ocr2/aggregator.test.ts b/contracts/test/ocr2/aggregator.test.ts index 8a254bdee..943c7447e 100644 --- a/contracts/test/ocr2/aggregator.test.ts +++ b/contracts/test/ocr2/aggregator.test.ts @@ -280,66 +280,9 @@ describe('Aggregator', function () { await expectInvokeError(transmit(4, UINT128_MAX), 'median is out of min-max range') }) - it('payee management', async () => { - let payees = oracles.map((oracle) => ({ - transmitter: oracle.transmitter.starknetContract.address, - payee: oracle.transmitter.starknetContract.address, // reusing transmitter acocunts as payees for simplicity - })) - // call set_payees, should succeed because all payees are zero - await owner.invoke(aggregator, 'set_payees', { payees }) - // call set_payees, should succeed because values are unchanged - await owner.invoke(aggregator, 'set_payees', { payees }) - - let oracle = oracles[0].transmitter - let transmitter = oracle.starknetContract.address - let payee = transmitter - - let proposed_oracle = oracles[1].transmitter - let proposed_transmitter = proposed_oracle.starknetContract.address - let proposed_payee = proposed_transmitter - - // can't transfer to self - try { - await oracle.invoke(aggregator, 'transfer_payeeship', { - transmitter, - proposed: payee, - }) - expect.fail() - } catch (err: any) { - // TODO: expect(err.message).to.contain(""); - } - - // only payee can transfer - try { - await proposed_oracle.invoke(aggregator, 'transfer_payeeship', { - transmitter, - proposed: proposed_payee, - }) - expect.fail() - } catch (err: any) {} - - // successful transfer - await oracle.invoke(aggregator, 'transfer_payeeship', { - transmitter, - proposed: proposed_payee, - }) - - // only proposed payee can accept - try { - await oracle.invoke(aggregator, 'accept_payeeship', { transmitter }) - expect.fail() - } catch (err: any) {} - - // successful accept - await proposed_oracle.invoke(aggregator, 'accept_payeeship', { - transmitter, - }) - }) - it('payments and withdrawals', async () => { let oracle = oracles[0] - // NOTE: previous test changed oracle0's payee to oracle1 - let payee = oracles[1].transmitter + let payee = oracle.transmitter aggregator.call let { response: owed } = await aggregator.call('owed_payment', { transmitter: oracle.transmitter.starknetContract.address, diff --git a/contracts/test/ocr2/aggregator_proxy.test.ts b/contracts/test/ocr2/aggregator_proxy.test.ts deleted file mode 100644 index e8421c0b8..000000000 --- a/contracts/test/ocr2/aggregator_proxy.test.ts +++ /dev/null @@ -1,98 +0,0 @@ -import { assert } from 'chai' -import { starknet } from 'hardhat' -import { num } from 'starknet' -import { Account, StarknetContract, StarknetContractFactory } from 'hardhat/types/runtime' -import { TIMEOUT } from '../constants' -import { account, expectSuccessOrDeclared } from '@chainlink/starknet' - -describe('AggregatorProxy', function () { - this.timeout(TIMEOUT) - const opts = account.makeFunderOptsFromEnv() - const funder = new account.Funder(opts) - let aggregatorContractFactory: StarknetContractFactory - let proxyContractFactory: StarknetContractFactory - - let owner: Account - let aggregator: StarknetContract - let proxy: StarknetContract - - before(async function () { - aggregatorContractFactory = await starknet.getContractFactory('mock_aggregator') - proxyContractFactory = await starknet.getContractFactory('aggregator_proxy') - - owner = await starknet.OpenZeppelinAccount.createAccount() - - await funder.fund([{ account: owner.address, amount: 1e21 }]) - await owner.deployAccount() - - await expectSuccessOrDeclared(owner.declare(aggregatorContractFactory, { maxFee: 1e20 })) - aggregator = await owner.deploy(aggregatorContractFactory, { decimals: 8 }) - - await expectSuccessOrDeclared(owner.declare(proxyContractFactory, { maxFee: 1e20 })) - proxy = await owner.deploy(proxyContractFactory, { - owner: owner.address, - address: aggregator.address, - }) - - console.log(proxy.address) - }) - - describe('proxy behaviour', function () { - it('works', async () => { - // insert round into the mock - await owner.invoke(aggregator, 'set_latest_round_data', { - answer: 10, - block_num: 1, - observation_timestamp: 9, - transmission_timestamp: 8, - }) - - // query latest round - let { response: round } = await proxy.call('latest_round_data') - // TODO: split_felt the round_id and check phase=1 round=1 - assert.equal(round.answer, '10') - assert.equal(round.block_num, '1') - assert.equal(round.started_at, '9') - assert.equal(round.updated_at, '8') - - // insert a second ocr2 aggregator - let new_aggregator = await owner.deploy(aggregatorContractFactory, { decimals: 8 }) - - // insert round into the mock - await owner.invoke(new_aggregator, 'set_latest_round_data', { - answer: 12, - block_num: 2, - observation_timestamp: 10, - transmission_timestamp: 11, - }) - - // propose it to the proxy - await owner.invoke(proxy, 'propose_aggregator', { - address: new_aggregator.address, - }) - - // query latest round, it should still point to the old aggregator - round = (await proxy.call('latest_round_data')).response - assert.equal(round.answer, '10') - - // but the proposed round should be newer - round = (await proxy.call('proposed_latest_round_data')).response - assert.equal(round.answer, '12') - - // confirm the new aggregator - await owner.invoke(proxy, 'confirm_aggregator', { - address: new_aggregator.address, - }) - - const { response: address } = await proxy.call('aggregator', {}) - assert.equal(address, num.toBigInt(new_aggregator.address)) - - const { response: phaseId } = await proxy.call('phase_id', {}) - assert.equal(phaseId, 2n) - - // query latest round, it should now point to the new aggregator - round = (await proxy.call('latest_round_data')).response - assert.equal(round.answer, '12') - }) - }) -}) From 9de1c81e4ab896b34b037ccbadfb8f5701da37e2 Mon Sep 17 00:00:00 2001 From: Calvin Wang Date: Wed, 14 Jun 2023 13:51:27 +1000 Subject: [PATCH 4/5] fix incorrect abi --- contracts/src/tests/test_aggregator.cairo | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/contracts/src/tests/test_aggregator.cairo b/contracts/src/tests/test_aggregator.cairo index 99682b97c..d9173ec7f 100644 --- a/contracts/src/tests/test_aggregator.cairo +++ b/contracts/src/tests/test_aggregator.cairo @@ -69,13 +69,7 @@ trait IAccessController { // importing from access_controller.cairo doesnt work } #[abi] -trait ILinkToken { - fn has_access(user: ContractAddress, data: Array) -> bool; - fn add_access(user: ContractAddress); - fn remove_access(user: ContractAddress); - fn enable_access_check(); - fn disable_access_check(); -} +trait ILinkToken {} fn setup() -> ( ContractAddress, ContractAddress, IAccessControllerDispatcher, ILinkTokenDispatcher From 34f4d42803e37d890db575c938ccbab8b6bba831 Mon Sep 17 00:00:00 2001 From: Calvin Wang Date: Wed, 14 Jun 2023 17:44:30 +1000 Subject: [PATCH 5/5] fix test --- contracts/test/ocr2/aggregator.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/contracts/test/ocr2/aggregator.test.ts b/contracts/test/ocr2/aggregator.test.ts index 943c7447e..5fbf0f61f 100644 --- a/contracts/test/ocr2/aggregator.test.ts +++ b/contracts/test/ocr2/aggregator.test.ts @@ -281,9 +281,15 @@ describe('Aggregator', function () { }) it('payments and withdrawals', async () => { + // set up payees + let payees = oracles.map((oracle) => ({ + transmitter: oracle.transmitter.starknetContract.address, + payee: oracle.transmitter.starknetContract.address, // reusing transmitter acocunts as payees for simplicity + })) + await owner.invoke(aggregator, 'set_payees', { payees }) + let oracle = oracles[0] let payee = oracle.transmitter - aggregator.call let { response: owed } = await aggregator.call('owed_payment', { transmitter: oracle.transmitter.starknetContract.address, })