Bagel refactor #5
Conversation
bagelface
commented
Dec 26, 2024
bagelface
commented
- Fix tests
- Refactor core functionality, variables, errors, functions, etc.
- Add NatSpec comments
- Bump Solidity version
…es, get test set up working
src/contracts/SavingCircles.sol
Outdated
| * @notice Simple implementation of a rotating savings and credit association (ROSCA) for ERC20 tokens | ||
| * @author Breadchain Collective | ||
| * @author @RonTuretzky | ||
| * @author @bagelface |
There was a problem hiding this comment.
on the YD contract everyone has their ENS, wdyt our convention should be for tagging authors?
There was a problem hiding this comment.
ya i personally like ENS, i think we should just put it however the author requests. do you want me to use your ENS for this?
There was a problem hiding this comment.
if you let me know which ENS to use for you i will change. i changed mine
| if (_circle.owner != msg.sender) { | ||
| if (!isMember[_id][msg.sender]) revert NotMember(); |
There was a problem hiding this comment.
what's the value of having access control here for the circle members as opposed to permissionless?
My concern is that practically what will happen is we /chain coop will have to decommission these, not the members themselves...
There was a problem hiding this comment.
i usually like to avoid "godmode" functions, but if we think we need it we can add it. it just gives the circle members complete control which is a good or bad thing depending on perspective
| block.timestamp < circles[_id].circleStart | ||
| || block.timestamp >= circles[_id].circleStart + (circles[_id].depositInterval * (circles[_id].currentIndex + 1)) | ||
| || block.timestamp >= circles[_id].circleStart + (circles[_id].depositInterval * circles[_id].maxDeposits) | ||
| || balances[_id][_member] + _value > circles[_id].depositAmount |
There was a problem hiding this comment.
I'm wondering if allowing flexibility in the deposit amount instead of only allowing the full amount contributes to ux or makes it more confusing.. thoughts @diterra-code ?
| error TransferFailed(); | ||
|
|
||
| // External functions (state-changing) | ||
| function initialize(address owner) external; |
There was a problem hiding this comment.
isn't the wonderland convention to put the natspec here and inheritdoc the contract?
There was a problem hiding this comment.
oh idk, is it? seems sort of strange but if that's convention i can change it
There was a problem hiding this comment.
fwiw i personally dont really like that convention
