-
Notifications
You must be signed in to change notification settings - Fork 35
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
Audit/hexens 2nd round part2.2 #209
Conversation
…ownableship is transferred by mistake
|
…anager ownership transfer
b4bc2a0
to
f5df2d4
Compare
abstract contract PoolManagerOwnable2Step is IPoolManagerOwner { | ||
error NotPendingPoolManagerOwner(); | ||
|
||
address private _pendingPoolManagerOwner; |
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 not just make it public?
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.
loll, actually refer this from OZ@Ownable2Step
src/base/PoolManagerOwnable2Step.sol
Outdated
|
||
address private _pendingPoolManagerOwner; | ||
|
||
modifier isFromPendingPoolManagerOwner() { |
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.
prefer naming it to onlyPendingPoolManagerOwner
*/ | ||
abstract contract PoolManagerOwnable2Step is IPoolManagerOwner { | ||
error NotPendingPoolManagerOwner(); | ||
|
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.
do we need event here ?
like "event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);"
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.
agreed
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.
lgtm
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.
LGTM
No description provided.