TransferOwnership feature #26
Conversation
transfer ownership feature is added with securities.
src/DepositWithdraw.sol
Outdated
| contract DepositWithdraw is Ownable(msg.sender), ReentrancyGuard { | ||
| mapping(address => uint256) private _balances; | ||
|
|
||
| event Deposited(address indexed depositor, uint256 amount); |
There was a problem hiding this comment.
the event names would be better to relate them to the function they go with, here it would be Deposit
src/DepositWithdraw.sol
Outdated
| mapping(address => uint256) private _balances; | ||
|
|
||
| event Deposited(address indexed depositor, uint256 amount); | ||
| event Withdrew(address indexed withdrawer, uint256 amount); |
There was a problem hiding this comment.
the event names would be better to relate them to the function they go with, here it would be Withdraw
src/DepositWithdraw.sol
Outdated
| function withdraw(uint256 amount) public nonReentrant { | ||
| require(amount <= _balances[msg.sender], "Insufficient balance"); | ||
| _balances[msg.sender] -= amount; | ||
| (bool success, ) = msg.sender.call{value: amount}(""); |
There was a problem hiding this comment.
you could use .transfer here instead of .call.
There was a problem hiding this comment.
payable(msg.sender).transfer(amount);
updated!!
src/DepositWithdraw.sol
Outdated
|
|
||
| contract DepositWithdraw is Ownable(msg.sender), ReentrancyGuard { | ||
| mapping(address => uint256) private _balances; | ||
|
|
There was a problem hiding this comment.
add an event that registers the change of ownership:
event OwnershipTransferred(address indexed owner, address indexed newOwner);
src/DepositWithdraw.sol
Outdated
| emit Withdrew(msg.sender, amount); | ||
| } | ||
|
|
||
| function transferOwnership(address newOwner) public onlyOwner override { |
There was a problem hiding this comment.
emit here the OwnershipTransferred event
src/DepositWithdraw.sol
Outdated
| emit Withdrew(msg.sender, amount); | ||
| } | ||
|
|
||
| function transferOwnership(address newOwner) public onlyOwner override { |
There was a problem hiding this comment.
add more validations here so that it does not check that the new owner is not address(0) either the address of a contract.
There was a problem hiding this comment.
require(newOwner == address(0),"invalid address");
validation added!
There was a problem hiding this comment.
you can add another validation so that the input address is not a contract address
src/DepositWithdraw.sol
Outdated
There was a problem hiding this comment.
rename the contract with TransferOwnership
src/DepositWithdraw.sol
Outdated
| pragma solidity ^0.8.24; | ||
|
|
||
| import "@openzeppelin/contracts/access/Ownable.sol"; | ||
| import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; |
There was a problem hiding this comment.
the route is not correct, check this ReentrancyGuard
There was a problem hiding this comment.
import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
done!
used .transfer instead of .call for security purpose.
|
|
||
| event Deposit(address indexed depositor, uint256 amount); | ||
| event Withdraw(address indexed withdrawer, uint256 amount); | ||
| event ownershipTransferred(address indexed owner, address indexed newOwner); |
There was a problem hiding this comment.
use capitalize, replace ownershipTransferred to OwnershipTransferred and change it on the other sites
There was a problem hiding this comment.
I get this error while capitalising it
DeclarationError: Event with same name and parameter types defined twice.
@openzeppelin/contracts/access/Ownable.sol
So I thought it's better to have ownershipTransferred!!
if you have anyother idea then kindly share it to me!!
src/TransferOwnership.sol
Outdated
|
|
||
| function transferOwnership(address newOwner) public onlyOwner override { | ||
| require(newOwner == address(0),"invalid address"); | ||
| super.transferOwnership(newOwner); |
There was a problem hiding this comment.
calling the transferOwnership function from the Ownable contract to actually transfer the ownership to the new owner!
|
hey @SIDHARTH20K4 I have made some other corrections and I think there were some things missing, you can reply to each comment with |
adding more validations
transfer ownership feature is added with securities.