poc for salary contract #2
Conversation
|
|
||
| let mut current_index = 0_u32; | ||
| loop { | ||
| if (current_index == contribution_data.len()) { |
There was a problem hiding this comment.
Just in case I'd put >= here since you are incrementing not +1 but +2
There was a problem hiding this comment.
contribution_data array will always be appended in the multiple of two objects. So if the current condition fails to terminate, will know the data was not correctly updated.
| // view functions | ||
| fn token(self: @TContractState) -> ContractAddress; | ||
| fn master(self: @TContractState) -> ContractAddress; | ||
| fn get_cum_salary(self: @TContractState, contributor: ContractAddress) -> u256; |
There was a problem hiding this comment.
Would such functions be useful?
get_cum_salary_by_monthset_masterset_token
There was a problem hiding this comment.
get_cum_salary_by_month -> maybe, if frontend need it can add it.
set_master -> added
set_token -> can't change token as user claiming prior salary, will withdraw in new tokens as well. Although we can have multiple tokens and set which month salary will be payout in which tokens, but it will just complicate without adding any value.
| fn add_fund_to_salary_pools(ref self: ContractState, month_id: u32, amounts: Array<u256>, guilds: Array<felt252>) { | ||
| self._only_owner(); | ||
| let caller = get_caller_address(); | ||
| let contract_address = get_contract_address(); |
There was a problem hiding this comment.
Could you change the object name? For example to: recipient_contract_address. Right now it's the same name as in line 194 (let tokenDispatcher = IERC20Dispatcher { contract_address: token }), which can be misleading.
There was a problem hiding this comment.
naming as recipient_contract_address will give the perception that tokens are going to some other wallet/contract instead of the same contract. will think of a better name.
| @@ -0,0 +1,418 @@ | |||
| use array::{Array, ArrayTrait, SpanTrait}; | |||
There was a problem hiding this comment.
I'd add tests for:
- the function
add_fund_to_salary_poolsto check the behavior if theamountsandguildsobjects have different lengths of values (should revert) - the function
update_cum_salarythat doesn't update the cum salary if it is already up-to-date?
There was a problem hiding this comment.
added test_add_fund_length_mismatch
There was a problem hiding this comment.
for verifying that cum_salary is not updated, there are two ways to check, either verify that CumulativeSalaryUpdated is not emitted or check the gas used by function call, it should be much lesser (probably constant) in case when cum salary is not updated
| Event { from: salary_address, name: 'SalaryPoolUpdated', keys: array![], data: event_data_research } | ||
| ]); | ||
|
|
||
| } |
There was a problem hiding this comment.
Should the _last_update_month_id object value be checked?
| stop_prank(usdc); | ||
|
|
||
| start_prank(salary_address, deployer_addr()); | ||
| salary_dispatcher.add_fund_to_salary_pools(092023, amounts1, guilds.clone()); |
There was a problem hiding this comment.
What's the reason for using guilds.clone()? In the previous tests, you are not using clone() method. I'm just curious :)
There was a problem hiding this comment.
when you parse a variable as an input in function, it is moved. So for using the same variable multiple places, we need to clone it.
|
|
||
| // verifying claimed_salary is updated | ||
| let user1_claimed_salary = salary_dispatcher.get_claimed_salary(user1()); | ||
| assert(user1_claimed_salary == user1_expected_cum_salary, 'incorrect claimed amount'); |
There was a problem hiding this comment.
Could you also check if the balance of the salary contract has changed? As you did in the line 343-344
|
|
||
| // verifying claimed_salary is updated | ||
| let user2_claimed_salary = salary_dispatcher.get_claimed_salary(user2()); | ||
| assert(user2_claimed_salary == user2_expected_cum_salary, 'incorrect claimed amount'); |
No description provided.