Skip to content
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

Gnosis Executor Extension #146

Merged
merged 24 commits into from Mar 30, 2024
Merged

Gnosis Executor Extension #146

merged 24 commits into from Mar 30, 2024

Conversation

rocketman-21
Copy link
Contributor

No description provided.

emit NewPendingAdmin(pendingAdmin);
}

function setAvatar(address _avatar) public onlyAvatar {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only want to let the avatar set another avatar here? or should it be the admin

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense to replace this with an onlyAdmin() modifier, rather than onlyAvatar().

On that, you currently have this same require statement duplicated multiple times require(msg.sender == admin..., I'd suggest putting this into an onlyAdmin() modifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm on second thought - the admin is the DAOLogic contract, which currently doesn't have a function to setAvatar. I think we should call rq to discuss the intended design here

@rocketman-21 rocketman-21 changed the title Gnosis Executor Gnosis Executor Extension Mar 29, 2024
@rocketman-21 rocketman-21 merged commit 7575f0a into main Mar 30, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants