KcPele
commented
1 month ago Great
On Mon, 27 May 2024, 16:55 escottalexander, @.***> wrote:
@.**** requested changes on this pull request.
Hey KC, A few more changes suggested. Nice work implementing the new mapping. Looking great!
Feel free to reach out on Telegram if you have questions about my comments.
In README.md https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616059170 :
@@ -1,4 +1,4 @@ -# Template For Challenge - ETH Tech Tree +# Dead Man's Switch
Oh sorry, I should have been clearer. Can you change this to "Dead Man's Switch - ETH Tech Tree". This way it will match the other challenges.
In README.md https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616064468 :
@@ -30,18 +30,12 @@ foundryup
In this challenge, you will create a smart contract that implements a "Dead Man's Switch". This contract allows users to deposit funds (ETH or other tokens) and set a time interval for regular "check-ins". If the user fails to check in within the specified time frame, designated beneficiaries can withdraw the funds.
-##### Scenario:
Sorry again, KC. I meant to say you should remove the "Here are some helpful references: ..." section. You can add your scenario section back in. I liked it.
The references section was just meant to be an example of where/how you could include some links to articles or other materials related to this challenge. If you don't know of any then you can remove the section.
In packages/foundry/contracts/DeadMansSwitch.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616085971 :
// Struct to store user data struct User { uint balance; uint lastCheckIn; uint checkInInterval;
- address[] beneficiaries;
- mapping(address => bool) beneficiaries;
Could we change the name of this to "isBeneficiary" or "beneficiaryLookup"? Because it is more of a lookup to see if a specific address is a beneficiary. I think that reads better when you are looking at where it is used and it helps the user understand it's purpose.
As it is right now, there is no way to return all beneficiaries from it and I feel like that is what it communicates if it is called beneficiaries.
In packages/foundry/contracts/DeadMansSwitch.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616089705 :
* @param userAddress The address of the user.
- @return The beneficiaries of the user.
- @param beneficiary The address of the beneficiary.
- @return The check-in interval of the user.
Need to add requirements so the challenger knows what it needs to do.
In packages/foundry/contracts/DeadMansSwitch.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616093946 :
}+
- receive() external payable {}
- fallback() external payable {}
These methods are not needed so I would remove them. When I mentioned them in the last review I was trying to say that they would need to be added to the testing contract "DeadMansSwitch.t.sol" but only if you didn't add the NON_CONTRACT_USER deal and startPranks. Looks like you did so you should be good. Just remove them from here.
The risk in adding receive and fallback here is that it allows a user to deposit to the contract without the contract updating their specific balance. leading to funds stuck in the contract.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616102086 :
);
- assertEq(beneficiaries.length, 1);
- assertEq(beneficiaries[0], BENEFICIARY_1); }
// // Test withdrawing funds by the userRemove the extra set of "//".
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616113682 :
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- vm.warp(block.timestamp + INTERVAL + 1);
- vm.startPrank(BENEFICIARY_1);
- deadMansSwitch.withdrawAsBeneficiary(THIS_CONTRACT);
- assertEq(deadMansSwitch.getBalance(THIS_CONTRACT), 0);
- }
- // Test that non-beneficiaries cannot withdraw funds
- function testWithdrawAsNonBeneficiary() public {
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- vm.warp(block.timestamp + INTERVAL + 1);
- vm.startPrank(NON_CONTRACT_USER);
- vm.expectRevert("Caller is not a beneficiary");
Change this line to vm.expectRevert();
Because we don't know what revert message to expect but we do know we want to it to revert in some way.
That way no matter what revert message the user chooses for this condition the test will still pass.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616113819 :
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- vm.warp(block.timestamp + INTERVAL + 1);
- vm.startPrank(NON_CONTRACT_USER);
- vm.expectRevert("Caller is not a beneficiary");
- deadMansSwitch.withdrawAsBeneficiary(THIS_CONTRACT);
- }
- // Test that beneficiaries cannot withdraw funds before the interval has passed
- function testWithdrawAsBeneficiaryBeforeInterval() public {
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- vm.warp(block.timestamp + INTERVAL - 1);
- vm.startPrank(BENEFICIARY_1);
- vm.expectRevert("Check-in interval has not passed");
Change this line to vm.expectRevert();
Because we don't know what revert message to expect but we do know we want to it to revert in some way.
That way no matter what revert message the user chooses for this condition the test will still pass.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616116720 :
- assertEq(deadMansSwitch.getBalance(THIS_CONTRACT), 0);
- }
- // Test withdrawing funds by a beneficiary after the interval has passed
- function testWithdrawAsBeneficiary() public {
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- vm.warp(block.timestamp + INTERVAL + 1);
- vm.startPrank(BENEFICIARY_1);
- deadMansSwitch.withdrawAsBeneficiary(THIS_CONTRACT);
- assertEq(deadMansSwitch.getBalance(THIS_CONTRACT), 0);
- }
- // Test that non-beneficiaries cannot withdraw funds
- function testWithdrawAsNonBeneficiary() public {
Let's add another version of this test where a non-beneficiary tries to withdraw before the interval. It will be good to have that edge case covered just in case the challenger's logic has a hole.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616164528 :
@@ -0,0 +1,148 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +import "forge-std/Test.sol"; +import "../contracts/DeadMansSwitch.sol"; + +contract DeadMansSwitchTest is Test {
- DeadMansSwitch public deadMansSwitch;
- address THIS_CONTRACT = address(this);
- address NON_CONTRACT_USER = vm.addr(1);
- address BENEFICIARY_1 = vm.addr(2);
- uint ONE_THOUSAND = 10 wei;
Shouldn't ONE_THOUSAND be 1000 wei?
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616180250 :
- // Test the check-in functionality
- function testCheckIn() public {
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.checkIn();
- assertEq(deadMansSwitch.getLastCheckIn(THIS_CONTRACT), block.timestamp);
- }
- // Test adding a beneficiary
- function testAddBeneficiary() public {
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- assertEq(
- deadMansSwitch.isBeneficiary(THIS_CONTRACT, BENEFICIARY_1),
- true
- );
- }
- // Test removing a beneficiary
Add new line above to separate these properly.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616190861 :
- vm.deal(NON_CONTRACT_USER, ONE_THOUSAND);
- vm.startPrank(NON_CONTRACT_USER);
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.withdraw(ONE_THOUSAND);
- assertEq(deadMansSwitch.getBalance(THIS_CONTRACT), 0);
- }
- // Test withdrawing funds by a beneficiary after the interval has passed
- function testWithdrawAsBeneficiary() public {
- deadMansSwitch.deposit{value: ONE_THOUSAND}();
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- vm.warp(block.timestamp + INTERVAL + 1);
- vm.startPrank(BENEFICIARY_1);
- deadMansSwitch.withdrawAsBeneficiary(THIS_CONTRACT);
- assertEq(deadMansSwitch.getBalance(THIS_CONTRACT), 0);
Add an assertion that the beneficiaries balance was updated to the amount expected.
In packages/foundry/contracts/DeadMansSwitch.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616197196 :
- Emits a
BeneficiaryAddedevent with the caller's address and the beneficiary address.- */
- function addBeneficiary(address beneficiary) public {
- require(beneficiary != address(0), "Invalid beneficiary address");
- User storage user = users[msg.sender];
- require(!user.beneficiaries[msg.sender], "Beneficiary already added");
- user.beneficiaries[beneficiary] = true;
- emit BeneficiaryAdded(msg.sender, beneficiary);
- }
- /**
- @dev Removes a beneficiary for the user's funds.
- @param beneficiary The address of the beneficiary to remove.
- Requirements:
- The beneficiary must be already added.
- Emits a
BeneficiaryRemovedevent with the caller's address and the beneficiary address.Add a requirement that "The beneficiary is removed". Hopefully it is obvious to them but no harm in adding clarity.
In packages/foundry/contracts/DeadMansSwitch.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616211588 :
- /**
- @dev Gets the balance of the user.
- @param userAddress The address of the user.
- @return The balance of the user.
- */
- function getBalance(address userAddress) public view returns (uint) {
- return users[userAddress].balance;
- }
- /**
- @dev Gets the last check-in time of the user.
- @param userAddress The address of the user.
- @return The last check-in time of the user.
- */
- function getLastCheckIn(address userAddress) public view returns (uint) {
- return users[userAddress].lastCheckIn;
- }
- /**
- @dev Gets the check-in interval of the user.
- @param userAddress The address of the user.
- @return The check-in interval of the user.
- */
- function getCheckInInterval(
- address userAddress
- ) public view returns (uint) {
- return users[userAddress].checkInInterval;
- }
Let's remove these methods completely. We can get the values of these in the tests with (uint balance, uint lastCheckIn, uint checkInInterval) = deadmansswitch.users[ADDRESS]; Or if you only need one of them you can do this:
(uint balance,,) = deadmansswitch.users[ADDRESS]; (,uint lastCheckIn,) = deadmansswitch.users[ADDRESS]; (,,uint checkInInterval) = deadmansswitch.users[ADDRESS];
Just no reason to require a user to build them if they aren't needed.
In packages/foundry/test/DeadMansSwitchTest.t.sol https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#discussion_r1616218561 :
+
- // Test the check-in functionality
- function testCheckIn() public {
- deadMansSwitch.setCheckInInterval(INTERVAL);
- deadMansSwitch.checkIn();
- assertEq(deadMansSwitch.getLastCheckIn(THIS_CONTRACT), block.timestamp);
- }
- // Test adding a beneficiary
- function testAddBeneficiary() public {
- deadMansSwitch.addBeneficiary(BENEFICIARY_1);
- assertEq(
- deadMansSwitch.isBeneficiary(THIS_CONTRACT, BENEFICIARY_1),
- true
- );
- }
Because we are requiring that the beneficiary is not the zero address and that they can't already be a beneficiary, we should add tests to check that those cases are true.
— Reply to this email directly, view it on GitHub https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/29#pullrequestreview-2080935295, or unsubscribe https://github.com/notifications/unsubscribe-auth/APJDTO24REMYRMU3UD3RHBDZENJQDAVCNFSM6AAAAABIIS4KBCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBQHEZTKMRZGU . You are receiving this because you modified the open/close state.Message ID: @.*** com>
escottalexander
Description
Added challenge smart contract, test and description Concise description of proposed changes, We recommend using screenshots and videos for better description
Additional Information
Related Issues
Closes #{issue number}
Note: If your changes are small and straightforward, you may skip the creation of an issue beforehand and remove this section. However, for medium-to-large changes, it is recommended to have an open issue for discussion and approval prior to submitting a pull request.
Your ENS/address: