-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ConfidentialVestingWallet/ConfidentialVestingWalletCliff #76
Conversation
e897ec7
to
c593586
Compare
c593586
to
7212873
Compare
f9e27fd
to
7501cdb
Compare
I prefer let @jatZama review it 🙏 |
7501cdb
to
e536334
Compare
*/ | ||
abstract contract ConfidentialVestingWallet { | ||
/// @notice Emitted when tokens are released to the beneficiary address. | ||
event ConfidentialERC20Released(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you didn't keep original OZ specs allowing different tokens? this event should then contain an address argument. It is always better to make standard contracts as generalizable as possible imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted.
IConfidentialERC20 public immutable CONFIDENTIAL_ERC20; | ||
|
||
/// @notice Duration (in seconds). | ||
uint64 public immutable DURATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you instead keep OZ's specs, with private variable and public getter functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to adjust it (for now) because I like that immutable variables that are easy to be spotted. I don't get why OZ does it as such with public getter functions.
This PR adds two Solidity contracts:
ConfidentialVestingWallet.sol
andConfidentialVestingWalletCliff.sol
.