From 45f80673ebc4a36a1bf7ea957e618434be018657 Mon Sep 17 00:00:00 2001 From: Calvin <78729586+calvwang9@users.noreply.github.com> Date: Fri, 12 May 2023 18:04:41 +1000 Subject: [PATCH] BCI-1289: Add Ownable test helper (#257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * contracts/.gitignore: remove typechain-types * Move cairo 0.x code to oldsrc/ * Initial boilerplate for Cairo 1.0 Based on https://github.com/auditless/cairo-template * create confirmed owner contract * Remove old helix config * Add ocr2 aggregator skeleton * ownable.cairo: update storage * Scarb.toml: add starknet contract target * cairo_project.toml: add crate_roots to refer to crate deps * libraries: add access controller trait, simple write access controller * simple_write_access_controller.cairo: add constructor, rename to _check_enabled to match previous contract * simple_read_access_controller.cairo: add skeleton * aggregator.cairo: Round derives Serde so that it can be returned by dispatch * aggregator.cairo: change Round.round_id into felt252 * ocr2: add initial aggregator proxy * ocr2: partial set_config implementation, events * Scarb.toml: cairo-test needs --starknet flag * Add span hashing, implement most of set_config, part of transmit * Billing config, query functions * Implement payee management, payments and resolve some TODOs * Clean up recursion in set_payee too * Add ERC20 calls via abi dispatcher * Use Ownable in the aggregator * Re-export ownership functions * Slight cleanup * Implement withdraw_funds * Use access controllers as libraries in aggregator and proxy * Add a vendor/ directory with cairo and scarb version pinned * Remove unused files * ocr2/aggregator: clamp config digest hash, add prefix * Add withdraw_gas_all annotations around recursion * aggregator.cairo: pass signatures by reference with verify_signatures * aggregator.cairo: implement duplicate signer checking * aggregator.cairo: deref median immediately * Add the billing controller dispatch in billing access check * Handle negative values in link_available_for_payment * Tightly pack Transmission for storage cost savings * Optimize Oracle struct storage * Use the same split_felt function * Resolve more TODOs * aggregator.cairo: add TODOs to pow * contracts: add initial tests * aggregator.cairo: set _oracles_len to index after adding oracles * aggregator.cairo: read oracle before incrementing index * Aggregator: add transmitters view function * multisig.cairo: initial implementation * aggregator.cairo: add withdraw_gas_all to transmitters_ for cairo-test * contracts: add initial test_multisig * BCI-990: Update er protocol to Cairo 1.0 (#228) * rewrite uptime feed * remove partial eq * code review comments * add external functions from simplewriteaccesscontroller to aggregator + aggregator proxy contracts * get rid of parens + cairo-format * remove unnecessary traits * Fix compilation on alpha.6 * Makefile: build cairo, scarb, and contract tests * contracts.yml: run cairo tests * amarna.yml: disable amarna workflow * contracts/requirements.txt: remove cairo-lang and openzeppelin-cairo-contracts * Makefile: build and run cairo-format * lint.yml: checkout cairo for cairo-format * Makefile: assume the deps are already available in the env * Install cairo and scarb from github releases * also download cairo for format check * Fix the format check * Resolve formatter issues * Disable examples check for now since we haven't updated them yet * contracts/tests/test_multisig.cairo: add tests * contracts/ocr2/aggregator_proxy.cairo: add external round_data and latest_round_data * BCI-1188: Rewrite Link Token Cairo 1.0 (#234) * Makefile: build contracts with release profile * contracts: import account contracts * Solidity contracts are still used, moved to solidity/ * Remove oldsrc/ * StarknetValidator.test.ts: yarn format * Only install venv on CI actions that need it (startNetwork) * Ignore the rest of the vendor/ folder * contracts/emergency/sequencer_uptime_feed.cairo: spell transfer_ownership correctly * port over mock aggregator (#250) * aggregator: Resolve a couple TODOs * Extract split_felt to utils * proxy: We initialize with an initial value so it will never be 0 * aggregator: Reintroduce calculate_reimbursement * relayer: Stricter assertions on report format * contracts/ocr2/aggregator_proxy.cairo: require access for latest_round_data and round_data * contracts/libraries/simple_read_access_controller.cairo: check_access should call SimpleReadAccessController::has_access, readd comment about offchain calls * contracts/ocr2/mocks/mock_aggregator.cairo: set latest round id * Update account to use latest implementation * On rc0 it's no longer necessary to import this * Integer conversions can now be direct without going through felt252 * Use the new format for should_panic * cairo format * Test ERC677 with contract dispatch (#241) * port aggregator proxy tests to cairo-test * update to cairo-rc1.0 and 0.2.0-alpha.2 * contracts/ocr2/aggregator.cairo: fix epoch_and_round assertion * contracts/ocr2/aggregator.cairo: finalize hash with length * camelcase vars and undo delete file * fix file name * BCI-1259: Add upgradeability (#251) * contracts/ocr2/aggregator.cairo: add billing function * contracts: run cairo-format * .github/actions/install-cairo/action.yml: add action * .github/workflows/contracts.yml: use install-cairo action * add ownable test helper * add ownable functions to access controllers * rename ownership -> ownable * rename fn to should_implement_ownable * missed merge conflicts * fix merge errs * revert linting * add IOwnable to Ownable file * trailing whitespace --------- Co-authored-by: cfal Co-authored-by: Blaž Hrastnik Co-authored-by: Augustus <14297860+augustbleeds@users.noreply.github.com> Co-authored-by: Calvin Wang Co-authored-by: Augustus Chang Co-authored-by: Calvin Wang --- contracts/src/libraries/ownable.cairo | 11 +++++ contracts/src/tests/test_aggregator.cairo | 30 ++++++++++++- .../src/tests/test_aggregator_proxy.cairo | 16 +++++++ contracts/src/tests/test_erc677.cairo | 14 +++--- contracts/src/tests/test_link_token.cairo | 30 +++++++++++-- contracts/src/tests/test_multisig.cairo | 6 ++- contracts/src/tests/test_ownable.cairo | 45 ++++++++++++++++++- 7 files changed, 139 insertions(+), 13 deletions(-) diff --git a/contracts/src/libraries/ownable.cairo b/contracts/src/libraries/ownable.cairo index fe0fb020f..f40048b43 100644 --- a/contracts/src/libraries/ownable.cairo +++ b/contracts/src/libraries/ownable.cairo @@ -1,3 +1,14 @@ +use starknet::ContractAddress; + +#[abi] +trait IOwnable { + fn owner() -> ContractAddress; + fn proposed_owner() -> ContractAddress; + fn transfer_ownership(new_owner: ContractAddress); + fn accept_ownership(); + fn renounce_ownership(); +} + #[contract] mod Ownable { use starknet::ContractAddress; diff --git a/contracts/src/tests/test_aggregator.cairo b/contracts/src/tests/test_aggregator.cairo index a0fae8a7c..cf8552893 100644 --- a/contracts/src/tests/test_aggregator.cairo +++ b/contracts/src/tests/test_aggregator.cairo @@ -2,10 +2,18 @@ use starknet::testing::set_caller_address; use starknet::ContractAddress; use starknet::contract_address_const; use starknet::class_hash::class_hash_const; +use starknet::class_hash::Felt252TryIntoClassHash; +use starknet::syscalls::deploy_syscall; + +use array::ArrayTrait; +use traits::Into; +use traits::TryInto; +use option::OptionTrait; +use core::result::ResultTrait; use chainlink::ocr2::aggregator::pow; use chainlink::ocr2::aggregator::Aggregator; - +use chainlink::tests::test_ownable::should_implement_ownable; // TODO: aggregator tests @@ -52,6 +60,26 @@ fn setup() -> ContractAddress { account } +#[test] +#[available_gas(2000000)] +fn test_ownable() { + let account = setup(); + // Deploy aggregator + let mut calldata = ArrayTrait::new(); + calldata.append(account.into()); // owner + calldata.append(contract_address_const::<777>().into()); // link token + calldata.append(0); // min_answer + calldata.append(100); // max_answer + calldata.append(contract_address_const::<999>().into()); // billing access controller + calldata.append(8); // decimals + calldata.append(123); // description + let (aggregatorAddr, _) = deploy_syscall( + Aggregator::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false + ).unwrap(); + + should_implement_ownable(aggregatorAddr, account); +} + #[test] #[available_gas(2000000)] diff --git a/contracts/src/tests/test_aggregator_proxy.cairo b/contracts/src/tests/test_aggregator_proxy.cairo index 095a0c10e..a786b9db3 100644 --- a/contracts/src/tests/test_aggregator_proxy.cairo +++ b/contracts/src/tests/test_aggregator_proxy.cairo @@ -15,6 +15,7 @@ use chainlink::ocr2::mocks::mock_aggregator::MockAggregator; use chainlink::ocr2::aggregator_proxy::AggregatorProxy; use chainlink::ocr2::aggregator::Round; use chainlink::utils::split_felt; +use chainlink::tests::test_ownable::should_implement_ownable; #[abi] trait IMockAggregator { @@ -54,6 +55,21 @@ fn setup() -> ( (account, mockAggregatorAddr1, mockAggregator1, mockAggregatorAddr2, mockAggregator2) } +#[test] +#[available_gas(2000000)] +fn test_ownable() { + let (account, mockAggregatorAddr, _, _, _) = setup(); + // Deploy aggregator proxy + let mut calldata = ArrayTrait::new(); + calldata.append(account.into()); // owner = account + calldata.append(mockAggregatorAddr.into()); + let (aggregatorProxyAddr, _) = deploy_syscall( + AggregatorProxy::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false + ).unwrap(); + + should_implement_ownable(aggregatorProxyAddr, account); +} + #[test] #[available_gas(2000000)] #[should_panic(expected: ('Ownable: caller is not owner', ))] diff --git a/contracts/src/tests/test_erc677.cairo b/contracts/src/tests/test_erc677.cairo index 877c85a76..4aefdd8df 100644 --- a/contracts/src/tests/test_erc677.cairo +++ b/contracts/src/tests/test_erc677.cairo @@ -1,18 +1,20 @@ -use chainlink::libraries::token::erc677::ERC677; use starknet::ContractAddress; use starknet::contract_address_const; use starknet::testing::set_caller_address; +use starknet::syscalls::deploy_syscall; +use starknet::class_hash::Felt252TryIntoClassHash; + use array::ArrayTrait; use traits::Into; -use zeroable::Zeroable; -use chainlink::token::mock::valid_erc667_receiver::ValidReceiver; -use chainlink::token::mock::invalid_erc667_receiver::InvalidReceiver; -use starknet::syscalls::deploy_syscall; use traits::TryInto; +use zeroable::Zeroable; use option::OptionTrait; -use starknet::class_hash::Felt252TryIntoClassHash; use core::result::ResultTrait; +use chainlink::token::mock::valid_erc667_receiver::ValidReceiver; +use chainlink::token::mock::invalid_erc667_receiver::InvalidReceiver; +use chainlink::libraries::token::erc677::ERC677; + #[abi] trait MockValidReceiver { fn on_token_transfer(sender: ContractAddress, value: u256, data: Array); diff --git a/contracts/src/tests/test_link_token.cairo b/contracts/src/tests/test_link_token.cairo index 4550f0d32..0f934e443 100644 --- a/contracts/src/tests/test_link_token.cairo +++ b/contracts/src/tests/test_link_token.cairo @@ -1,10 +1,19 @@ -use chainlink::token::link_token::LinkToken; -use zeroable::Zeroable; use starknet::ContractAddress; use starknet::testing::set_caller_address; use starknet::contract_address_const; -use traits::Into; use starknet::class_hash::class_hash_const; +use starknet::class_hash::Felt252TryIntoClassHash; +use starknet::syscalls::deploy_syscall; + +use array::ArrayTrait; +use traits::Into; +use traits::TryInto; +use zeroable::Zeroable; +use option::OptionTrait; +use core::result::ResultTrait; + +use chainlink::token::link_token::LinkToken; +use chainlink::tests::test_ownable::should_implement_ownable; // only tests link token specific functionality // erc20 and erc677 functionality is already tested elsewhere @@ -16,6 +25,21 @@ fn setup() -> ContractAddress { account } +#[test] +#[available_gas(2000000)] +fn test_ownable() { + let account = setup(); + // Deploy LINK token + let mut calldata = ArrayTrait::new(); + calldata.append(class_hash_const::<123>().into()); // minter + calldata.append(account.into()); // owner + let (linkAddr, _) = deploy_syscall( + LinkToken::TEST_CLASS_HASH.try_into().unwrap(), 0, calldata.span(), false + ).unwrap(); + + should_implement_ownable(linkAddr, account); +} + #[test] #[available_gas(2000000)] #[should_panic(expected: ('minter is 0', ))] diff --git a/contracts/src/tests/test_multisig.cairo b/contracts/src/tests/test_multisig.cairo index beebfd0d1..95c9e7183 100644 --- a/contracts/src/tests/test_multisig.cairo +++ b/contracts/src/tests/test_multisig.cairo @@ -1,9 +1,11 @@ -use array::ArrayTrait; use starknet::contract_address_const; use starknet::testing::set_caller_address; +use starknet::class_hash::class_hash_const; + +use array::ArrayTrait; + use chainlink::multisig::assert_unique_values; use chainlink::multisig::Multisig; -use starknet::class_hash::class_hash_const; // TODO: test execute_confirmation happy path with mocked // call_contract_syscall diff --git a/contracts/src/tests/test_ownable.cairo b/contracts/src/tests/test_ownable.cairo index 7a48652d7..8a72b48eb 100644 --- a/contracts/src/tests/test_ownable.cairo +++ b/contracts/src/tests/test_ownable.cairo @@ -1,8 +1,9 @@ -use chainlink::libraries::ownable::Ownable; use starknet::contract_address_const; use starknet::ContractAddress; use starknet::testing::set_caller_address; +use starknet::testing::set_contract_address; use zeroable::Zeroable; +use chainlink::libraries::ownable::Ownable; fn setup() -> (ContractAddress, ContractAddress) { let owner: ContractAddress = contract_address_const::<1>(); @@ -121,3 +122,45 @@ fn test_renounce_ownership_panics_if_not_owner() { set_caller_address(other_user); Ownable::renounce_ownership(); } +// +// General ownable contract tests +// + +#[abi] +trait IOwnable { + fn owner() -> ContractAddress; + fn proposed_owner() -> ContractAddress; + fn transfer_ownership(new_owner: ContractAddress); + fn accept_ownership(); + fn renounce_ownership(); +} + +fn should_implement_ownable(contract_addr: ContractAddress, owner: ContractAddress) { + let contract = IOwnableDispatcher { contract_address: contract_addr }; + let acc2: ContractAddress = contract_address_const::<2222>(); + + // check owner is set correctly + assert(owner == contract.owner(), 'owner does not match'); + let caller = starknet::get_caller_address(); + assert(!caller.is_zero(), 'test1'); + + // transfer ownership - check owner unchanged and proposed owner set correctly + set_contract_address(owner); // required to call contract as owner + let caller = starknet::get_caller_address(); + assert(!caller.is_zero(), 'test2'); + assert(caller == owner, 'test3'); + contract.transfer_ownership(acc2); + assert(owner == contract.owner(), 'owner should remain unchanged'); + assert(acc2 == contract.proposed_owner(), 'acc2 should be proposed owner'); + + // accept ownership - check owner changed and proposed owner set to zero + set_contract_address(acc2); // required to call function as acc2 + contract.accept_ownership(); + assert(contract.owner() == acc2, 'failed to change ownership'); + assert(contract.proposed_owner().is_zero(), 'proposed owner should be zero'); + + // renounce ownership + contract.renounce_ownership(); + assert(contract.owner().is_zero(), 'owner not 0 after renounce'); +} +