Azavalla feature/inheritable contract #680
Conversation
contracts/ownership/Heritable.sol
Outdated
| uint public heartbeatTimeout; | ||
|
|
||
| // Timestamp of the owner's death, as pronounced by the heir. | ||
| uint public timeOfDeath; |
There was a problem hiding this comment.
what do you think about using uint256 explicitly?
| require(newHeir != owner); | ||
| heartbeat(); | ||
| HeirChanged(owner, newHeir); | ||
| heir = newHeir; |
There was a problem hiding this comment.
If we provide a setter, I would go with a private visibility for the heir and providing a getter as well. This will ensure that inheriting contracts can not modify the heir in another way than the one proposed.
There was a problem hiding this comment.
we may want the same thing for heartbeatTimeout and timeOfDeath
There was a problem hiding this comment.
Makes sense, but this is not something we're implementing in general in OZ. Eg. Ownable has:
address public owner
There was a problem hiding this comment.
Actually we are caring about this from now on
There was a problem hiding this comment.
Great. I won't be changing it in this PR tho.
I will create an issue instead and ping @azavalla
contracts/ownership/Heritable.sol
Outdated
| } | ||
|
|
||
| function setHeartbeatTimeout(uint newHeartbeatTimeout) internal onlyOwner { | ||
| require(ownerLives()); |
There was a problem hiding this comment.
do we want a heartbeat to be greater than 0?
There was a problem hiding this comment.
What do you mean? Do you mean that we should check for newHeartbeatTimeout > 0?
If so, isn't that implied in the uint type?
| contract SimpleSavingsWallet is Heritable { | ||
|
|
||
| event Sent(address payee, uint amount, uint balance); | ||
| event Received(address payer, uint amount, uint balance); |
There was a problem hiding this comment.
2 things:
- addresses may be indexed?
- we should use
uint256here too
|
LGTM |
…/inheritable-contract Azavalla feature/inheritable contract
Manual merge for PR #595
Applies styling to tests.