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
| * @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?
| * @param _circle A new saving circle | ||
| */ | ||
| function create(Circle memory _circle) external override { | ||
| bytes32 _id = keccak256(abi.encodePacked(_circle.name)); |
There was a problem hiding this comment.
I was thinking a lot about how maybe the ID should be a hash of all the variables + timestamp? so there can be name collisions. Not sure if it's worth it or not.
There was a problem hiding this comment.
tbh i cant think of any reason to not just use serial ids (like you would do if you were just minting new NFTs) and return the value when a new circle is created
| * @notice Make a withdrawal from a specified circle | ||
| * @param _id Identifier of the circle | ||
| */ | ||
| function withdraw(bytes32 _id) external override nonReentrant { |
There was a problem hiding this comment.
As discussed, we might want to make a withdrawFor function in order to allow withdrawing for another member in the event they are unable (like the depositFor functionality)
| 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
…use of common scripts
