feat/synchronized project change

[RonTuretzky]

Addressing #26

[RonTuretzky]

My feeling here is adding or removing a project should only require the wallet addresses of the projects being added/removed. 2 separate methods where only the wallet addresses concerned are passed would be a cleaner API imo.

You'd then be able to emit explicit events when a project is added or removed. With this implementation you'd need to compare the array of projects before and after a change to see whether a project was added or removed.

Another thing with separate methods is you'd be able to validate the input when trying to remove a project by checking the wallet address is actually in the current projects array.

Sure , I'll implement that and add events 👍🏻

[bagelface]

Sorry I have to dissent on this one. I think updating them individually like this adds a ton of overhead logic that doesn't need to exist. Updating the entire array is simpler and more gas efficient.

[subject026]

I'd be reluctant to concede on this tbh. Updating the entire array is definitely simpler from one perspective but it creates complexity for users.

Rather than just passing the address of the wallet to be added/removed the user has to fetch the array of existing projects and add/remove the address manually. Then to see what is queued up for the next disbursement round they'd have to compare both arrays to see what was due to change.

I really don't think the difference in gas is worth thinking about. Cost to deploy according to the gas reports would be 1,520,486 / 1,735,583 which is like $0.005 at current prices and it looks like the cost of any of these methods is a tiny fraction of that.

[RonTuretzky]

I'd be reluctant to concede on this tbh. Updating the entire array is definitely simpler from one perspective but it creates complexity for users.

Rather than just passing the address of the wallet to be added/removed the user has to fetch the array of existing projects and add/remove the address manually. Then to see what is queued up for the next disbursement round they'd have to compare both arrays to see what was due to change.

I really don't think the difference in gas is worth thinking about. Cost to deploy according to the gas reports would be 1,520,486 / 1,735,583 which is like $0.005 at current prices and it looks like the cost of any of these methods is a tiny fraction of that.

It's already implemented anyways, do you approve of the changes?

[bagelface]

I'd be reluctant to concede on this tbh. Updating the entire array is definitely simpler from one perspective but it creates complexity for users. Rather than just passing the address of the wallet to be added/removed the user has to fetch the array of existing projects and add/remove the address manually. Then to see what is queued up for the next disbursement round they'd have to compare both arrays to see what was due to change. I really don't think the difference in gas is worth thinking about. Cost to deploy according to the gas reports would be 1,520,486 / 1,735,583 which is like $0.005 at current prices and it looks like the cost of any of these methods is a tiny fraction of that.

It's already implemented anyways, do you approve of the changes?

This is the type of thing that requires abstraction from the UI. The point of a smart contract is to be as simple and efficient as possible. It shouldn't be designed with "quality of life" features if they make any kind of compromises in complexity. Unfortunately, I think our smart contract is already too complex as it is. Extra view functions and events are fine. A nested for loop just to add/remove a project is way beyond the limit of acceptability imo.

Also, fwiw, we are not really concerned with deployment costs. We're concerned with execution costs.

[subject026]

OK but atm there are no plans to build a UI for adding/removing member projects so we're talking about creating a transaction manually in safe.

I think you've misunderstood my point re gas. Difference in execution costs between the two approaches is a tiny fraction of the deployment costs. I didn't share the gas costs for the individual methods because the report output depends on what you're doing in the tests and without checking I wasn't sure they were doing equivalent work. Shared the 2 reports below so you can see what I mean.

That all said let's just revert the changes and move forward with it - no problem.

src/YieldDisburser.sol:YieldDisburser contract
Deployment Cost	Deployment Size
1520486	6890
Function Name	min	avg	median	max	# calls
breadchainProjects	702	1568	1568	2434	2
castVote	77346	102828	97246	114346	769
distributeYield	255122	353442	358671	358671	258
getVotingPowerForPeriod	566	49011	9369	308017	1031
initialize	116514	127983	127983	139452	12
minimumTimeBetweenClaims	424	1424	1424	2424	2
queuedBreadchainProjects	457	457	457	457	1
setLastClaimedBlocknumber	24544	24544	24544	24544	258
setMinimumTimeBetweenClaims	24833	24833	24833	24833	258
setProjects	69543	69543	69543	69543	1
setlastClaimedTimestamp	7571	7571	7571	7571	258

src/YieldDisburser.sol:YieldDisburser contract
Deployment Cost	Deployment Size
1735583	7886
Function Name	min	avg	median	max	# calls
breadchainProjects	724	2634	2456	4724	3
castVote	77373	102815	97273	114373	770
distributeYield	265341	367430	372813	372813	258
getBreadchainProjectsLength	348	348	348	348	1
getVotingPowerForPeriod	566	49071	9396	308935	1032
initialize	116448	127917	127917	139386	12
minimumTimeBetweenClaims	335	1335	1335	2335	2
queueProjectAddition	51646	51646	51646	51646	1
queueProjectRemoval	7247	29450	29450	51653	2
queuedProjectsForAddition	414	414	414	414	1
queuedProjectsForRemoval	392	392	392	392	1
setLastClaimedBlocknumber	24566	24566	24566	24566	258
setMinimumTimeBetweenClaims	24788	24788	24788	24788	258
setlastClaimedTimestamp	7593	7593	7593	7593	258

[RonTuretzky]

OK but atm there are no plans to build a UI for adding/removing member projects so we're talking about creating a transaction manually in safe.

I think you've misunderstood my point re gas. Difference in execution costs between the two approaches is a tiny fraction of the deployment costs. I didn't share the gas costs for the individual methods because the report output depends on what you're doing in the tests and without checking I wasn't sure they were doing equivalent work. Shared the 2 reports below so you can see what I mean.

That all said let's just revert the changes and move forward with it - no problem.

src/YieldDisburser.sol:YieldDisburser contract
Deployment Cost Deployment Size
1520486 6890
Function Name min avg median max # calls breadchainProjects 702 1568 1568 2434 2 castVote 77346 102828 97246 114346 769 distributeYield 255122 353442 358671 358671 258 getVotingPowerForPeriod 566 49011 9369 308017 1031 initialize 116514 127983 127983 139452 12 minimumTimeBetweenClaims 424 1424 1424 2424 2 queuedBreadchainProjects 457 457 457 457 1 setLastClaimedBlocknumber 24544 24544 24544 24544 258 setMinimumTimeBetweenClaims 24833 24833 24833 24833 258 setProjects 69543 69543 69543 69543 1 setlastClaimedTimestamp 7571 7571 7571 7571 258 src/YieldDisburser.sol:YieldDisburser contract
Deployment Cost Deployment Size
1735583 7886
Function Name min avg median max # calls breadchainProjects 724 2634 2456 4724 3 castVote 77373 102815 97273 114373 770 distributeYield 265341 367430 372813 372813 258 getBreadchainProjectsLength 348 348 348 348 1 getVotingPowerForPeriod 566 49071 9396 308935 1032 initialize 116448 127917 127917 139386 12 minimumTimeBetweenClaims 335 1335 1335 2335 2 queueProjectAddition 51646 51646 51646 51646 1 queueProjectRemoval 7247 29450 29450 51653 2 queuedProjectsForAddition 414 414 414 414 1 queuedProjectsForRemoval 392 392 392 392 1 setLastClaimedBlocknumber 24566 24566 24566 24566 258 setMinimumTimeBetweenClaims 24788 24788 24788 24788 258 setlastClaimedTimestamp 7593 7593 7593 7593 258

I'm electing to merge this in for now as the complex logic is already implemented and tested, the gas overhead is negligible as this will be used very infrequently and if we elect to remove this in the future it will be very simple to do so.